flow-go icon indicating copy to clipboard operation
flow-go copied to clipboard

Published 20 hours ago • verified publisher icon onflow

Standardize nested transaction creation/restart/commitment

Open pattyshack opened this issue 2 years ago • 4 comments

  1. Refactor program handler's state stack into StateHolder.
  2. Switch transaction invoker and seq num verifier to the new api

In a future PR, I'll restrict access to Environment methods (if cadence accesses anything besides GetCode, we need to know about it, and invalidate the programs cache accordingly).

pattyshack avatar Aug 11 '22 21:08 pattyshack

FVM Benchstat comparison

This branch with compared with the base branch onflow:master commit 519f8eb111b1384c7ff0849612327847ce02b484

The command (for i in {1..10}; do go test ./fvm ./engine/execution/computation --bench . --tags relic -shuffle=on --benchmem --run ^$; done) was used.

Collapsed results for better readability

old.txtnew.txt
time/opdelta
pkg:github.com/onflow/flow-go/engine/execution/computation goos:linux goarch:amd64
ComputeBlock/16/cols/32/txes-21.63s ± 2%1.64s ± 1%+0.81%(p=0.019 n=9+9)
ComputeBlock/16/cols/128/txes-26.27s ± 1%6.32s ± 2%+0.80%(p=0.011 n=9+9)
pkg:github.com/onflow/flow-go/fvm goos:linux goarch:amd64
RuntimeTransaction/reference_tx-233.3ms ± 7%32.7ms ± 8%~(p=0.356 n=10+9)
RuntimeTransaction/convert_int_to_string-235.7ms ±10%37.3ms ±12%~(p=0.190 n=10+10)
RuntimeTransaction/convert_int_to_string_and_concatenate_it-237.8ms ±10%38.5ms ±10%~(p=0.684 n=10+10)
RuntimeTransaction/get_signer_address-235.2ms ± 5%34.3ms ±11%~(p=0.315 n=10+10)
RuntimeTransaction/get_public_account-236.2ms ± 9%37.1ms ± 8%~(p=0.353 n=10+10)
RuntimeTransaction/get_account_and_get_balance-2554ms ± 3%554ms ± 3%~(p=0.968 n=9+10)
RuntimeTransaction/get_account_and_get_storage_used-239.9ms ± 7%40.8ms ± 7%~(p=0.353 n=10+10)
RuntimeTransaction/get_account_and_get_storage_capacity-2422ms ± 3%421ms ± 3%~(p=0.579 n=10+10)
RuntimeTransaction/get_signer_vault-241.8ms ± 3%41.4ms ± 6%~(p=0.356 n=9+10)
RuntimeTransaction/get_signer_receiver-253.9ms ± 2%54.0ms ± 2%~(p=0.546 n=9+9)
RuntimeTransaction/transfer_tokens-2222ms ± 1%222ms ± 3%~(p=0.573 n=8+10)
RuntimeTransaction/load_and_save_empty_string_on_signers_address-242.0ms ± 5%42.8ms ± 5%~(p=0.243 n=10+9)
RuntimeTransaction/load_and_save_long_string_on_signers_address-285.3ms ± 2%85.7ms ± 4%~(p=0.684 n=10+10)
RuntimeTransaction/create_new_account-21.00s ± 2%1.00s ± 1%~(p=0.549 n=10+9)
RuntimeTransaction/call_empty_contract_function-238.2ms ± 3%38.3ms ± 5%~(p=0.968 n=9+10)
RuntimeTransaction/emit_event-253.9ms ± 3%53.9ms ± 4%~(p=0.971 n=10+10)
RuntimeTransaction/borrow_array_from_storage-2140ms ± 5%143ms ± 5%~(p=0.089 n=10+10)
RuntimeTransaction/copy_array_from_storage-2145ms ± 2%147ms ± 4%~(p=0.218 n=10+10)
RuntimeNFTBatchTransfer-2124ms ± 2%122ms ± 5%~(p=0.122 n=8+10)
pkg:github.com/onflow/flow-go/engine/execution/computation goos:linux goarch:amd64
ComputeBlock/1/cols/16/txes-259.0ms ± 6%61.0ms ±10%~(p=0.079 n=10+9)
ComputeBlock/1/cols/32/txes-2114ms ± 4%115ms ± 1%~(p=0.094 n=9+9)
ComputeBlock/1/cols/64/txes-2219ms ± 1%220ms ± 2%~(p=0.661 n=9+10)
ComputeBlock/1/cols/128/txes-2421ms ± 1%423ms ± 2%~(p=0.370 n=9+8)
ComputeBlock/4/cols/16/txes-2227ms ± 2%228ms ± 2%~(p=0.481 n=9+8)
ComputeBlock/4/cols/32/txes-2429ms ± 2%428ms ± 1%~(p=0.661 n=9+10)
ComputeBlock/4/cols/64/txes-2824ms ± 1%822ms ± 1%~(p=0.481 n=10+10)
ComputeBlock/4/cols/128/txes-21.61s ± 1%1.61s ± 2%~(p=0.684 n=10+10)
ComputeBlock/16/cols/16/txes-2847ms ± 0%849ms ± 2%~(p=0.541 n=8+9)
ComputeBlock/16/cols/64/txes-23.19s ± 1%3.20s ± 2%~(p=0.393 n=10+10)
pkg:github.com/onflow/flow-go/fvm goos:linux goarch:amd64
RuntimeTransaction/get_account_and_get_available_balance-2475ms ±10%454ms ± 2%−4.41%(p=0.021 n=10+8)
 
