Node from stream backrefs optimisation #532

matt-o-how · 2025-01-13T16:09:52Z

Use a Vec<NodePtr> stack instead of NodePtr / SExps in node_from_stream_backrefs and add a new traverse_path_with_vec() function to handle backrefs

coveralls-official · 2025-01-13T16:19:03Z

Pull Request Test Coverage Report for Build 13076959208

Details

147 of 147 (100.0%) changed or added relevant lines in 2 files are covered.
No unchanged relevant lines lost coverage.
Overall coverage increased (+0.2%) to 94.061%

Totals
Change from base Build 12933937517:	0.2%
Covered Lines:	6208
Relevant Lines:	6600

💛 - Coveralls

arvidn

it looks correct, as far as I can tell. I think we need tests for all interesting cases, to make sure it works. I'm also interested in seeing a benchmark. Does this make a difference? I would expect it to at least use less memory, which typically means faster on small machines (like Raspberry PI)

arvidn · 2025-01-13T16:47:04Z

src/traverse_path.rs

@@ -72,6 +72,83 @@ pub fn traverse_path(allocator: &Allocator, node_index: &[u8], args: NodePtr) ->
    Ok(Reduction(cost, arg_list))
 }

+pub fn traverse_path_with_vec(


it would be good to have unit tests for this function

arvidn · 2025-01-13T16:53:26Z

src/serde/de_br.rs

@@ -22,7 +22,7 @@ pub fn node_from_stream_backrefs(
    f: &mut Cursor<&[u8]>,
    mut backref_callback: impl FnMut(NodePtr),
 ) -> io::Result<NodePtr> {
-    let mut values = allocator.nil();
+    let mut values = Vec::<NodePtr>::new();


one idea I had was that you could make this Vec<(NodePtr, Option<NodePtr>)>, where the optional NodePtr is a cache of nodes you've created for this stack "link", in case there are multiple references to the same one.

I think this is required to avoid consensus problems. consider the worst case scenario where you have a tree that's then referenced thousands of times. With todays deserializer, they will point to the same tree, but in your version, you'll create the same pairs over and over again. In fact, I would expect the fuzzer to fail if you let it run long enough because of this.

arvidn · 2025-01-13T16:54:56Z

src/traverse_path.rs

+    // find first non-zero byte
+    let first_bit_byte_index = first_non_zero(node_index);
+
+    let mut cost: Cost = TRAVERSE_BASE_COST


this version doesn't need to track cost, I don't think. In fact, I think this is sufficiently different (and specialized) that it makes sense to move it into the de_br.rs file.

arvidn · 2025-01-13T16:58:07Z

src/traverse_path.rs

+    let mut bitmask = 0x01;
+
+    // if we move from parsing the Vec stack to parsing the SExp stack use the following variables
+    let mut parsing_sexp = false;


it might be simpler to have a separate loop in the beginning that just reads 1-bits (we're still on the Vec-stack), until it hits a 0-bit (we select a stack item), and then moves into the next loop that only considers nodes in the allocator.

Given the change I had to make for parsing in the case of the stack being empty I don't believe we should make this change

arvidn · 2025-01-13T16:58:57Z

src/traverse_path.rs

+) -> Response {
+    // the vec is a stack so a ChiaLisp list of (3 . (2 . (1 . NIL))) would be [1, 2, 3]
+    // however entries in this vec may be ChiaLisp SExps so it may look more like [1, (2 . NIL), 3]
+    let mut arg_list: Vec<NodePtr> = args.to_owned();


it would be more efficient to just keep an index into args, rather than cloning it

src/traverse_path.rs

arvidn

I think these things are still needed:

preserve the existing function, partly to control when we switch over to the new one, and also to be able to test that both behave the same
ensure the new function produce the same result as the old one, e.g. with a fuzzer.
ensure the new function behave the same with regards to limits to the number of pairs created by Allocator. It can be tested in a fuzzer by building with the counters build feature
benchmark to demonstrate that this is an improvement (this should probably be done early, as we might want to scrap this idea if it doesn't carry its weight)
survey the mainnet and testnet blockchains to see if back references into the parse-stack eveer exists in the wild
unit tests for all edge cases

arvidn · 2025-01-29T14:02:05Z

src/serde/de_br.rs

@@ -83,6 +129,86 @@ pub fn node_from_bytes_backrefs_record(
    Ok((ret, backrefs))
 }

+pub fn traverse_path_with_vec(


this is a pretty big function for not having a unit test.

Added a unit test

arvidn · 2025-01-29T14:03:12Z

.github/workflows/build-test.yml

@@ -212,7 +212,7 @@ jobs:
          cd tools
          cargo run --bin generate-fuzz-corpus
      - name: build
-        run: cargo fuzz list | xargs -I "%" sh -c "cargo fuzz run % -- -max_total_time=30 || exit 255"
+        run: cargo fuzz list | xargs -I "%" sh -c "cargo fuzz run % -- -max_total_time=30 || exit 255  --all-features"


If you make the fuzzer depend on clvmr/counters I don't think you would need this

fuzz/Cargo.toml

arvidn · 2025-01-29T14:28:31Z

fuzz/fuzz_targets/deserialize_br.rs

+        Ok(r) => r,
+    };
+
+    let b1 = node_to_bytes_backrefs(&allocator, program).unwrap();


I don't think you need this step. Also, you may loose interesting cases by "filtering" the tree when you re-encode it. It won't necessarily round-trip.
Is there a good reason to do this rather than using data directly?

arvidn · 2025-01-29T14:28:43Z

fuzz/fuzz_targets/deserialize_br.rs

+    let mut allocator = Allocator::new();
+    let mut allocator_old = Allocator::new();
+
+    let mut allocator = Allocator::new();


this line looks like a mistake

arvidn · 2025-01-29T14:34:44Z

fuzz/fuzz_targets/deserialize_br.rs

+    let mut allocator = Allocator::new();
+    let program = node_from_bytes_backrefs(&mut allocator, &b1).unwrap();
+    let program_old = node_from_bytes_backrefs_old(&mut allocator_old, &b1).unwrap();
+    assert!(allocator.pair_count() <= allocator_old.pair_count());


Another important test (that I believe would fail right now) is to deserialize bytes again, using both old and new functions and an allocator that has a pair-limit of allocator_old.pair_count() - 1. i.e. it should run out of memory, and so should the new function. Otherwise consensus rules will have changed, and we have a potential for a chain split.

I believe the simplest way to implement this is to have a function on Allocator to create a "fake" pair. I just needs to decrement its limit. It also means checkpoint() and restore_checkpoint() would need to track and update this limit.

I think it would be great to also ensure that the deseialization yields the same tree. You can use node_eq (see other fuzzers). This precludes using a single Allocator though, as I suggested earlier.

arvidn · 2025-01-29T14:37:49Z

src/serde/de_br.rs

@@ -22,7 +22,7 @@ pub fn node_from_stream_backrefs(
    f: &mut Cursor<&[u8]>,
    mut backref_callback: impl FnMut(NodePtr),
 ) -> io::Result<NodePtr> {
-    let mut values = allocator.nil();
+    let mut values = Vec::<NodePtr>::new();


I think this is required to avoid consensus problems. consider the worst case scenario where you have a tree that's then referenced thousands of times. With todays deserializer, they will point to the same tree, but in your version, you'll create the same pairs over and over again. In fact, I would expect the fuzzer to fail if you let it run long enough because of this.

arvidn · 2025-01-29T14:45:18Z

src/serde/de_br.rs

+        if parsing_sexp {
+            match allocator.sexp(sexp_to_parse) {
+                SExp::Atom => {
+                    return Err(EvalErr(sexp_to_parse, "path into atom".into()).into());


we could make this error message better now I think. We know it's a backref and that the serialization is invalid

Changed error message to: "invalid backreference during deserialisation"

src/serde/de_br.rs

arvidn · 2025-01-29T14:52:16Z

src/serde/de_br.rs

+    // if we move from parsing the Vec stack to parsing the SExp stack use the following variables
+    let mut sexp_to_parse = NodePtr::NIL;
+
+    while byte_idx > first_bit_byte_index || bitmask < last_bitmask {


did you explore making this two loops, one for the Vec stack and one for traversing the SExp tree?
I think it would be easier to follow. partly because you wouldn't need two loop bodies separated by the parsing_sexp check. It would also place the tail of this function in between those loops. If we exit after having pointed to a stack node

features=["counters"] rather than [features]

Co-authored-by: Arvid Norberg <[email protected]>

matt-o-how requested a review from arvidn January 13, 2025 16:18

arvidn reviewed Jan 13, 2025

View reviewed changes

arvidn reviewed Jan 15, 2025

View reviewed changes

matt-o-how force-pushed the node_from_stream_backrefs_optimisation branch from 166b35f to cb47c16 Compare January 17, 2025 10:09

matt-o-how force-pushed the node_from_stream_backrefs_optimisation branch from cb47c16 to 17f7c09 Compare January 27, 2025 16:53

arvidn reviewed Jan 29, 2025

View reviewed changes

matt-o-how added 23 commits January 30, 2025 12:23

initial commit

438e2df

basic structure

fdeef9f

comment for clarity

bdeea87

Improve clarity in error messages and comments

a9b3351

passing tests!

47e41b8

clippy cleanups

5d38c06

clarify comment

99b0c1d

remove Cost and move into de_br

ddceeba

use an index to simulate stack rather than cloning vec

8060a47

add catch for underflow

d394e0b

catch another underflow

f8f1e08

add fuzz against old

de2b748

fmt

3d35f09

add pair count fuzz

5231929

fmt fuzz

791030b

try adding features to fuzzer cargo.toml

675ab2c

forward the counters feature

982498f

fix name in fuzz cargo.toml

3b86c79

add all features to cargo fuzz github action

423dd73

return error when traversing empty values stack

64340f1

special case 0xfe 0x01

bd93d2a

special case 0xfe 0x00

49e26e2

treat empty stack as sexp

25cf07c

matt-o-how added 8 commits January 30, 2025 12:25

clippy cleanups

31e4fb4

clarify comment

b2946db

remove Cost and move into de_br

1ef2bc7

use

14d1de5

features=["counters"] rather than [features]

use checkpoint in fuzzer

c469103

pub(crate) msb_mask

d3986f9

add unit test for traverse_path_with_vec - not working

e7a8b5c

fix rebase weirdness

864ef16

matt-o-how force-pushed the node_from_stream_backrefs_optimisation branch from cf0be53 to 864ef16 Compare January 30, 2025 12:32

matt-o-how and others added 21 commits January 30, 2025 14:21

fix the unit test

d6d8cd4

fmt unit test

0a00f77

Update src/serde/de_br.rs

85a9785

Co-authored-by: Arvid Norberg <[email protected]>

add cache

e8f3f2a

clippy fix

1a30940

Update fuzz/Cargo.toml

1f790fe

Co-authored-by: Arvid Norberg <[email protected]>

remove useless line

e2fed6c

underscore unused vars

eb9f678

merge the fuzzers into one

62c4a82

check original version of fuzzer

c6a4071

cache when arg_index == 0

55cdb7f

test fuzzing without checkpoint

437f493

try 2 allocators

08dd203

try checkpointing differently

948712f

fix typo

11ad810

test if assertion was the real problem

90bb3de

clippy fix for check

bf09050

check old first

24abb5a

fmt for check

11fb7f6

disable checkpoint

a82e47f

add pair count fuzz

e1d0b32

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Node from stream backrefs optimisation #532

Node from stream backrefs optimisation #532

matt-o-how commented Jan 13, 2025

coveralls-official bot commented Jan 13, 2025 •

edited

Loading

arvidn left a comment

arvidn Jan 13, 2025

arvidn Jan 13, 2025

arvidn Jan 29, 2025

arvidn Jan 13, 2025

matt-o-how Jan 14, 2025

arvidn Jan 13, 2025

matt-o-how Jan 29, 2025

arvidn Jan 13, 2025

matt-o-how Jan 14, 2025

arvidn left a comment •

edited by matt-o-how

Loading

arvidn Jan 29, 2025

matt-o-how Jan 30, 2025

arvidn Jan 29, 2025

arvidn Jan 29, 2025

arvidn Jan 29, 2025

matt-o-how Jan 30, 2025

arvidn Jan 29, 2025

arvidn Jan 29, 2025

arvidn Jan 29, 2025

arvidn Jan 29, 2025

matt-o-how Jan 31, 2025

arvidn Jan 29, 2025

Node from stream backrefs optimisation #532

Are you sure you want to change the base?

Node from stream backrefs optimisation #532

Conversation

matt-o-how commented Jan 13, 2025

coveralls-official bot commented Jan 13, 2025 • edited Loading

Pull Request Test Coverage Report for Build 13076959208

Details

💛 - Coveralls

arvidn 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

Choose a reason for hiding this comment

arvidn left a comment • edited by matt-o-how 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

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

coveralls-official bot commented Jan 13, 2025 •

edited

Loading

arvidn left a comment •

edited by matt-o-how

Loading