Replacing a vector of paths with PathMap in zipper excl. tracking by marcin-rzeznicki · Pull Request #4 · Adam-Vandervorst/PathMap

marcin-rzeznicki · 2024-10-23T13:02:27Z

Plus a couple of related changes:

replacing panic with error,
more precise types.

Changes

`ZipperTracker` is now parametrized by `TrackingMode`

TrackingMode serves as a type-level indicator whether we're tracking writes or reads. In Rust you cannot do much with this - but at the very least we don't need to panic at runtime e.g. in Clone. Also various constructors throughout the code are aware of what mode they expect, which may be helpful in further development as there is no way to mix read-tracking zippers with write-tracking zippers.

`BytesTrieMap` is used instad of `Vec` to track locked paths (`SharedTrackerPaths`)

This helps the complexity as checking no longer needs traversing all of the n paths and checking their prefixes.
The replacement uses BytesTrieMap as a prefix tree annotated with IsTracking values that designate the subtree as locked for writing, or reading (unsigned non-zero counter)

Replacing panic with error

The Conflict type is implemented, serving as a structured error info, and methods that previously panicked (and were poisoning the locks) now return Result<(), Conflict> allowing the caller to graciously handle read/write conflicts

What does not work?

Tests - I have not finished adapting them - they don't eve compile currently. Will fix in a subsequent PR

Things to scrutinize

Usage of zippers may be imprecise. Please check methods that rely on certain zipper/trie behavior e.g. Conflict::check_subtree_for_conflict, SharedTrackPaths::try_add_writer, SharedTrackPaths::try_add_reader

I am sorry for