computationdelta
pkg:github.com/onflow/flow-go/fvm goos:linux goarch:amd64
RuntimeTransaction/reference_tx-2202 ± 0%202 ± 0%~(all equal)
RuntimeTransaction/convert_int_to_string-2402 ± 0%402 ± 0%~(all equal)
RuntimeTransaction/convert_int_to_string_and_concatenate_it-2502 ± 0%502 ± 0%~(all equal)
RuntimeTransaction/get_signer_address-2302 ± 0%302 ± 0%~(all equal)
RuntimeTransaction/get_public_account-2402 ± 0%402 ± 0%~(all equal)
RuntimeTransaction/get_account_and_get_balance-21.00k ± 0%1.00k ± 0%~(all equal)
RuntimeTransaction/get_account_and_get_available_balance-22.60k ± 0%2.60k ± 0%~(all equal)
RuntimeTransaction/get_account_and_get_storage_used-2402 ± 0%402 ± 0%~(all equal)
RuntimeTransaction/get_account_and_get_storage_capacity-21.30k ± 0%1.30k ± 0%~(all equal)
RuntimeTransaction/get_signer_vault-2402 ± 0%402 ± 0%~(all equal)
RuntimeTransaction/get_signer_receiver-2602 ± 0%602 ± 0%~(all equal)
RuntimeTransaction/transfer_tokens-23.50k ± 0%3.50k ± 0%~(all equal)
RuntimeTransaction/load_and_save_empty_string_on_signers_address-2602 ± 0%602 ± 0%~(all equal)
RuntimeTransaction/load_and_save_long_string_on_signers_address-2602 ± 0%602 ± 0%~(all equal)
RuntimeTransaction/create_new_account-2202 ± 0%202 ± 0%~(all equal)
RuntimeTransaction/call_empty_contract_function-2402 ± 0%402 ± 0%~(all equal)
RuntimeTransaction/emit_event-2602 ± 0%602 ± 0%~(all equal)
RuntimeTransaction/borrow_array_from_storage-22.60k ± 0%2.60k ± 0%~(all equal)
RuntimeTransaction/copy_array_from_storage-22.60k ± 0%2.60k ± 0%~(all equal)
 
interactionsdelta
pkg:github.com/onflow/flow-go/fvm goos:linux goarch:amd64
RuntimeTransaction/reference_tx-244.4k ± 0%44.4k ± 0%~(all equal)
RuntimeTransaction/convert_int_to_string-244.4k ± 0%44.4k ± 0%~(all equal)
RuntimeTransaction/convert_int_to_string_and_concatenate_it-244.4k ± 0%44.4k ± 0%~(all equal)
RuntimeTransaction/get_signer_address-244.4k ± 0%44.4k ± 0%~(all equal)
RuntimeTransaction/get_public_account-244.4k ± 0%44.4k ± 0%~(all equal)
RuntimeTransaction/get_account_and_get_balance-216.8M ± 0%16.8M ± 0%~(all equal)
RuntimeTransaction/get_account_and_get_available_balance-25.28M ± 0%5.28M ± 0%~(all equal)
RuntimeTransaction/get_account_and_get_storage_used-248.0k ± 0%48.0k ± 0%~(all equal)
RuntimeTransaction/get_account_and_get_storage_capacity-25.27M ± 0%5.27M ± 0%~(all equal)
RuntimeTransaction/get_signer_vault-244.7k ± 0%44.7k ± 0%~(all equal)
RuntimeTransaction/get_signer_receiver-245.0k ± 0%45.0k ± 0%~(all equal)
RuntimeTransaction/transfer_tokens-245.8k ± 0%45.8k ± 0%~(all equal)
RuntimeTransaction/load_and_save_empty_string_on_signers_address-244.8k ± 0%44.8k ± 0%~(all equal)
RuntimeTransaction/load_and_save_long_string_on_signers_address-249.7k ± 0%49.7k ± 0%~(all equal)
RuntimeTransaction/create_new_account-28.53M ± 0%8.53M ± 0%~(all equal)
RuntimeTransaction/call_empty_contract_function-244.6k ± 0%44.6k ± 0%~(all equal)
RuntimeTransaction/emit_event-244.6k ± 0%44.6k ± 0%~(all equal)
RuntimeTransaction/borrow_array_from_storage-249.8k ± 0%49.8k ± 0%~(all equal)
RuntimeTransaction/copy_array_from_storage-249.8k ± 0%49.8k ± 0%~(all equal)
 
alloc/opdelta
pkg:github.com/onflow/flow-go/engine/execution/computation goos:linux goarch:amd64
ComputeBlock/16/cols/16/txes-2390MB ± 2%396MB ± 4%+1.55%(p=0.029 n=10+10)
pkg:github.com/onflow/flow-go/fvm goos:linux goarch:amd64
RuntimeTransaction/reference_tx-240.8MB ± 9%40.2MB ± 8%~(p=0.315 n=10+10)
RuntimeTransaction/convert_int_to_string-241.4MB ± 5%41.5MB ± 5%~(p=0.912 n=10+10)
RuntimeTransaction/convert_int_to_string_and_concatenate_it-240.9MB ± 9%40.4MB ± 6%~(p=0.353 n=10+10)
RuntimeTransaction/get_signer_address-241.6MB ± 6%40.8MB ±11%~(p=0.739 n=10+10)
RuntimeTransaction/get_public_account-240.8MB ±10%41.5MB ± 6%~(p=0.481 n=10+10)
RuntimeTransaction/get_account_and_get_balance-2236MB ± 1%236MB ± 1%~(p=0.796 n=10+10)
RuntimeTransaction/get_account_and_get_available_balance-2187MB ± 2%186MB ± 3%~(p=0.218 n=10+10)
RuntimeTransaction/get_account_and_get_storage_used-240.8MB ± 6%41.5MB ± 6%~(p=0.481 n=10+10)
RuntimeTransaction/get_account_and_get_storage_capacity-2180MB ± 3%182MB ± 2%~(p=0.247 n=10+10)
RuntimeTransaction/get_signer_vault-241.0MB ± 4%40.7MB ± 6%~(p=0.315 n=8+10)
RuntimeTransaction/get_signer_receiver-246.4MB ± 4%45.5MB ± 3%~(p=0.113 n=10+9)
RuntimeTransaction/transfer_tokens-293.0MB ± 2%92.3MB ± 3%~(p=0.280 n=10+10)
RuntimeTransaction/load_and_save_empty_string_on_signers_address-241.9MB ± 7%42.1MB ±10%~(p=0.739 n=10+10)
RuntimeTransaction/load_and_save_long_string_on_signers_address-261.9MB ± 3%61.8MB ± 5%~(p=0.971 n=10+10)
RuntimeTransaction/create_new_account-2262MB ± 2%262MB ± 2%~(p=0.529 n=10+10)
RuntimeTransaction/call_empty_contract_function-242.6MB ± 3%41.9MB ± 6%~(p=0.400 n=9+10)
RuntimeTransaction/emit_event-248.4MB ± 3%47.6MB ± 5%~(p=0.278 n=9+10)
RuntimeTransaction/borrow_array_from_storage-275.3MB ± 6%75.7MB ± 3%~(p=0.912 n=10+10)
RuntimeTransaction/copy_array_from_storage-287.0MB ± 2%87.6MB ± 3%~(p=0.436 n=10+10)
RuntimeNFTBatchTransfer-260.2MB ± 4%58.7MB ± 5%~(p=0.089 n=10+10)
pkg:github.com/onflow/flow-go/engine/execution/computation goos:linux goarch:amd64
ComputeBlock/1/cols/16/txes-244.2MB ± 3%44.4MB ± 6%~(p=0.460 n=8+10)
ComputeBlock/1/cols/32/txes-265.2MB ± 5%65.6MB ± 4%~(p=0.481 n=10+10)
ComputeBlock/1/cols/64/txes-291.8MB ± 6%92.0MB ± 4%~(p=0.912 n=10+10)
ComputeBlock/1/cols/128/txes-2146MB ± 2%146MB ± 4%~(p=0.796 n=10+10)
ComputeBlock/4/cols/16/txes-2124MB ± 0%124MB ± 1%~(p=1.000 n=6+9)
ComputeBlock/4/cols/32/txes-2177MB ± 2%177MB ± 0%~(p=0.604 n=10+9)
ComputeBlock/4/cols/128/txes-2490MB ± 0%490MB ± 0%~(p=0.481 n=9+8)
ComputeBlock/16/cols/32/txes-2603MB ± 2%600MB ± 0%~(p=0.161 n=10+7)
ComputeBlock/16/cols/64/txes-21.03GB ± 1%1.02GB ± 1%~(p=0.481 n=10+10)
ComputeBlock/16/cols/128/txes-21.86GB ± 1%1.86GB ± 1%~(p=0.796 n=10+10)
ComputeBlock/4/cols/64/txes-2284MB ± 2%281MB ± 0%−1.12%(p=0.009 n=10+8)
 
allocs/opdelta
pkg:github.com/onflow/flow-go/engine/execution/computation goos:linux goarch:amd64
ComputeBlock/4/cols/16/txes-2926k ± 0%927k ± 0%+0.04%(p=0.029 n=10+10)
ComputeBlock/1/cols/128/txes-21.84M ± 0%1.84M ± 0%+0.04%(p=0.001 n=10+10)
ComputeBlock/1/cols/64/txes-2925k ± 0%926k ± 0%+0.04%(p=0.001 n=10+10)
pkg:github.com/onflow/flow-go/fvm goos:linux goarch:amd64
RuntimeTransaction/reference_tx-2125k ± 0%125k ± 0%+0.04%(p=0.000 n=10+10)
RuntimeTransaction/get_signer_address-2130k ± 0%130k ± 0%+0.03%(p=0.000 n=10+10)
RuntimeTransaction/convert_int_to_string-2139k ± 0%139k ± 0%+0.03%(p=0.000 n=10+10)
RuntimeTransaction/call_empty_contract_function-2142k ± 0%142k ± 0%+0.03%(p=0.000 n=10+10)
pkg:github.com/onflow/flow-go/engine/execution/computation goos:linux goarch:amd64
ComputeBlock/4/cols/128/txes-27.34M ± 0%7.35M ± 0%+0.03%(p=0.001 n=9+8)
ComputeBlock/1/cols/16/txes-2238k ± 0%238k ± 0%+0.03%(p=0.000 n=10+10)
pkg:github.com/onflow/flow-go/fvm goos:linux goarch:amd64
RuntimeTransaction/get_public_account-2161k ± 0%161k ± 0%+0.03%(p=0.000 n=10+10)
RuntimeTransaction/convert_int_to_string_and_concatenate_it-2153k ± 0%153k ± 0%+0.03%(p=0.000 n=9+10)
RuntimeTransaction/load_and_save_empty_string_on_signers_address-2175k ± 0%175k ± 0%+0.03%(p=0.000 n=10+9)
RuntimeTransaction/get_account_and_get_storage_used-2183k ± 0%183k ± 0%+0.03%(p=0.000 n=10+10)
pkg:github.com/onflow/flow-go/engine/execution/computation goos:linux goarch:amd64
ComputeBlock/16/cols/64/txes-214.7M ± 0%14.7M ± 0%+0.02%(p=0.001 n=9+8)
ComputeBlock/1/cols/32/txes-2467k ± 0%467k ± 0%+0.02%(p=0.006 n=10+10)
pkg:github.com/onflow/flow-go/fvm goos:linux goarch:amd64
RuntimeTransaction/get_signer_vault-2177k ± 0%177k ± 0%+0.02%(p=0.000 n=9+10)
RuntimeTransaction/emit_event-2186k ± 0%186k ± 0%+0.02%(p=0.001 n=10+10)
RuntimeTransaction/copy_array_from_storage-2370k ± 0%370k ± 0%+0.02%(p=0.000 n=10+9)
RuntimeTransaction/get_signer_receiver-2266k ± 0%266k ± 0%+0.01%(p=0.000 n=9+10)
RuntimeTransaction/load_and_save_long_string_on_signers_address-2277k ± 0%277k ± 0%+0.01%(p=0.000 n=9+10)
RuntimeTransaction/borrow_array_from_storage-2414k ± 0%414k ± 0%+0.01%(p=0.002 n=10+10)
RuntimeTransaction/transfer_tokens-21.03M ± 0%1.03M ± 0%+0.01%(p=0.006 n=10+8)
RuntimeTransaction/get_account_and_get_storage_capacity-22.39M ± 0%2.39M ± 0%+0.00%(p=0.009 n=10+10)
RuntimeTransaction/get_account_and_get_balance-23.17M ± 0%3.17M ± 0%~(p=0.735 n=9+10)
RuntimeTransaction/get_account_and_get_available_balance-22.55M ± 0%2.55M ± 0%~(p=0.315 n=10+10)
RuntimeTransaction/create_new_account-23.60M ± 0%3.60M ± 0%~(p=0.143 n=10+10)
RuntimeNFTBatchTransfer-2336k ± 1%336k ± 1%~(p=1.000 n=10+10)
pkg:github.com/onflow/flow-go/engine/execution/computation goos:linux goarch:amd64
ComputeBlock/4/cols/32/txes-21.84M ± 0%1.84M ± 0%~(p=0.165 n=10+10)
ComputeBlock/4/cols/64/txes-23.68M ± 0%3.68M ± 0%~(p=0.143 n=10+10)
ComputeBlock/16/cols/16/txes-23.68M ± 0%3.68M ± 0%~(p=0.063 n=10+10)
ComputeBlock/16/cols/32/txes-27.35M ± 0%7.35M ± 0%~(p=0.122 n=8+10)
ComputeBlock/16/cols/128/txes-229.4M ± 0%29.4M ± 0%~(p=0.211 n=10+9)
 
