-
Notifications
You must be signed in to change notification settings - Fork 0
docs(binary-protocol): document the unboxed NodeRef content field #477
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from 1 commit
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -654,7 +654,7 @@ pub enum NodeStr<'a> { | |
| pub struct NodeRef<'a> { | ||
| pub tag: NodeStr<'a>, // Borrowed or inline | ||
| pub attrs: AttrsRef<'a>, // Vec<(NodeStr<'a>, ValueRef<'a>)> | ||
| pub content: Option<Box<NodeContentRef<'a>>>, | ||
| pub content: Option<NodeContentRef<'a>>, | ||
| } | ||
|
|
||
| pub enum NodeContentRef<'a> { | ||
|
|
@@ -670,6 +670,14 @@ pub enum NodeContentRef<'a> { | |
| `NodeStr` replaces the previous `Cow<'a, str>` used in `NodeRef`, `AttrsRef`, `ValueRef`, and `NodeContentRef`. The key difference is that the `Owned` variant uses `CompactString` (inline up to 24 bytes) instead of `String` (always heap-allocated), reducing allocation pressure for the many short protocol strings that can't be statically interned. | ||
| </Note> | ||
|
|
||
| <Note> | ||
| `NodeRef::content` dropped its `Box` — it is now `Option<NodeContentRef<'a>>` rather than `Option<Box<NodeContentRef<'a>>>`. Code matching on it via `.as_deref()` should switch to `.as_ref()`; a `Box<NodeContentRef<'_>>` you're holding directly derefs the same as before. | ||
|
|
||
| The box existed only to keep `NodeRef` pointer-sized, but for the commonest node in a fanout — a leaf with `Cow::Borrowed` byte content, which already allocates nothing — the box was the *only* allocation. Removing it cuts allocations per decode by 40% on a fanout (45 → 27) and drops native decode time by 11.2% on both the fanout and large-stanza benchmarks. `size_of::<NodeRef>()` grows from 48 to 72 bytes in exchange, which does not regress any measured shape, including one with no content to begin with. | ||
|
greptile-apps[bot] marked this conversation as resolved.
Outdated
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
The old AGENTS.md reference: AGENTS.md:L25-L25 Useful? React with 👍 / 👎. |
||
|
|
||
| `NodeContentRef::Nodes` keeps its own `Box<NodeVec<'a>>` — that recursion is what makes the type coherent, and removing it isn't part of this change. | ||
|
coderabbitai[bot] marked this conversation as resolved.
Outdated
|
||
| </Note> | ||
|
|
||
| Location: `wacore/binary/src/node.rs:10-106`, `465-469`, `437-441` | ||
|
|
||
| ### Node nesting depth cap | ||
|
|
@@ -936,6 +944,12 @@ The `spilled()` method on `SmallVec` reports whether a given node's attrs overfl | |
|
|
||
| Location: `wacore/binary/src/node.rs`, `wacore/binary/tests/attrs_inline_alloc.rs` | ||
|
|
||
| ### Unboxed NodeRef content | ||
|
|
||
| `NodeRef::content` (`Option<NodeContentRef<'a>>`) has no `Box` around the inner value — see the note under [Zero-copy decoding](#zero-copy-decoding). On `bench_unmarshal_fanout`, this drops allocations per decode from 45 to 27 and instructions by 9.5% under callgrind (the instrument CodSpeed uses), alongside an 11.2% native wall-time improvement on both the fanout and large-stanza benchmarks. `bench_unmarshal_small`, which carries no content, is unaffected either way — the gain tracks how many nodes in the tree carry content, not a fixed per-node cost. | ||
|
coderabbitai[bot] marked this conversation as resolved.
Outdated
|
||
|
|
||
| Location: `wacore/binary/src/node.rs`, `wacore/binary/benches/binary_benchmark.rs` | ||
|
|
||
| ## Common protocol patterns | ||
|
|
||
| ### IQ (info/query) stanzas | ||
|
|
||
Uh oh!
There was an error while loading. Please reload this page.