Skip to content
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

[FVM] refactor computation and memory metering #2129

Merged
merged 20 commits into from Mar 15, 2022

Conversation

ramtinms
Copy link
Member

@ramtinms ramtinms commented Mar 10, 2022

This pr

  • bumps cadence version to support computation metering interface changes
  • moves computation metering logic into the state to enable recording metrics when programs are getting cached and replying when returned from cache (prevents execution forks)
  • adds basic metering that behaves similar to the cadence runtime limits before moving responsibility to FVM (+tests)
  • enables building more complex computation and memory metering logic that depends on the execution state (e.g. reading values from service account)

fvm/meter/basic/meter.go Outdated Show resolved Hide resolved
Copy link
Contributor

@janezpodhostnik janezpodhostnik left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good.

How come the base for this branch is cadence-issue-1127?

fvm/transactionEnv.go Outdated Show resolved Hide resolved
@ramtinms ramtinms changed the base branch from cadence-issue-1127 to master March 10, 2022 16:58
@github-actions
Copy link
Contributor

github-actions bot commented Mar 11, 2022

FVM Benchstat comparison

This branch with compared with the base branch onflow:master commit 6c08a14

The command (for i in {1..N}; do go test ./fvm --bench . --tags relic -shuffle=on; done) was used.

Bench tests were run a total of 7 times on each branch.

Results

old.txtnew.txt
time/opdelta
RuntimeTransaction/transfer_tokens-2207ms ± 5%221ms ± 5%+6.44%(p=0.007 n=7+7)
RuntimeTransaction/get_signer_vault-244.0ms ± 2%46.5ms ± 4%+5.69%(p=0.001 n=7+7)
RuntimeTransaction/convert_int_to_string-236.4ms ± 3%38.3ms ± 6%+5.26%(p=0.004 n=7+7)
RuntimeTransaction/get_account_and_get_available_balance-2542ms ± 1%563ms ± 3%+3.92%(p=0.001 n=6+7)
RuntimeTransaction/get_public_account-239.5ms ± 3%41.0ms ± 4%+3.63%(p=0.017 n=7+7)
RuntimeTransaction/get_signer_address-236.0ms ± 3%37.1ms ± 4%+3.10%(p=0.038 n=7+7)
RuntimeTransaction/get_account_and_get_storage_capacity-2500ms ± 3%515ms ± 3%+2.98%(p=0.026 n=7+7)
RuntimeNFTBatchTransfer-2137ms ± 6%139ms ± 4%~(p=0.259 n=7+7)
RuntimeTransaction/reference_tx-235.3ms ± 7%36.0ms ± 5%~(p=0.209 n=7+7)
RuntimeTransaction/convert_int_to_string_and_concatenate_it-238.7ms ± 8%38.4ms ± 1%~(p=0.945 n=7+6)
RuntimeTransaction/get_account_and_get_balance-2648ms ± 4%663ms ± 2%~(p=0.053 n=7+7)
RuntimeTransaction/get_account_and_get_storage_used-252.6ms ± 4%53.6ms ± 1%~(p=0.073 n=7+5)
RuntimeTransaction/get_signer_receiver-254.9ms ± 4%56.2ms ± 4%~(p=0.209 n=7+7)
RuntimeTransaction/load_and_save_empty_string_on_signers_address-243.9ms ± 1%43.5ms ± 2%~(p=0.394 n=6+6)
RuntimeTransaction/load_and_save_long_string_on_signers_address-286.0ms ± 4%86.4ms ± 1%~(p=0.755 n=7+5)
RuntimeTransaction/create_new_account-21.28s ± 2%1.32s ± 4%~(p=0.053 n=7+7)
RuntimeTransaction/call_empty_contract_function-240.6ms ± 3%41.2ms ± 3%~(p=0.209 n=7+7)
RuntimeTransaction/emit_event-254.0ms ± 2%55.8ms ± 4%~(p=0.073 n=7+7)
 

@ramtinms ramtinms requested a review from m4ksio March 11, 2022 21:14
Copy link
Member

@turbolent turbolent left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you for also already starting on integration / preparing for the memory metering 👍

fvm/context.go Outdated Show resolved Hide resolved
fvm/context.go Outdated Show resolved Hide resolved
fvm/context.go Show resolved Hide resolved
fvm/fvm_test.go Show resolved Hide resolved
@codecov-commenter
Copy link

codecov-commenter commented Mar 11, 2022

Codecov Report

Merging #2129 (2d159e0) into master (41f34ec) will decrease coverage by 0.04%.
The diff coverage is 65.38%.

@@            Coverage Diff             @@
##           master    #2129      +/-   ##
==========================================
- Coverage   57.52%   57.47%   -0.05%     
==========================================
  Files         637      637              
  Lines       37211    37261      +50     
==========================================
+ Hits        21405    21416      +11     
- Misses      13126    13163      +37     
- Partials     2680     2682       +2     
Flag Coverage Δ
unittests 57.47% <65.38%> (-0.05%) ⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files Coverage Δ
fvm/errors/codes.go 100.00% <ø> (ø)
fvm/errors/execution.go 2.94% <0.00%> (-1.23%) ⬇️
fvm/context.go 69.00% <22.22%> (-4.12%) ⬇️
fvm/state/state_holder.go 38.09% <25.00%> (-1.91%) ⬇️
fvm/state/state.go 74.63% <42.85%> (-3.59%) ⬇️
fvm/transaction.go 62.85% <50.00%> (-6.71%) ⬇️
fvm/script.go 87.23% <55.55%> (-7.64%) ⬇️
fvm/scriptEnv.go 40.00% <92.30%> (-0.32%) ⬇️
fvm/meter/basic/meter.go 93.33% <93.33%> (ø)
fvm/bootstrap.go 84.63% <100.00%> (+0.07%) ⬆️
... and 10 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 41f34ec...2d159e0. Read the comment docs.

@SupunS SupunS self-requested a review March 14, 2022 17:48
fvm/meter/basic/meter.go Outdated Show resolved Hide resolved
fvm/meter/basic/meter.go Outdated Show resolved Hide resolved
fvm/state/state.go Outdated Show resolved Hide resolved
@ramtinms ramtinms merged commit e52604d into master Mar 15, 2022
@bors bors bot deleted the ramtin/fvm-refactor-metring branch March 15, 2022 21:21
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants