-
Notifications
You must be signed in to change notification settings - Fork 256
Benchmarks for balance transfer check extension #3557
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
Merged
Merged
Changes from 1 commit
Commits
Show all changes
11 commits
Select commit
Hold shift + click to select a range
9238983
move domains enabled check to domains extension
vedhavyas 3b0ea9d
move balance transfer check to primitives crate for reusability
vedhavyas a56a06e
define benchmarks for balance transfer extension
vedhavyas 4ae0708
add benchmarks and generate weights
vedhavyas 734a273
integrate weights to extension
vedhavyas 7a789ef
enable domins for test consensus runtime
vedhavyas fe4075a
Merge branch 'main' into benchmarks_disable_extension
vedhavyas 4f4900a
clarify and update benchmark weight usage
vedhavyas 0d37ad2
Benchmark a list of non-transfer calls instead of mixed nested calls
teor2345 7c80471
Try 5,000 benchmark calls
teor2345 2c43c56
Merge branch 'main' into benchmarks_disable_extension
vedhavyas File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains hidden or 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
Oops, something went wrong.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What happens if the actual number of nested calls is greater than this? Does the submitter get the extra calls for free?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes if it exceeds 1000, the remaining are free but max decoding depth for extrinsics is 256. So we will never reach that
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There are at least 3 ways to get more than 1000 loop iterations without exceeding the decoding depth:
utility([non_transfer_call; 2000]): 2000 calls in one utility callutility([utility([non_transfer_call; 100]); 100]): 100 utility calls each containing 100 callsIt is likely that decoding will have different performance for case 3, due to branch misprediction (there are no long runs of the same item).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The
MAXIMUM_NUMBER_OF_CALLSis inevitable because the weight system works as:So we have to first define the worst situation weight with
MAXIMUM_NUMBER_OF_CALLS, the value ofMAXIMUM_NUMBER_OF_CALLSis hard to determine since we don't really have an explicit limit on the maximum nested calls, options I can think of:MAXIMUM_NUMBER_OF_CALLSbyblock_size / min_extrinsic_sizeMAXIMUM_NUMBER_OF_CALLSas is or pick another value, since the check is lightweight, the resulting weight won't be much different I guess.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I have tried again with max of 1000 nested utility calls with 500 system.remark calls until the last nested call. Last one included on balance transfer.
Allocator failed to allocate the memory. If such a case arise in practical, that extrinsic will never be inluded
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not sure if that's the data structure I was talking about above.
Either way, there's going to be some number of calls that fits within the allocation limits. I'll try a few things and add a benchmark if it's heavier.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A list of 1000 non-transfer calls fits in memory and is heavier in some cases:
https://github.com/autonomys/subspace/tree/benchmarks_disable_extension_exploration
I haven't had time to explore the other data structures like squares and trees. Since both "long" and "tall" calls have similar results, I'm not sure it's going to be much different. Trees might be different if there's a lot of pointer dereferences, but I don't think it's a blocker for this PR.