split node files #360

richardpringle · 2023-11-20T20:33:20Z

Add path to branch-node
Split leaf, branch, and extension node files
Remove path again

firewood/src/merkle/node.rs

rkuris · 2023-11-20T22:35:12Z

firewood/src/merkle/node.rs

                let mut items = items.into_iter();
                let decoded_key: Vec<u8> = items.next().unwrap().decode()?;

                let decoded_key_nibbles = Nibbles::<0>::new(&decoded_key);

                let (cur_key_path, term) =
-                    PartialPath::from_nibbles(decoded_key_nibbles.into_iter());
+                    dbg!(PartialPath::from_nibbles(decoded_key_nibbles.into_iter()));


firewood/src/merkle/node.rs

rkuris · 2023-11-20T22:55:32Z

firewood/src/merkle/node.rs

        let meta_raw =
            mem.get_view(addr, META_SIZE as u64)
                .ok_or(ShaleError::InvalidCacheView {
                    offset: addr,
                    size: META_SIZE as u64,
                })?;
-        let attrs = meta_raw.as_deref()[32];
+
+        let attrs = meta_raw.as_deref()[TRIE_HASH_LEN];


every use of meta_raw calls as_deref(). Can we move this to the assignment?

All this code is going away in a future PR

rkuris · 2023-11-20T22:57:24Z

firewood/src/merkle/node.rs

@@ -727,12 +505,15 @@ impl Storable for Node {
                    .collect();

                let (path, _) = PartialPath::decode(&nibbles);
-                let value = Data(remainder.as_deref()[path_len as usize..].to_vec());
-                Ok(Self::new_from_hash(
+                let data = Data(remainder.as_deref()[path_len as usize..].to_vec());


Same thing with remainder. We immediately call as_deref() on every use.

firewood/src/merkle/node.rs

rkuris · 2023-11-20T22:58:35Z

firewood/src/merkle/node.rs

@@ -792,6 +574,7 @@ impl Storable for Node {

        match &self.inner {
            NodeType::Branch(n) => {
+                // TODO: add path


Not sure what this means?

rkuris

This is challenging to review, because I can't tell if changes were made to the data structures without extracting them and doing a different set of diffs.

Would it be possible to split this into 3 parts:
(1) rename leaf.0 and leaf.1 to leaf.key and leaf.data
(2) rename NBRANCH to MAX_CHILDREN
(3) move different node types to different files

richardpringle · 2023-11-20T23:38:29Z

@rkuris

Would it be possible to split this into 3 parts

It would be possible, but it's a lot of work for very little value. You have already identified the three major changes in this PR, and you can rest assured that the functionality hasn't changed when the tests pass. You'll notice that I did not change any of the integration test files here.

You may also notice that there are currently three commits here, one of which will be very easy to remove when continuing to get the roughly 2500 lines of #344 applied. If I didn't have that many more lines to go, I wouldn't push back, but this is the tip of the ice-berg.

Doesn't actually get used yet

rkuris · 2023-11-21T17:57:24Z

It would be possible, but it's a lot of work for very little value. You have already identified the three major changes in this PR, and you can rest assured that the functionality hasn't changed when the tests pass. You'll notice that I did not change any of the integration test files here.

There was some value here, but to review it I had to split it up locally, identifying a few problems I would have otherwise missed; comments coming. Please consider splitting these up in the future; it literally took just over an hour.

firewood/src/shale/mod.rs

rkuris · 2023-11-21T18:08:16Z

firewood/src/merkle/node/partial_path.rs

+    }
+}
+
+impl From<Vec<u8>> for PartialPath {


I don't like this addition. It's now easy for someone to put non-nibbles into a PartialPath. If you really think this is needed as a convenience, it should be TryFrom, and each byte should be checked.

This also ties PartialPath more strongly to a Vec, which is something I'd like to change.

I created #366 suggesting to add convenience methods for extending and shrinking PartialPath. The current problem is that in order to perform any change to the path, you essentially need to copy the PartialPath, and then make changes to the internal Vec and reset the value.

Adding From was a convenience for getting the rest of the "branch with path" implementation further along. I will improve this once it's complete.

rkuris · 2023-11-21T18:10:46Z

firewood/src/merkle/node/branch.rs

+        }
+
+        // TODO: add path
+        let path = Vec::new().into();


PartialPath::new() (or empty() or default())

This code is temporary

rkuris · 2023-11-21T18:13:35Z

firewood/src/merkle/node/leaf.rs

+    #[test_case(0b10 << 4, vec![0x12, 0x34], vec![1, 2, 3, 4]; "even length")]
+    // first nibble is part of the prefix
+    #[test_case((0b11 << 4) + 2, vec![0x34], vec![2, 3, 4]; "odd length")]
+    fn encode_regression_test(prefix: u8, path: Vec<u8>, nibbles: Vec<u8>) {


nit: at a minimum add a comment in the PR that this is a new, not moved, test

Is this just testing partial_path? If you're checking to see if the node is storing it correctly, rather than hardcoding this here (which can change if partial_path changes), perhaps use partial_path and compare that encoding?

Yes, it looks like this is just testing partial-path

I've created #365 to move the test

rkuris · 2023-11-21T18:41:23Z

firewood/src/merkle/node/leaf.rs

+use crate::merkle::{from_nibbles, PartialPath};
+
+#[derive(PartialEq, Eq, Clone)]
+pub struct LeafNode {


nit: LeafNode shouldn't escape the crate, so the inner ones can just be pub, but this one should be pub(crate).

agreed, added to #362

rkuris · 2023-11-21T18:43:03Z

firewood/src/merkle/node/leaf.rs

+        }
+    }
+
+    pub fn path(&self) -> &PartialPath {


nit: since these are pub fn we should reduce the visibility of the members themselves, or get rid of this.

Agreed (both path and data methods are redundant)

#364

rkuris · 2023-11-21T18:44:30Z

firewood/src/merkle/node/leaf.rs

+    #[test_case(0b10 << 4, vec![0x12, 0x34], vec![1, 2, 3, 4]; "even length")]
+    // first nibble is part of the prefix
+    #[test_case((0b11 << 4) + 2, vec![0x34], vec![2, 3, 4]; "odd length")]
+    fn encode_regression_test(prefix: u8, path: Vec<u8>, nibbles: Vec<u8>) {


Is this just testing partial_path? If so, can't we move these tests there? If not, a better choice here would be to call partial_path in this test

rkuris · 2023-11-21T18:46:10Z

firewood/src/merkle/node/partial_path.rs

+    iter::once,
+};
+
+// TODO: use smallvec


comment only: We might even get by with Box<[u8]> since we almost always know how big it will be when we start. I'm not sure how often these grow.

Box<[u8]> would definitely be an improvement, but the goal of smallvec or something like it would be quick stack-copies since we actually copy these partial-paths a lot.

Something like Bytes might be more of a benefit, though, where we store the longest keys on the heap and reference subsets of the keys in the nodes themselves. Again, the goal is a stack-copy when creating a node (branch of leaf) instead of having to allocate copied data.

rkuris

approved with new issues

richardpringle requested review from xinifinity and rkuris as code owners November 20, 2023 20:33

richardpringle force-pushed the split-node-files branch 2 times, most recently from f53251a to dc9001f Compare November 20, 2023 21:47

rkuris reviewed Nov 20, 2023

View reviewed changes

firewood/src/merkle/node.rs Show resolved Hide resolved

rkuris reviewed Nov 20, 2023

View reviewed changes

firewood/src/merkle/node.rs Show resolved Hide resolved

rkuris reviewed Nov 20, 2023

View reviewed changes

firewood/src/merkle/node.rs Show resolved Hide resolved

rkuris reviewed Nov 20, 2023

View reviewed changes

rkuris self-requested a review November 20, 2023 23:03

richardpringle mentioned this pull request Nov 20, 2023

Clean up public api for merkle mod #362

Closed

richardpringle force-pushed the split-node-files branch from dc9001f to 6ac3637 Compare November 20, 2023 23:23

richardpringle enabled auto-merge (squash) November 20, 2023 23:46

richardpringle added 4 commits November 21, 2023 12:11

Add path to branch-node

6e6f938

Doesn't actually get used yet

Used named properties in Leaf-nodes

652d3ba

Rename NBRANCH to MAX_CHILDREN

058ec3e

Split leaf, branch, and extension node files

0451a0c

richardpringle force-pushed the split-node-files branch from 6ac3637 to 8f28cbd Compare November 21, 2023 17:18

Remove dbg! and add licence header

028be27

richardpringle force-pushed the split-node-files branch from 49e1562 to a1e34a9 Compare November 21, 2023 17:28

rkuris mentioned this pull request Nov 21, 2023

pub(crate) or pub(super). We don't want these as part of the public API. #363

Closed

rkuris requested changes Nov 21, 2023

View reviewed changes

This was referenced Nov 21, 2023

Remove redundant getters from LeafNode #364

Closed

Move encode_regression_test from leaf.rs to partial_path.rs with necessary changes #365

Closed

Remove From<Vec<u8>> implementation from PartialPath #366

Closed

Limit DiskAddress visibility

34199e5

richardpringle force-pushed the split-node-files branch from a1e34a9 to ab685a2 Compare November 21, 2023 20:23

richardpringle requested a review from rkuris November 21, 2023 20:27

richardpringle force-pushed the split-node-files branch from 702f671 to 9a78695 Compare November 21, 2023 20:31

richardpringle added 2 commits November 21, 2023 15:34

Merge remote-tracking branch 'origin/main' into split-node-files

d31155e

Remove path again

f12df2b

richardpringle force-pushed the split-node-files branch from 9a78695 to f12df2b Compare November 21, 2023 20:34

richardpringle disabled auto-merge November 22, 2023 17:57

rkuris approved these changes Nov 22, 2023

View reviewed changes

xinifinity approved these changes Nov 22, 2023

View reviewed changes

richardpringle merged commit 9e1fd97 into main Nov 22, 2023

richardpringle deleted the split-node-files branch November 22, 2023 18:54

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

split node files #360

split node files #360

richardpringle commented Nov 20, 2023

rkuris Nov 20, 2023

rkuris Nov 20, 2023

richardpringle Nov 20, 2023

rkuris Nov 20, 2023

rkuris Nov 20, 2023

rkuris left a comment

richardpringle commented Nov 20, 2023

rkuris commented Nov 21, 2023

rkuris Nov 21, 2023

richardpringle Nov 21, 2023

rkuris Nov 21, 2023 •

edited

Loading

richardpringle Nov 21, 2023

rkuris Nov 21, 2023

rkuris Nov 21, 2023

richardpringle Nov 21, 2023

rkuris Nov 21, 2023

richardpringle Nov 21, 2023

rkuris Nov 21, 2023

richardpringle Nov 21, 2023 •

edited

Loading

rkuris Nov 21, 2023

rkuris Nov 21, 2023

richardpringle Nov 21, 2023

rkuris left a comment

split node files #360

split node files #360

Conversation

richardpringle commented Nov 20, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rkuris left a comment

Choose a reason for hiding this comment

richardpringle commented Nov 20, 2023

rkuris commented Nov 21, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rkuris Nov 21, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

richardpringle Nov 21, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rkuris left a comment

Choose a reason for hiding this comment

rkuris Nov 21, 2023 •

edited

Loading

richardpringle Nov 21, 2023 •

edited

Loading