us/txdelta
pkg:github.com/onflow/flow-go/engine/execution/computation goos:linux goarch:amd64
ComputeBlock/16/cols/32/txes-23.18k ± 2%3.21k ± 1%+0.81%(p=0.017 n=9+9)
ComputeBlock/16/cols/128/txes-23.06k ± 1%3.08k ± 2%+0.80%(p=0.011 n=9+9)
ComputeBlock/1/cols/16/txes-23.69k ± 6%3.81k ±10%~(p=0.079 n=10+9)
ComputeBlock/1/cols/32/txes-23.55k ± 4%3.59k ± 1%~(p=0.089 n=9+9)
ComputeBlock/1/cols/64/txes-23.42k ± 1%3.43k ± 2%~(p=0.661 n=9+10)
ComputeBlock/1/cols/128/txes-23.29k ± 1%3.30k ± 2%~(p=0.370 n=9+8)
ComputeBlock/4/cols/16/txes-23.55k ± 2%3.56k ± 2%~(p=0.526 n=9+8)
ComputeBlock/4/cols/32/txes-23.35k ± 2%3.34k ± 1%~(p=0.646 n=9+10)
ComputeBlock/4/cols/64/txes-23.22k ± 1%3.21k ± 1%~(p=0.494 n=10+10)
ComputeBlock/4/cols/128/txes-23.14k ± 1%3.14k ± 2%~(p=0.670 n=10+10)
ComputeBlock/16/cols/16/txes-23.31k ± 0%3.32k ± 2%~(p=0.541 n=8+9)
ComputeBlock/16/cols/64/txes-23.11k ± 1%3.12k ± 2%~(p=0.382 n=10+10)
 

