Skip to content

Fix graft_child_maps below a long root path - #131

Merged
luketpeterson merged 3 commits into
Adam-Vandervorst:masterfrom
imlvts:fix/graft-child-maps-long-root
Sep 25, 2026
Merged

luketpeterson merged 3 commits into
Adam-Vandervorst:masterfrom
imlvts:fix/graft-child-maps-long-root

Conversation

@imlvts

@imlvts imlvts commented Sep 23, 2026

Copy link
Copy Markdown
Collaborator

with_node_at_path joined the focus's node key and the child path in a fixed MAX_NODE_KEY_BYTES buffer, so a root path of 48 bytes or more overflowed it. Fall back to a heap buffer.

with_node_at_path joined the focus's node key and the child path in a
fixed MAX_NODE_KEY_BYTES buffer, so a root path of 48 bytes or more
overflowed it. Fall back to a heap buffer.
… in an unsupported context. The PR's original fix was to remove that limitation, but it did it by allocating a temporary buffer in a situation that could have been a tight loop! This commit fixes the internal function correctly (without the perf foot-gun), and now, as was always the plan, we can use that function to streamling PathMap value insertion (up to 20%)
@imlvts

imlvts commented Sep 25, 2026

Copy link
Copy Markdown
Collaborator Author

Yeah, I agree allocating in a buffer was unnecessary.

Picking up another 10% on PathMap::set_val_at by skipping the temporary WZ creation
@luketpeterson

luketpeterson commented Sep 25, 2026 •

Copy link
Copy Markdown
Collaborator

Merging. I'm not cherry-picking this back to the 0.3.4 release branch because I chose to modify the API (added a method to a trait (WriteZipper) that could have been implemented externally)

Reasons I'm not worried to let the original bug persist in v0.3.x:

  1. The original bug is in the slow path of a function that people would mainly use for its fast path (graft_child_maps).
  2. There is an easy work-around by using a different API to do the same thing.
  3. It's simply a panic. No silent corruption or UB.

@luketpeterson
luketpeterson merged commit deb2cff into Adam-Vandervorst:master Sep 25, 2026
3 of 4 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants