Skip to content

Add reusable zipper buffers - #65

Open
imlvts wants to merge 2 commits into
Adam-Vandervorst:masterfrom
imlvts:zipper-buffer-reuse
Open

imlvts wants to merge 2 commits into
Adam-Vandervorst:masterfrom
imlvts:zipper-buffer-reuse

Conversation

@imlvts

@imlvts imlvts commented Aug 18, 2026 •

Copy link
Copy Markdown
Collaborator

This introduces ZipperBuffer & WriteZipperBuffer. They are different because they store different things.
This allows re-using zipper path and stack Vecs.

fn ReadZipper::into_zipper_buffer(self) -> ZipperBuffer; // keeps allocations, clear path
fn PathMap::read_zipper_with_buffer(&self, ZipperBuffer) -> ReadZipper; // reuse alloc
fn ReadZipper::clone_with_buffer(&self, ZipperBuffer) -> ReadZipper; // clone with reuse alloc

fn WriteZipper::into_zipper_buffer(self) -> WriteZipperBuffer; // keeps allocations, clear path
fn PathMap::write_zipper_with_buffer(&mut self, WriteZipperBuffer) -> WriteZipper; // reuse alloc
fn ZipperHead::write_zipper_at_exclusive_path_with_buffer(&self, path, WriteZipperBuffer) -> Result<WriteZipper, Conflict>; // reuse alloc
unsafe fn ZipperHead::write_zipper_at_exclusive_path_with_buffer_unchecked(&self, path, WriteZipperBuffer) -> WriteZipper; // reuse alloc

@adamv-symbolica

Copy link
Copy Markdown
Contributor

Nice! I quite like the API and naming.
The ancestor_node thing is a little ugly, but the lifetime tagging makes sense.
What's the motivation for going for a new object, rather than just a "disabled RZ" which is a zero-overhead opaque-type wrapper of a RZ?

@luketpeterson

luketpeterson commented Aug 21, 2026 •

Copy link
Copy Markdown
Collaborator

I'm ok with the API concept. I'd prefer different names for detach and attach. And I think relocate has some issues.

The word detach doesn't say enough about what it does. It sounds too fundamental for an optimization like this. We could use the rust idiom for a consuming conversion, i.e. into_buffer or into_zipper_buffer (I prefer the latter). Another option is to make it clear we're destroying the zipper but saving the buffers with salvage_buffer. I think I like into_zipper_buffer.

On the word attach, same issue. That verb sounds really central to what a zipper can do conceptually. Which this API isn't. My preference would be variants of PathMap::read_zipper_with_buffer(&self, buffer: ZipperBuffer<V, A>).

ZipperBuffer::into_read_zipper(map: &'map PathMap<V, A>) could work, but. I have a fairly strong preference for read_zipper_with_buffer because it establishes a pattern that could work with ZipperHead methods too. (_unchecked ZH methods are what MORK will really want, I assume)

As far as relocate, my issue is the same as with attach / ZipperBuffer::into_read_zipper It assumes a PathMap so we'd need to create a different API shape for a ZipperHead. And it is basically a convenience. But anyone who is looking for this API wants runtime performance over convenience.

@adamv-symbolica

Copy link
Copy Markdown
Contributor

I think the most important part not covered there is that you want to take these zippers at a path, you may not have access to the root, nor want to pay the cost of re-descending.

@luketpeterson

Copy link
Copy Markdown
Collaborator

you may not have access to the root

There are two possible interpretations of what you might want: 1. The ZipperBuffer represents the path, and 2. The ZipperBuffer represents the origin_path. An API limited to basing on PathMaps makes this distinction irrelevant.

But if I assume you want 2. it does mean there is now a failure mode where the beginning of the buffer deviates from the origin_path above the ZipperHead location, making it impossible to create zippers with some paths from some ZipperHeads.

So the ZH version of ZipperHead::read_zipper_with_buffer would need to return an Option or Result (Probably a Result, since it is an arg error)

nor want to pay the cost of re-descending.

Some amount of re-descending is unavoidable. There is no way a node stack can be transplanted from one base to another without redoing the work to descend from the new base.

@imlvts
imlvts force-pushed the zipper-buffer-reuse branch from 1c2a2ca to 5e4e6a7 Compare October 24, 2026 00:36
A ZipperBuffer holds the path and ancestor-stack allocations of a read
zipper, so a later zipper can reuse them instead of growing new ones.

- ReadZipper{Untracked,Tracked,Owned}::into_zipper_buffer consumes a zipper
  and keeps its (emptied) buffers
- PathMap::read_zipper_with_buffer creates a zipper at the map root on top
  of a buffer
- ReadZipper{Untracked,Tracked}::clone_with_buffer is Clone, but writes the
  path and ancestor stack into the buffer's allocations

The ancestor stack's element type borrows from the map, so the buffer
stores that allocation with its element type erased; traversal code is
unchanged.
@imlvts
imlvts force-pushed the zipper-buffer-reuse branch from 5e4e6a7 to 81a420c Compare October 24, 2026 00:54
A WriteZipperBuffer holds the path, key-index and node-stack allocations
of a write zipper, so a later write zipper can reuse them.

- WriteZipper{Untracked,Tracked,Owned}::into_zipper_buffer consumes a
  zipper and keeps its (emptied) buffers
- PathMap::write_zipper_with_buffer creates a write zipper at the map root
  on top of a buffer
- ZipperCreation::write_zipper_at_exclusive_path_with_buffer(_unchecked)
  creates a ZipperHead write zipper on top of a buffer
@luketpeterson

luketpeterson commented Oct 24, 2026 •

Copy link
Copy Markdown
Collaborator

To reiterate what I said on discord (for the sake of project memory) I think it's crucial to document why these methods exist. I.e. that they are only optimizations to enable allocations to be reused; they don't provide any functionality beyond performance for rapid zipper destruction and recreation.

There may still be a use for the methods, for the node stack, even after the path buffer is moved outside the zipper. But the people who will want to bother with them is probably limited to a very small handful.

@adamv-symbolica

Copy link
Copy Markdown
Contributor
let mut map = PathMap::<usize>::new();
map.set_val_at(b"ab", 1);
let zh = map.zipper_head();

let (cloned, buffered) = {
    let rz = zh.read_zipper_at_path(b"ab").unwrap();
    let buffer = PathMap::<usize>::new().read_zipper().into_zipper_buffer();
    (rz.clone(), rz.clone_with_buffer(buffer))
};

let _junk = Box::new([0xdead_usize; 2]);
println!("clone()             -> {:?}", cloned.val());
println!("clone_with_buffer() -> {:?}", buffered.val());

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants