Delegate branch-storage logic to the branch mod #373

richardpringle · 2023-11-22T22:57:57Z

There is a code move here and well as some code changes. They are unfortunately in one commit. Breaking it up would mean re-implementing it and possibly introducing a bug.

I will be doing the same thing with both leaf and extension nodes.

Upon reviewing my own code, I see that this is actually just serialization. There's a todo!() that will come later for deserialization.

rkuris

Partial review, will finish on the moved bits later

rkuris · 2023-11-23T02:28:35Z

firewood/src/merkle/node.rs


-                let offset = if raw_len == u32::MAX as u64 {
+                let offset = if raw_len == u32::MAX {


rkuris · 2023-11-23T02:32:39Z

firewood/src/merkle/node/branch.rs

+enum BranchDataLength {
+    None,
+    Length(u32),
+}
+


This is confusing; why is this not an Option<u32>, or even better, Option<NonZeroU32>?

If zero is a valid value, consider https://crates.io/crates/nonmax

This way you get niche optimization with None so the size is smaller and serialization is easier.

Would it be less confusing if it was

enum BranchDataLength { NoData, /// `0` is a valid length, *empty* data and *no* data are treated separately Length(u32), }

Shepmaster put this pretty nicely in a comment on SO:

I'll just have to do more profiling to see if it's truly worth it. Using less bytes seems like an obvious win; using less bytes and mandatory math everywhere is less sure-fire.

(emphasis mine)

In any case, I didn't end up using this code. I'll delete it.

deleted 313cae7

rkuris · 2023-11-23T02:39:27Z

firewood/src/merkle/node/branch.rs

@@ -179,3 +212,59 @@ impl BranchNode {
            .unwrap()
    }
 }
+


I will have to review this later to verify it didn't change.

You actually do not have to verify that it didn't change. What you have to verify is that we have tests that make sure that things that we serialize can also be deserialized since we have no burden of backward compatibility (yet).

🤔 , now that I say that... I should have explicit tests for this. This is actually tested in all the merkle.rs tests because of this line, but that's pretty fragile.

I created a new issue: #375

rkuris · 2023-11-23T02:40:22Z

firewood/src/shale/disk_address.rs

@@ -30,6 +30,8 @@ impl DerefMut for DiskAddress {
 }

 impl DiskAddress {
+    pub const MSIZE: u64 = size_of::<Self>() as u64;


why was the visibility increase needed?

It's used in (de)serialization

pub(crate) please

rkuris

approved, almost all nits

rkuris · 2023-11-27T17:14:11Z

firewood/src/merkle/node/branch.rs

+}
+
+fn optional_data_len<Len, T: AsRef<[u8]>>(data: Option<T>) -> u64 {
+    size_of::<Len>() as u64 + data.as_ref().map_or(0, |data| data.as_ref().len() as u64)


Remove unnecessary as_ref(), which is just changing Option to Option<&T>:

Suggested change

size_of::<Len>() as u64 + data.as_ref().map_or(0, |data| data.as_ref().len() as u64)

size_of::<Len>() as u64 + data.map_or(0, |data| data.as_ref().len() as u64)

I wonder why clippy doesn't care about this.

rkuris · 2023-11-27T17:19:00Z

firewood/src/merkle/node/branch.rs

+    }
+}
+
+fn optional_data_len<Len, T: AsRef<[u8]>>(data: Option<T>) -> u64 {


This method needs a comment that describes what it does, and maybe better names, so if you mouse over it you can see what it does:

Suggested change

fn optional_data_len<Len, T: AsRef<[u8]>>(data: Option<T>) -> u64 {

/// Adds the size of some base object plus the length of some optional data

fn base_plus_optional_data_len<Base, T: AsRef<[u8]>>(data: Option<T>) -> u64 {

I'm also not convinced this is any more readable than either (a) just inlining the size_of part; or (b) doing both parts in the callers.

rkuris · 2023-11-27T17:22:54Z

firewood/src/merkle/node/branch.rs

+            .value
+            .as_ref()
+            .map(|val| (val.len() as DataLen, val.deref()))
+            .unwrap_or((DataLen::MAX, &[]));


nit: This needs a new const somewhere (with a comment) since this isn't actually MAX. Maybe DataLen::None? Even better would be for this to be an Option<T> where the value of None is DataLen::MAX.

rkuris · 2023-11-27T17:29:19Z

firewood/src/merkle/node/branch.rs

+        let children_encoded_len = self.children_encoded.iter().fold(0, |len, child| {
+            len + optional_data_len::<EncodedChildLen, _>(child.as_ref())


nit: readability:

Suggested change

let children_encoded_len = self.children_encoded.iter().fold(0, |len, child| {

len + optional_data_len::<EncodedChildLen, _>(child.as_ref())

let children_encoded_len = self.children_encoded.iter().fold(0, |acc_len, child| {

acc_len + optional_data_len::<EncodedChildLen, _>(child.as_ref())

rkuris · 2023-11-27T17:57:02Z

firewood/src/merkle/node/branch.rs

+            let (child_len, child) = child_encoded
+                .as_ref()
+                .map(|child| (child.len() as EncodedChildLen, child.as_slice()))
+                .unwrap_or((EncodedChildLen::MIN, &[]));


similar comment. Using MAX and MIN is magics reduces readability

richardpringle · 2023-11-29T04:37:59Z

@rkuris, I see your nits and raise you one
#394

I think storage could be done in a much more idiomatic way.

richardpringle force-pushed the move-storable-logic-to-branch branch from f5a3c51 to 2c794ea Compare November 22, 2023 22:59

richardpringle force-pushed the refactor-node-serialization branch 2 times, most recently from d1650bd to fd6e3b1 Compare November 22, 2023 23:25

richardpringle force-pushed the move-storable-logic-to-branch branch from 2c794ea to 751b7fa Compare November 22, 2023 23:26

richardpringle force-pushed the refactor-node-serialization branch from fd6e3b1 to 49a0e8a Compare November 23, 2023 00:16

richardpringle force-pushed the move-storable-logic-to-branch branch from 751b7fa to cc490cf Compare November 23, 2023 00:21

Base automatically changed from refactor-node-serialization to main November 23, 2023 00:23

richardpringle marked this pull request as ready for review November 23, 2023 00:26

richardpringle requested review from xinifinity and rkuris as code owners November 23, 2023 00:26

richardpringle force-pushed the move-storable-logic-to-branch branch from cc490cf to 0dceb44 Compare November 23, 2023 00:26

rkuris reviewed Nov 23, 2023

View reviewed changes

rkuris approved these changes Nov 27, 2023

View reviewed changes

xinifinity approved these changes Nov 27, 2023

View reviewed changes

richardpringle force-pushed the move-storable-logic-to-branch branch from 313cae7 to 9fa4827 Compare November 29, 2023 03:10

richardpringle added 2 commits November 28, 2023 23:18

Delegate branch-storage logic to the branch mod

ad1e027

Remove unused BranchDataLength

4c9f31b

richardpringle force-pushed the move-storable-logic-to-branch branch from 9fa4827 to 4c9f31b Compare November 29, 2023 04:19

richardpringle merged commit 4a4f4ba into main Nov 29, 2023
5 checks passed

richardpringle deleted the move-storable-logic-to-branch branch November 29, 2023 04:38

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Delegate branch-storage logic to the branch mod #373

Delegate branch-storage logic to the branch mod #373

richardpringle commented Nov 22, 2023 •

edited

Loading

rkuris left a comment

rkuris Nov 23, 2023

rkuris Nov 23, 2023

richardpringle Nov 23, 2023

richardpringle Nov 23, 2023

rkuris Nov 23, 2023

richardpringle Nov 23, 2023

richardpringle Nov 23, 2023

rkuris Nov 23, 2023

richardpringle Nov 23, 2023 •

edited

Loading

rkuris Nov 27, 2023

rkuris left a comment

rkuris Nov 27, 2023

rkuris Nov 27, 2023

rkuris Nov 27, 2023

rkuris Nov 27, 2023

rkuris Nov 27, 2023

richardpringle commented Nov 29, 2023


		let offset = if raw_len == u32::MAX as u64 {
		let offset = if raw_len == u32::MAX {

	size_of::<Len>() as u64 + data.as_ref().map_or(0, \|data\| data.as_ref().len() as u64)
	size_of::<Len>() as u64 + data.map_or(0, \|data\| data.as_ref().len() as u64)

	fn optional_data_len<Len, T: AsRef<[u8]>>(data: Option<T>) -> u64 {
	/// Adds the size of some base object plus the length of some optional data
	fn base_plus_optional_data_len<Base, T: AsRef<[u8]>>(data: Option<T>) -> u64 {

		let children_encoded_len = self.children_encoded.iter().fold(0, \|len, child\| {
		len + optional_data_len::<EncodedChildLen, _>(child.as_ref())

Delegate branch-storage logic to the branch mod #373

Delegate branch-storage logic to the branch mod #373

Conversation

richardpringle commented Nov 22, 2023 • edited Loading

rkuris left a 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

richardpringle Nov 23, 2023 • edited Loading

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

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 commented Nov 29, 2023

richardpringle commented Nov 22, 2023 •

edited

Loading

richardpringle Nov 23, 2023 •

edited

Loading