[FVM] Storage check optimisation #2837
Merged
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
…-go into janez/storage-check-optimisation
FVM Benchstat comparisonThis branch with compared with the base branch onflow:master commit 5bcb1f3 The command Bench tests were run a total of 10 times on each branch. Collapsed results for better readability
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
Codecov Report
@@ Coverage Diff @@
## rbtz/OTEL1 #2837 +/- ##
=============================================
Coverage ? 56.26%
=============================================
Files ? 690
Lines ? 63350
Branches ? 0
=============================================
Hits ? 35641
Misses ? 24725
Partials ? 2984
Flags with carried forward coverage won't be shown. Click here to find out more. Continue to review full report at Codecov.
|
janezpodhostnik
changed the base branch from
master
to
supun/cadence-error-refactor
|
looks like you're doing two independent things in the same PR (bumping cadence version & the actual optimization). could you split out the cadence change into a different pr? thx |
Tonix517
reviewed
Jul 21, 2022
fvm/contractFunctionInvocations.go
Outdated
| traceSpan opentracing.Span, | ||
| ) func(addresses []common.Address) (cadence.Value, error) { | ||
| return func(addresses []common.Address) (cadence.Value, error) { | ||
| arrayValues := make([]cadence.Value, len(addresses)) |
There was a problem hiding this comment.
can we check nil or empty of addresses here to save one invocation if no valid address passed in?
There was a problem hiding this comment.
I am not sure if I want to put this logic here. This is just a wrapper to call the smart contract function. Any conditions like that should be either in the smart contract or better yet (performance wise) in the function calling this one.
I will check that the function calling this one doesn't do it with an empty array.
What do you think?
pattyshack
reviewed
Jul 21, 2022
fvm/transactionStorageLimiter.go
Outdated
|
|
||
| usage, err := env.GetStorageUsed(commonAddress) | ||
| usage, err := env.GetStorageUsed(commonAddresses[i]) |
There was a problem hiding this comment.
there's no point in invoking the account storage capacity contract if this fails. maybe collect the usages first in a separate loop (and early exit on error), then invoke the contract and check capacity in this loop.
|
Does existing Benchstat numbers include expected metrics we want? If not, could you publish the metrics before\after this optimization? thanks. |
|
@Tonix517 Benchstat numbers do already include the performance comparison here. I added a section to the PR description |
pattyshack
approved these changes
Jul 21, 2022
janezpodhostnik
requested review from
yhassanzadeh13 and
AlexHentschel
as code owners
janezpodhostnik
removed request for
simonhf,
Kay-Zee,
DEPRICATED-Dapperlabs-ex-employee,
yhassanzadeh13 and
AlexHentschel
bors bot
added a commit
that referenced
this pull request
Jul 25, 2022
2853: [FVM] Update cadence to support Executor pattern r=SaveTheRbtz a=SaveTheRbtz Factored out cadence update and test fixes out of #2837 so that we can break circular dependency on #2823 Co-authored-by: Janez Podhostnik <janez.podhostnik@gmail.com> 2856: migrate environment to new span api r=pattyshack a=pattyshack Co-authored-by: Alexey Ivanov <rbtz@dapperlabs.com> Co-authored-by: Patrick Lee <patrick.lee@dapperlabs.com>
bors bot
added a commit
that referenced
this pull request
Jul 25, 2022
2853: [FVM] Update cadence to support Executor pattern r=SaveTheRbtz a=SaveTheRbtz Factored out cadence update and test fixes out of #2837 so that we can break circular dependency on #2823 Co-authored-by: Janez Podhostnik <janez.podhostnik@gmail.com> Co-authored-by: Alexey Ivanov <rbtz@dapperlabs.com>
bors bot
added a commit
that referenced
this pull request
Jul 26, 2022
2853: [FVM] Update cadence to support Executor pattern r=SaveTheRbtz a=SaveTheRbtz Factored out cadence update and test fixes out of #2837 so that we can break circular dependency on #2823 Co-authored-by: Janez Podhostnik <janez.podhostnik@gmail.com> 2858: [FVM] cleanup and regenerate mocks r=SaveTheRbtz a=SaveTheRbtz Co-authored-by: Alexey Ivanov <rbtz@dapperlabs.com>
bors bot
added a commit
that referenced
this pull request
Jul 26, 2022
2853: [FVM] Update cadence to support Executor pattern r=SaveTheRbtz a=SaveTheRbtz Factored out cadence update and test fixes out of #2837 so that we can break circular dependency on #2823 Co-authored-by: Janez Podhostnik <janez.podhostnik@gmail.com> Co-authored-by: Alexey Ivanov <rbtz@dapperlabs.com>
SaveTheRbtz
force-pushed
the
rbtz/OTEL1
branch
2 times, most recently
from
5e27e69 to
93370c8
Compare
SaveTheRbtz
reviewed
Jul 26, 2022
|
#2853 has landed. You can rebase on top of master now. |
|
maybe naive question, but why don't you filter this list with storage_used ? You should be able to skip a lot of checks where storage_used < N ( N = minimimAccountBalance granted storage ~ 100Kb) |
|
@bluesign the minimum account balance is defined in the state, so getting it requires a call to GetValue. A GetValue call is expensive, but not as expensive as a Contract function invocation. With the previous approach fetching minimum account balance once would definitely make sense, but with this approach (only one Contract function invocation) I'm not sure it does. In some cases (the failure ones) this would mean that the check is faster, but the happy path would be a bit longer because of it. |
|
bors merge |
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.
Make one storage check for all account at once instead of one per account, reducing the number of ContractFunctionInvocations per transaction.
ref: https://github.com/dapperlabs/flow-go/issues/6308
Depends on cadence runtime.Interface changes from onflow/cadence#1811
Results
The improvement can be seen below in the FVM Benchstat comparison.
All of the benchnet scenarios below check storage of 3 or 4 accounts which is also the most common case on Mainnet.
The touched accounts are:
The performance improvement on shorter transactions like
RuntimeTransaction/reference_tx-2is about 15%. Tapering off on long running transactions, where a lot of other things also happens during the transaction. OnRuntimeTransaction/transfer_tokens-2(which does 100 token transfers in 1 transaction between the same 2 users) the benefit is only ~2.5%.There is one exception, which is
RuntimeTransaction/create_new_accountthis runs about 8% faster even though it is a very long running transaction. The reason for this is that it creates 50 accounts, that all need to be checked for storage in addition to the 3 default ones.