Reformatting a lot of code which makes the PR much bigger. This is my lack of tooling knowledge showing up (I don't know how to turn it off 😄 ). Perhaps we could have some kind of project-wide formatting config to avoid this stylistic inconsistency?

luketpeterson · 2024-10-29T08:47:42Z

Ugh... I manually merged this with all the latest changes, but it seems I don't have the permissions to push back to this branch.

Adam-Vandervorst · 2024-10-29T15:00:48Z

    /// A more efficient version of [read_zipper_at_path](Self::read_zipper_at_path), where the returned
    /// zipper is constrained by the `'path` lifetime


We need to justify the failure mode of this function, i.e. "why could it be the case that I can't read at a path" in its docs.

Adam-Vandervorst · 2024-10-29T15:05:48Z

-            let zipper_tracker = ZipperTracker::new_read_tracker(self.tracker_paths.clone(), path);
-            ReadZipperUntracked::new_with_node_and_cloned_path(root.borrow(), path.as_ref(), Some(path.len()), Some(zipper_tracker))
+            let zipper_tracker =
+                ZipperTracker::<TrackingRead>::new_no_check(self.tracker_paths.clone(), path);


read_zipper_at_path_unchecked is a very hot method, is it possible to avoid the clone here? How deep is this clone? Debug mode is close to uselessly slow as is, so we have to avoid any additional "order of magnitude" reduction in speed.

I think it clones Arc - which results in just increasing ref count. https://doc.rust-lang.org/src/alloc/sync.rs.html#2090

Adam-Vandervorst · 2024-10-29T15:10:03Z

    /// Creates a new [WriteZipper] with the specified path from the `ZipperHead`
-    pub fn write_zipper_at_exclusive_path<'k, K: AsRef<[u8]>>(&self, path: K) -> WriteZipperTracked<'a, 'k, V> {
+    pub fn write_zipper_at_exclusive_path<'k, K: AsRef<[u8]>>(
+        &self,
+        path: K,
+    ) -> Result<WriteZipperTracked<'a, 'k, V>, Conflict> {


With Trackers being more internalized now, perhaps we should rename this method to "write_zipper_at_path", and the result indicates whether or not the path was exclusive or not.

@luketpeterson ⏫ I'm leaving it for you to decide

Adam-Vandervorst · 2024-10-29T15:19:13Z

        //Make a ZipperHead for the whole map, and two child zippers
        let map_head = map.zipper_head();
-        let mut a_zipper = map_head.write_zipper_at_exclusive_path(b"a");
-        let mut b_zipper = map_head.write_zipper_at_exclusive_path(b"b");
+        let mut a_zipper = map_head.write_zipper_at_exclusive_path(b"a").unwrap();
+        let mut b_zipper = map_head.write_zipper_at_exclusive_path(b"b").unwrap();

        //Make a separate ZipperHead on each WriteZipper
        let a_head = a_zipper.zipper_head();
        let b_head = b_zipper.zipper_head();

        //Make some WriteZippers on each head
-        let mut a0_zipper = a_head.write_zipper_at_exclusive_path(b"0");
-        let mut a1_zipper = a_head.write_zipper_at_exclusive_path(b"1");
-        let mut b0_zipper = b_head.write_zipper_at_exclusive_path(b"0");
-        let mut b1_zipper = b_head.write_zipper_at_exclusive_path(b"1");
+        let mut a0_zipper = a_head.write_zipper_at_exclusive_path(b"0").unwrap();
+        let mut a1_zipper = a_head.write_zipper_at_exclusive_path(b"1").unwrap();
+        let mut b0_zipper = b_head.write_zipper_at_exclusive_path(b"0").unwrap();
+        let mut b1_zipper = b_head.write_zipper_at_exclusive_path(b"1").unwrap();


Seems like we should reduce the synthetic overhead here if possible. Is it possible for users to use the ? operator? As mentioned, the "exclusive" seems to be captured by the unwrap here.

Makes perfect sense. Agreed

Adam-Vandervorst · 2024-10-29T15:22:14Z

+#[derive(Debug)]
+pub struct Conflict {
+    with: IsTracking,
+    at: Vec<u8>,


Seems like every context Conflicts occur in, you'd have access to the reference of the path? If people are expecting a Conflict, copying the path is a silly price to pay.

Yes, perhaps, but the path does not have to be simply a clone of the original path, as conflicts can occur, depending on the usage pattern, below or above the passed path.

Adam-Vandervorst · 2024-10-29T15:51:25Z

+        self.modify_at(path, try_add_reader_internal)
+    }
+
+    fn add_reader_unchecked(&self, path: &[u8]) {


What's the use of this function, or in what notion is this unchecked? This object being used on a relevant path seems mutually exclusive to at_path_unchecked being used.

It's only used for cloning tracked zippers right now. The difference is that it simply increments the counter, without checking for exclusivity. In this case it is actually safe, because during cloning you obviously do not thread on a writer.

Adam-Vandervorst · 2024-10-29T15:54:52Z

+/// A shared registry of every outstanding zipper
 #[derive(Clone, Default)]
-pub(crate) struct SharedTrackerPaths(Arc<RwLock<ZipperPaths>>);
+pub(crate) struct SharedTrackerPaths(Arc<RwLock<BytesTrieMap<IsTracking>>>);


Why is this Arc and RwLock, only one thread may have access to the tracking?

Can drop happen on a different thread?

Plus a couple of related changes: - replacing panic with error, - more precise types

Cherry picked from 798815e

luketpeterson

Looks fine except for the build-breaking change in release mode. Which should simply be reverted.

luketpeterson · 2024-11-15T02:31:24Z

I resolved the merge in my 8ceb571 commit.

I also attached the diff. zipper_head_ext_merge_diff.txt

…ipper::into_read_zipper

marcin-rzeznicki · 2024-11-15T10:18:34Z

I resolved the merge in my 8ceb571 commit.

I also attached the diff. zipper_head_ext_merge_diff.txt

Thank you @luketpeterson . Cherry picked as 06477da

marcin-rzeznicki requested review from Adam-Vandervorst, ClarkeRemy and luketpeterson October 23, 2024 13:02

marcin-rzeznicki self-assigned this Oct 23, 2024

Adam-Vandervorst assigned luketpeterson Oct 29, 2024

Adam-Vandervorst reviewed Oct 29, 2024

View reviewed changes

marcin-rzeznicki added 4 commits November 6, 2024 13:50

Replacing a vector of paths with PathMap in zipper excl. tracking

55bd031

Plus a couple of related changes: - replacing panic with error, - more precise types

Adding better error reporting when ZipperTracker is panicking

0c4e484

adding ascend_until_value helper to Zipper trait

11862fa

A better algorithm for zipper excl. tracking

c947ca1

marcin-rzeznicki force-pushed the zipper_head_ext branch from 31e36f0 to c947ca1 Compare November 8, 2024 12:40

Checking zipper paths in debug mode, in the _unchecked calls.

d6fdbae

Cherry picked from 798815e

luketpeterson requested changes Nov 13, 2024

View reviewed changes

Comment thread src/zipper_head.rs Outdated

marcin-rzeznicki added 3 commits November 13, 2024 18:00

Split reads and writes tracking for better performance

3ae18f7

Fix compilation when debug assertions are on

9d84b45

Removing no_check methods, unused

139a350

marcin-rzeznicki requested review from Adam-Vandervorst and luketpeterson November 14, 2024 14:50

Merge zipper_head with zipper_head_ext, adding better test for WriteZ…

06477da

…ipper::into_read_zipper

Merge branch 'zipper_head' into zipper_head_ext

1050d82

luketpeterson merged commit 88f3256 into zipper_head Nov 16, 2024

marcin-rzeznicki deleted the zipper_head_ext branch April 19, 2026 02:33

MesTTo mentioned this pull request Jul 15, 2026

Add zipper-type property tests: same_trie, seeded ring algebra, long-path serialization #48

Open

2 tasks

		/// A more efficient version of [read_zipper_at_path](Self::read_zipper_at_path), where the returned
		/// zipper is constrained by the `'path` lifetime

Conversation

marcin-rzeznicki commented Oct 23, 2024

Changes

ZipperTracker is now parametrized by TrackingMode

BytesTrieMap is used instad of Vec to track locked paths (SharedTrackerPaths)

Replacing panic with error

What does not work?

Things to scrutinize

I am sorry for

Uh oh!

luketpeterson commented Oct 29, 2024

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

luketpeterson left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

luketpeterson commented Nov 15, 2024

Uh oh!

marcin-rzeznicki commented Nov 15, 2024

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

`ZipperTracker` is now parametrized by `TrackingMode`

`BytesTrieMap` is used instad of `Vec` to track locked paths (`SharedTrackerPaths`)