Trie: Simplify findPath() #2962
Open
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
holgerd77
force-pushed
the
trie-find-path-simplification-new
branch
from
bd8bc04
to
606873c
Compare
Codecov Report
Additional details and impacted files
Flags with carried forward coverage won't be shown. Click here to find out more. |
Closed
ScottyPoi
reviewed
Aug 21, 2023
| @@ -66,7 +66,7 @@ export class WalkController { | |||
| * @param node - Node to get all children of and call onNode on. | |||
| * @param key - The current `key` which would yield the `node` when trying to get this node with a `get` operation. | |||
| */ | |||
| allChildren(node: TrieNode, key: Nibbles = []) { | |||
| allChildren(node: TrieNode, progress: Nibbles = []) { | |||
There was a problem hiding this comment.
:thumbsup
i like this name change. we should adopt it elsewhere in trie methods.
There was a problem hiding this comment.
Yes, I have the impression that especially in the trie library these kind of simple and local changes can add up to a significantly better readability if we apply consistently.
ScottyPoi
force-pushed
the
trie-find-path-simplification-new
branch
from
1df3061
to
55ecaeb
Compare
|
Side note: we likely want to run the reactivated trie benchmarks on this from #2969 with before and after results once we have this ready and the tests going. |
|
One of the This tested a valid proof with an invalid key, which should return null. I tweaked some things so all of the Trie tests would pass, but a few downstream tests fail now. Will comb through it this weekend to find out why |
|
Cool 🙏, seems pretty close already! 🙂 Can you do an in-between benchmarks run before and after and paste the results here? |
ScottyPoi
force-pushed
the
trie-find-path-simplification-new
branch
from
0243734
to
6b06a64
Compare
Benchmarks:master:trie-find-path-simplified-new |
|
|
Guess we need to put this on hold due to the performance results. Will give this a "blocked" label for now. |
…l chained Promise calls (WIP)
ScottyPoi
force-pushed
the
trie-find-path-simplification-new
branch
from
0ace4b6
to
ed6b063
Compare
|
I pared this down to as few lines of code as I could manage. the benchmark performance is the same, though. |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This is re-taken from the trie refactor from #2785 which turned out to be too extensive to get completed, there is a a lot of stuff in there which likely can be extracted and integrated via separate and independent PRs.
This specific PR takes on simplifying the over-complicated findPath() logic and call chaining, which makes it pretty hard to follow on what
findPath()is doing.Goal of this PR would be to get the existing tests back to working, then replace the
findPath()implementation with the adoptedfindPath2()entrypoint and remove the old functionality.I might take on this but can also be taken on by @ScottyPoi or someone else.
One deeper description on what this is doing: so this is bascially going down the call-chain of the existing
findPath()method and renames a lot of things, also takes in functionality from separate unnecessary methods over since the old call chain was unnecessarily re-instantiating stuff and the like on methods with just a one-line logic thing, all a bit insane. This made it close to impossible to follow the call-chain.So while a bit hard to see since there is no code diff, this PR does not refactor the
findPath()logic at all (and is not intended to do so). This is simply about simplification and streamlining and should remain in this scope.