[Enhancement] Zero-copy DAG_CBOR into Vec<Cid> parser. #3379

ruseinov · 2023-08-15T14:20:25Z

Summary of changes

Changes introduced in this pull request:

Introduced a deserializer to speed up CBOR > Vec parsing.

Reference issue to close (if applicable)

Closes

Change checklist

I have performed a self-review of my own code,
I have made corresponding changes to the documentation,
I have added tests that prove my fix is effective or that my feature works (if possible),
I have made sure the CHANGELOG is up-to-date. All user-facing changes should be reflected in this document.

src/utils/encoding/cid_de_cbor.rs

aatifsyed

I'm confused - the title says zero-copy serialization of Vec<Cid>, but I'd expect to see a lifetime in that case, deserializing into a &'serial [Cid] or something.

Plus, the in-memory representation of a CID is different to the CBOR serialization, so I'd expect to see a new struct, but I don't - what am I missing?

lemmih · 2023-08-15T15:21:59Z

I'm confused - the title says zero-copy serialization of Vec<Cid>, but I'd expect to see a lifetime in that case, deserializing into a &'serial [Cid] or something.

Plus, the in-memory representation of a CID is different to the CBOR serialization, so I'd expect to see a new struct, but I don't - what am I missing?

The CIDs are copied (which can be done without any heap allocations.) Other data, such as bytes and strings, are not copied.

ruseinov · 2023-08-15T15:28:41Z

I'm confused - the title says zero-copy serialization of Vec<Cid>, but I'd expect to see a lifetime in that case, deserializing into a &'serial [Cid] or something.

Plus, the in-memory representation of a CID is different to the CBOR serialization, so I'd expect to see a new struct, but I don't - what am I missing?

The title does not say anything about serialization whatsoever. It's deserialization of a CBOR-encoded blob into a Vec<Cid>.

We are recursively traversing the DAG_CBOR-encoded blob without parsing everything into a recursive Ipld struct.
All the results are saved into a Vec<Cid>.
That yields 3x performance boost over normal parsing.

ruseinov · 2023-08-15T15:30:32Z

Single-run results cargo forest-tool benchmark car-streaming --inspect forest_snapshot_calibnet_2023-08-02_height_788380.forest.car

 main traversed 87.43 GiB at 163.18 MiB/s in 00:09:08
 this branch traversed 87.43 GiB at 456.14 MiB/s in 00:03:16

Co-authored-by: David Himmelstrup <david.himmelstrup@chainsafe.io>

ruseinov · 2023-08-15T15:42:30Z

I'm confused - the title says zero-copy serialization of Vec<Cid>, but I'd expect to see a lifetime in that case, deserializing into a &'serial [Cid] or something.
Plus, the in-memory representation of a CID is different to the CBOR serialization, so I'd expect to see a new struct, but I don't - what am I missing?

The CIDs are copied (which can be done without any heap allocations.) Other data, such as bytes and strings, are not copied.

Yeah, zero-copy here is a bit misleading, I'll make sure this is documented properly.

lemmih · 2023-08-15T16:06:34Z

I'm confused - the title says zero-copy serialization of Vec<Cid>, but I'd expect to see a lifetime in that case, deserializing into a &'serial [Cid] or something.
Plus, the in-memory representation of a CID is different to the CBOR serialization, so I'd expect to see a new struct, but I don't - what am I missing?

The CIDs are copied (which can be done without any heap allocations.) Other data, such as bytes and strings, are not copied.

Yeah, zero-copy here is a bit misleading, I'll make sure this is documented properly.

I don't think it's inaccurate to call this a zero-copy parser. The PR is doing two things: a) parsing an ipld structure, b) extracting CIDs from Link branches. The parsing part is definitely zero-copy. It's not a generalized zero-copy Ipld parser, though.

aatifsyed · 2023-08-16T08:42:12Z

Ahh I understand now!

The CIDs are copied (which can be done without any heap allocations.) Other data, such as bytes and strings, are not copied.
We are recursively traversing the DAG_CBOR-encoded blob without parsing everything into a recursive Ipld struct.
The PR is doing two things: a) parsing an ipld structure, b) extracting CIDs from Link branches. The parsing part is definitely zero-copy. It's not a generalized zero-copy Ipld parser, though.

Zero-copy in my head means holding references into buffers. Copying CIDs out of it is not zero copy ;)

