Node from stream backrefs optimisation #532
Conversation
Pull Request Test Coverage Report for Build 12826960470Details
💛 - Coveralls |
There was a problem hiding this comment.
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)
src/traverse_path.rs
Outdated
| @@ -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( | |||
There was a problem hiding this comment.
it would be good to have unit tests for this function
| @@ -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(); | |||
There was a problem hiding this comment.
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.
src/traverse_path.rs
Outdated
| // find first non-zero byte | ||
| let first_bit_byte_index = first_non_zero(node_index); | ||
|
|
||
| let mut cost: Cost = TRAVERSE_BASE_COST |
There was a problem hiding this comment.
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.
src/traverse_path.rs
Outdated
| 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; |
There was a problem hiding this comment.
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.
src/traverse_path.rs
Outdated
| ) -> 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(); |
There was a problem hiding this comment.
it would be more efficient to just keep an index into args, rather than cloning it
There was a problem hiding this comment.
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 thecountersbuild 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
matt-o-how
force-pushed
the
node_from_stream_backrefs_optimisation
branch
from
166b35f to
cb47c16
Compare
Use a
Vec<NodePtr>stack instead ofNodePtr/SExps innode_from_stream_backrefsand add a newtraverse_path_with_vec()function to handle backrefs