github-actions[bot] avatar Aug 11 '22 21:08 github-actions[bot]

Codecov Report

Merging #2963 (841b85c) into master (af019c1) will increase coverage by 0.05%. The diff coverage is 74.61%.

@@            Coverage Diff             @@
##           master    #2963      +/-   ##
==========================================
+ Coverage   54.66%   54.72%   +0.05%     
==========================================
  Files         712      712              
  Lines       66463    66536      +73     
==========================================
+ Hits        36334    36410      +76     
+ Misses      27087    27080       -7     
- Partials     3042     3046       +4
Flag Coverage Δ
unittests 54.72% <74.61%> (+0.05%) :arrow_up:

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

Impacted Files Coverage Δ
fvm/env.go 61.80% <ø> (+0.27%) :arrow_up:
fvm/transactionSequenceNum.go 54.16% <18.18%> (-10.12%) :arrow_down:
fvm/transactionInvoker.go 71.62% <41.66%> (-3.38%) :arrow_down:
fvm/handler/programs.go 63.82% <66.66%> (-4.22%) :arrow_down:
fvm/state/state_holder.go 78.31% <88.40%> (+54.31%) :arrow_up:
module/mempool/epochs/transactions.go 90.32% <0.00%> (-9.68%) :arrow_down:
consensus/hotstuff/eventloop/event_loop.go 74.82% <0.00%> (ø)
fvm/state/state.go 69.50% <0.00%> (+1.50%) :arrow_up:

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

codecov-commenter avatar Aug 11 '22 22:08 codecov-commenter

BeginNestedTransaction(WithReadRestriction(address)) I'm anti optional arguments in general. It's easy to accidentally misconfigure the call. I prefer something more explicit.

I'm also anti-abbreviation in method names (and to a lesser degree, variable names; one or two letter variable names are horrible; the bigger the variable scope, the less abbreviated the variable name should be). Abbreviation don't usually save you much in terms of typing, but impedes readability, especially if the abbreviation is company specific.

(In general, program should be written in such a way that it's easily followable/readable for a person not familiar with the code base.)

wrt 1.

By ParseRestricted, I meant "restricted to cadence runtime methods involved in the process of creating the parsed cadence program". So far, I'm aware of the following operations: GetProgram, SetProgram, GetCode, and ResolveLocation. (If cadence does any storage read besides these methods, then our caching assumption is wrong, and we need to update cache invalidation accordingly)

wrt 2.

The unfortunate part is that crypto space reinvented a lot of standard cs/database terminology. The classic example is smart contract is really just a specific from of stored procedures. I probably would have name cadence/fvm transaction as cadence/fvm interpretation instead.

In the long run, we'll probably end up with 3 layers of "transactions"

  1. cadence transaction/intepretation, which only handles interpreting cadence programs
  2. fvm state transaction/interpretation, which handles metering, storage check, spock, etc.
  3. (the real) storage transaction, which handles storage concurrency control and actual commit (Right now this is only a "view" and don't not have proper concurrency access control)

wrt 3.

That's a good idea. I'll make the recommended change

pattyshack avatar Aug 16 '22 16:08 pattyshack

i've added some comments for clarification, and combined CommitToParent & CommitToMain into Commit

pattyshack avatar Aug 16 '22 20:08 pattyshack

bors merge

pattyshack avatar Aug 25 '22 18:08 pattyshack

Build failed (retrying...):

bors[bot] avatar Aug 25 '22 20:08 bors[bot]

bors retry

pattyshack avatar Aug 25 '22 20:08 pattyshack

Already running a review

bors[bot] avatar Aug 25 '22 20:08 bors[bot]

bors merge

pattyshack avatar Aug 25 '22 22:08 pattyshack

Canceled.

bors[bot] avatar Aug 25 '22 22:08 bors[bot]

Build succeeded:

bors[bot] avatar Aug 25 '22 22:08 bors[bot]