I think (pre?) filtered deserialization could be a better term here?

aatifsyed · 2023-08-16T08:47:20Z

src/utils/encoding/cid_de_cbor.rs

+            }
+
+            #[inline]
+            fn visit_str<E>(self, _value: &str) -> Result<Self::Value, E>


nonblocking: can you macro these away with a comment? Or at least lift the most relevant fns to the top?
"the default visitor errors when visiting these, but we want to skip over them, so return Ok(())"
Out of curiosity why have you only done that for some of the visited types?

We only care about list, map and newtype_struct, because a list and a map could contain more of those and the only type that converts to a CID is the newtype_struct.

I don't think that introducing a macro here is going to help anything, it's just going to obfuscate the obvious. I'll re-arrange the order, however.

aatifsyed · 2023-08-16T08:53:25Z

src/utils/encoding/cid_de_cbor.rs

+impl Deref for CidVec {
+    type Target = Vec<Cid>;
+
+    fn deref(&self) -> &Self::Target {
+        &self.0
+    }
+}


Implementing Deref for smart pointers makes accessing the data behind them convenient, which is why they implement Deref. On the other hand, the rules regarding Deref and DerefMut were designed specifically to accommodate smart pointers. Because of this, Deref should only be implemented for smart pointers to avoid confusion.

https://doc.rust-lang.org/std/ops/trait.Deref.html (emphasis not mine)

Not a smart pointer

Not a smart pointer, but since we have a thin wrapper here strictly for deserialization that also explicitly states it is a ‘Vec’ - it’s not surprising that we want to use a ‘CidVec’ as it’s underlying ‘Vec’, without any explicit conversions.

I guess it’s a “choose your poison” kind of thing and not using helpful abstractions in this case seems wasteful.

I would have gone with into_inner here.

Alright, into_inner it is then.

not using helpful abstractions in this case seems wasteful.

It's the wrong abstraction, and a commonly misused one at that, which is why it's called out in the docs

It's the wrong abstraction, and a commonly misused one at that, which is why it's called out in the docs

Sure is, though I'm still of the opinion that thin wrappers could benefit a lot from this. I would go further and say that the need for such wrappers to override certain behaviours calls for something magic/transparent to make the wrapper adhere to the same interface as whatever is wrapped. I wish there was a more eloquent way of implementing custom ser/de for types, e.g. the ability to have an implementation for a type alias.

Personal opinions aside - this has been fixed.

src/utils/encoding/cid_de_cbor.rs

Co-authored-by: Aatif Syed <38045910+aatifsyed@users.noreply.github.com>

lemmih · 2023-08-16T09:38:35Z

Ahh I understand now!

The CIDs are copied (which can be done without any heap allocations.) Other data, such as bytes and strings, are not copied.
We are recursively traversing the DAG_CBOR-encoded blob without parsing everything into a recursive Ipld struct.
The PR is doing two things: a) parsing an ipld structure, b) extracting CIDs from Link branches. The parsing part is definitely zero-copy. It's not a generalized zero-copy Ipld parser, though.

Zero-copy in my head means holding references into buffers. Copying CIDs out of it is not zero copy ;)

Imagine a zero-copy parser that returns an Ipld structure with references instead of owned data. Then imagine traversing that structure to extract the CIDs. This PR does both at once, with the intermediate structure deforested out.

lemmih · 2023-08-16T09:43:30Z

CHANGELOG.md

