fix: remove one way branching in bytecode trie #775
Conversation
|
agostbiro
had a problem deploying
to
github-action-benchmark
GitHub Actions
Error
agostbiro
force-pushed
the
fix/bytecode-trie
branch
from
d64399d to
84ade23
Compare
agostbiro
temporarily deployed
to
github-action-benchmark
GitHub Actions
Inactive
agostbiro
force-pushed
the
fix/bytecode-trie
branch
from
84ade23 to
76840d8
Compare
agostbiro
had a problem deploying
to
github-action-benchmark
GitHub Actions
Failure
|
Do we want a changelog item for this? It's not a user reported issue. |
| return Some(TrieSearch::LongestPrefixNode(cursor)); | ||
| // Otherwise the cursor's range is greater than the key's length which means the | ||
| // key is a prefix of the match. | ||
| None |
There was a problem hiding this comment.
Is this the case where the key is "foo" and a stored entry is "foobar"?
Wouldn't we still be interested in that case? Or is there a separate function for that?
There was a problem hiding this comment.
Is this the case where the key is "foo" and a stored entry is "foobar"?
Yes, we'd return None in this case.
Wouldn't we still be interested in that case? Or is there a separate function for that?
Yes, normally this is an interesting result for trie lookups, but for the purposes of the contract identifier, it's a no match, so we're following the previous implementation here by returning None.
| let mut descendants = Vec::with_capacity(node_to_split.descendants.len() + 1); | ||
| descendants.extend(node_to_split.descendants.iter().cloned()); | ||
| descendants.push(new_item.clone()); |
There was a problem hiding this comment.
I don't think this variable is used in this function
There was a problem hiding this comment.
Uhh that's something I broke when fixing the last bug. Thanks for catching it! Fixed + tested in
bc378e0
| } | ||
|
|
||
| pub fn criterion_benchmark(c: &mut Criterion) { | ||
| let Some(build_info_config) = load_build_info_config().expect("loads build info config") else { |
There was a problem hiding this comment.
You might be able to avoid the overhead of loading using this API of criterion:
https://bheisler.github.io/criterion.rs/book/user_guide/benchmarking_with_inputs.html
There was a problem hiding this comment.
Thanks, could you explain this a bit more? I don't follow.
agostbiro
had a problem deploying
to
github-action-benchmark
GitHub Actions
Failure
|
The |
agostbiro
had a problem deploying
to
github-action-benchmark
GitHub Actions
Failure
This PR fixes stack overflow on drop in the
BytecodeTrieby eliminating one-way branching which produces very deep tries for large contracts. Care has been taken to preserve the implementation details of the previous implementation which the stack trace heuristics rely on.I moved the
BytecodeTrieto a separate module in 7bcb871. The changes to eliminate one-way branching can be reviewed by reviewing 76840d8.I also added a benchmark for constructing the
ContractDecoder, but maybe it'd be better to put this in thetoolscrate, as running the benchmark requires some manual setup.Eliminating one way branching resulted in a 5x speed up for constructing the
ContractDecoderfor theforge-stdtest suite.We are benchmarking the
ContractDecoderconstruction instead of theBytecodeTrieconstruction as the former takes care of constructing the data that goes in the trie and I didn't want to spend time on recreating that logic separately. This does mean however that the benchmark also measures parsing and normalizing the contract metadata, I suspect which now takes the majority of the time.