-
Notifications
You must be signed in to change notification settings - Fork 123
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
[TEMP ON HOLD] tweak: Use a visitor pattern for more efficient SBOR traversals #1926
Draft
dhedey
wants to merge
3
commits into
develop
Choose a base branch
from
investigation/visitor-pattern-for-sbor-traversal
base: develop
Could not load branches
Branch not found: {{ refName }}
Loading
Could not load tags
Nothing to show
Loading
Are you sure you want to change the base?
Some commits from the old base branch may be removed from the timeline,
and old review comments may become outdated.
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
And make VecTraverser use a shim to the new visitor pattern to validate it in tests
Docker tags |
Benchmark for 3ebc88bClick to view benchmark
|
dhedey
changed the title
tweak: Use a visitor pattern for more efficient traversals
[ON TEMP HOLD] tweak: Use a visitor pattern for more efficient traversals
Sep 25, 2024
dhedey
changed the title
[ON TEMP HOLD] tweak: Use a visitor pattern for more efficient traversals
[ON TEMP HOLD] tweak: Use a visitor pattern for more efficient SBOR traversals
Sep 25, 2024
dhedey
changed the title
[ON TEMP HOLD] tweak: Use a visitor pattern for more efficient SBOR traversals
[TEMP ON HOLD] tweak: Use a visitor pattern for more efficient SBOR traversals
Sep 26, 2024
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.
Note
This PR attempts to convert the "Pulling" event style SBOR traverser "exterior iterator" to a "Visiting" "interior iterator" style abstraction, to validate potential performance gains. Side node: Technically the "Pulling" style abstraction is actually a "Generator / Iterator / Coroutine without arguments" in new Rust terminology [article, 2023 blog - so we should probably just make it an Iterator of an Event - the main issue is it's a borrowing iterator, because its items refer to the traverser itself. It can actually also probably be rewritten as a generator when the
gen
block is stabilized.This PR actually only goes half-way, to a "resumable visiting" pattern.
By attempting this, it was discovered we likely want to support both a pulling and a non-resumable visiting traverser. This would need to be done both at the untyped and typed layer.
That is because:
So quick wins:
VecTraverser => UntypedPullingTraverser
as its own thing - this lets us optimizeUntypedTraverser => UntypedVisitingTraverser
further to not be resumable, and allowing more declarative logic.TypedTraverser
and use it inPayloadValidator
Summary
Relates to #1860
Basically just a side-exploration on my off-day to trial a visitor pattern for SBOR traversals.
And makes VecTraverser use a shim to the new visitor pattern to validate it in tests.
I imagine it makes
VecTraverser
too slow in its current form, so we probably don't want to merge this.But I'm interested to see if the RawSborValue decoding is significantly faster (I'm hoping it can be, due to various optimizations which can be done for
calculate_value_tree_body_byte_length
now).When I have some more down time, I might investigate if we can do the same treatment to
TypedTraverser
, payload validation, etc.Perf review:
Winners:
And losers:
This broadly fits with expectations.
costing::validate_sbor_payload
is an anomoly. Otherwise, validation is ~10% slower. This would be fixed when it's resumable.Details
TBC
Testing
Existing tests pass... Let's take a look at the benchmark comparison for validation!