+- [#3379](https://github.com/ChainSafe/forest/pull/3379): Zero-copy DAG_CBOR
+  into Vec<Cid> parser.


This is not user-visible addition. I don't think it should be in the CHANGELOG. If we want a CHANGELOG entry, it should be under Changed, and it should say the performance was improved when walking the state graph.

Sounds good.

aatifsyed · 2023-08-16T13:09:46Z

src/utils/encoding/cid_de_cbor.rs

+/// [`CidVec`] allows for efficient zero-copy de-serialization of `DAG_CBOR`-encoded nodes into a
+/// vector of [`Cid`].
+#[derive(Default)]
+pub struct CidVec(Vec<Cid>);


I don't really like this name -it's too general - it was the original name for what is now FrozenCids, for example.

Can you think of a better name, or maybe different API? pub fn filter_cids(_: ???)

Yeah, it's a good point. My idea behind this name is exactly that - this represents a Vec<Cid>. If we had a better way of dealing with ser/de without a wrapper - this wouldn't be needed.

Perhaps a good middleground is to do something like: pub fn extract_cids(cbor_blob: &bytes): Vec<Cid>.

LesnyRumcajs

Is this feature covered by unit tests?

ruseinov · 2023-08-16T13:49:48Z

Is this feature covered by unit tests?

Good point, it actually isn't.

src/utils/encoding/cid_de_cbor.rs

LesnyRumcajs · 2023-08-17T14:43:28Z

src/utils/encoding/cid_de_cbor.rs

+                    // Cleaning up Integer and Float in order to avoid parser mistakes that result
+                    // in tag detection and a subsequent Cid decoding failure.
+                    // See https://github.com/ipld/serde_ipld_dagcbor/blob/master/src/de.rs#L178 and
+                    // https://github.com/ipld/serde_ipld_dagcbor/blob/master/src/de.rs#L119 .


This could use more explanation, for someone fairly fresh to this subject it's a bit shady to replace all numeric occurrences with zeroes.

Also, let's use permalinks, otherwise those links will get quickly outdated and point to random pieces of code (or 404).

Sounds good.

ruseinov · 2023-08-17T15:10:17Z

Follow-up: create an issue to tackle roundtrip ser/de issues mentioned here:

forest/src/utils/encoding/cid_de_cbor.rs

Lines 218 to 228 in ec7e65b

    
           // Cleaning up Integer and Float in order to avoid parser mistakes that result 
        
           // in tag detection and a subsequent Cid decoding failure. 
        
           // Otherwise the `serde_ipld_dagcbor` library incorrectly treats some of those 
        
           // values as [`cbor4ii::core::major::TAG`] and tries to deserialize a [`Cid`] 
        
           // from it. 
        
           // See https://github.com/ipld/serde_ipld_dagcbor/blob/37f6f00408331b76c6dac8ec4dc08a85d7764cec/src/de.rs#L178 and 
        
           // https://github.com/ipld/serde_ipld_dagcbor/blob/37f6f00408331b76c6dac8ec4dc08a85d7764cec/src/de.rs#L119. 
        
           // 
        
           // Note that we don't actually care about what integer or float contain for 
        
           // these tests, because our deserializer ignores those as it only cares about 
        
           // maps, lists and [`Cid`]s.

LesnyRumcajs

rock-solid

ruseinov added 5 commits August 15, 2023 16:19

[Enhancement] Zero-copy DAG_CBOR into Vec<Cid> parser.

eca8ffe

more comments

8795daf

add header

1383144

a little fix

c0ffc53

fix some warns

a0f7f81

lemmih reviewed Aug 15, 2023

View reviewed changes

src/utils/encoding/cid_de_cbor.rs Outdated Show resolved Hide resolved

ruseinov added 2 commits August 15, 2023 16:50

unused imports

b6c649d

remove rendundant code

b6c7289

aatifsyed reviewed Aug 15, 2023

View reviewed changes

more comments

a83b395

ruseinov and others added 2 commits August 15, 2023 17:32

Update src/utils/encoding/cid_de_cbor.rs

d711f0c

Co-authored-by: David Himmelstrup <david.himmelstrup@chainsafe.io>

Merge branch 'main' into ru/feature/streaming-inspect-bench

557ddfb

some clippy fixes

1b2d6a0

fix comments

3daed61

ruseinov marked this pull request as ready for review August 15, 2023 21:54

ruseinov requested a review from a team as a code owner August 15, 2023 21:54

ruseinov requested review from elmattic and aatifsyed and removed request for a team August 15, 2023 21:54

CHANGELOG update

cccd31f

aatifsyed reviewed Aug 16, 2023

View reviewed changes

Update src/utils/encoding/cid_de_cbor.rs

c50f79c

Co-authored-by: Aatif Syed <38045910+aatifsyed@users.noreply.github.com>

lemmih reviewed Aug 16, 2023

View reviewed changes

ruseinov added 3 commits August 16, 2023 12:01

fix comments

97b8c88

fix docs

0c794b1

Merge branch 'main' into ru/feature/streaming-inspect-bench

6224445

aatifsyed reviewed Aug 16, 2023

View reviewed changes

aatifsyed approved these changes Aug 16, 2023

View reviewed changes

ruseinov mentioned this pull request Aug 16, 2023

[Enhancement] DFS is using a faster parser now. #3384

Merged

4 tasks

cleaner api

f467c3f

LesnyRumcajs reviewed Aug 16, 2023

View reviewed changes

aatifsyed reviewed Aug 16, 2023

View reviewed changes

src/utils/encoding/cid_de_cbor.rs Outdated Show resolved Hide resolved

[Enhancement] DFS is using a faster parser now. (#3384)

52ed7e9

ruseinov marked this pull request as draft August 17, 2023 14:02

ruseinov marked this pull request as ready for review August 17, 2023 14:16

ruseinov added 3 commits August 17, 2023 16:19

Ru/enhancement/cid parser dfs (#3393)

eca15fb

Merge branch 'main' into ru/feature/streaming-inspect-bench

fcb9f58

fix

a8000d8

LesnyRumcajs reviewed Aug 17, 2023

View reviewed changes

more comments and better links

ec7e65b

ruseinov enabled auto-merge August 17, 2023 15:08

ruseinov mentioned this pull request Aug 17, 2023

Tackle roundtrip ser/de issues related to arbitrary Float and Int #3395

Closed

ruseinov added this pull request to the merge queue Aug 17, 2023

LesnyRumcajs approved these changes Aug 17, 2023

View reviewed changes

github-merge-queue bot removed this pull request from the merge queue due to failed status checks Aug 17, 2023

ruseinov added this pull request to the merge queue Aug 17, 2023

github-merge-queue bot removed this pull request from the merge queue due to failed status checks Aug 17, 2023

ruseinov added this pull request to the merge queue Aug 17, 2023

Merged via the queue into main with commit 7e2f19a Aug 18, 2023
20 checks passed

ruseinov deleted the ru/feature/streaming-inspect-bench branch August 18, 2023 00:00

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Enhancement] Zero-copy DAG_CBOR into Vec<Cid> parser. #3379

[Enhancement] Zero-copy DAG_CBOR into Vec<Cid> parser. #3379

ruseinov commented Aug 15, 2023 •

edited

aatifsyed left a comment

lemmih commented Aug 15, 2023

ruseinov commented Aug 15, 2023 •

edited

ruseinov commented Aug 15, 2023

ruseinov commented Aug 15, 2023

lemmih commented Aug 15, 2023

aatifsyed commented Aug 16, 2023

aatifsyed Aug 16, 2023

ruseinov Aug 16, 2023

ruseinov Aug 16, 2023

aatifsyed Aug 16, 2023

ruseinov Aug 16, 2023

ruseinov Aug 16, 2023

lemmih Aug 16, 2023

ruseinov Aug 16, 2023

aatifsyed Aug 16, 2023

ruseinov Aug 16, 2023

lemmih commented Aug 16, 2023

lemmih Aug 16, 2023

ruseinov Aug 16, 2023

aatifsyed Aug 16, 2023

ruseinov Aug 16, 2023

ruseinov Aug 16, 2023

LesnyRumcajs left a comment

ruseinov commented Aug 16, 2023

LesnyRumcajs Aug 17, 2023

ruseinov Aug 17, 2023

ruseinov Aug 17, 2023

ruseinov commented Aug 17, 2023

LesnyRumcajs left a comment

		- [#3379](https://github.com/ChainSafe/forest/pull/3379): Zero-copy DAG_CBOR
		into Vec<Cid> parser.

[Enhancement] Zero-copy DAG_CBOR into Vec<Cid> parser. #3379

[Enhancement] Zero-copy DAG_CBOR into Vec<Cid> parser. #3379

Conversation

ruseinov commented Aug 15, 2023 • edited

Summary of changes

Reference issue to close (if applicable)

Other information and links

Change checklist

aatifsyed left a comment

Choose a reason for hiding this comment

lemmih commented Aug 15, 2023

ruseinov commented Aug 15, 2023 • edited

ruseinov commented Aug 15, 2023

ruseinov commented Aug 15, 2023

lemmih commented Aug 15, 2023

aatifsyed commented Aug 16, 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

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

lemmih commented Aug 16, 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

LesnyRumcajs left a comment

Choose a reason for hiding this comment

ruseinov commented Aug 16, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ruseinov commented Aug 17, 2023

LesnyRumcajs left a comment

Choose a reason for hiding this comment

ruseinov commented Aug 15, 2023 •

edited

ruseinov commented Aug 15, 2023 •

edited