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

Refactor epochs related contract deployments, use blueprints pattern #2057

Merged
merged 8 commits into from Mar 8, 2022

Conversation

kc1116
Copy link
Contributor

@kc1116 kc1116 commented Feb 23, 2022

This PR updates the transactions related to epochs in the fvm bootstrap procedure, it moves these transactions to the blueprints package.

…ootstrap procedure

Use the blueprints pattern
@github-actions
Copy link
Contributor

github-actions bot commented Feb 23, 2022

FVM Benchstat comparison

This branch with compared with the base branch onflow:master commit a3a7ad8

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/get_signer_vault-245.6ms ± 3%48.1ms ± 5%+5.42%(p=0.002 n=6+7)
RuntimeTransaction/get_account_and_get_balance-2633ms ± 2%656ms ± 5%+3.64%(p=0.014 n=6+7)
RuntimeTransaction/get_public_account-240.1ms ± 2%41.3ms ± 2%+2.84%(p=0.026 n=7+7)
RuntimeTransaction/get_account_and_get_storage_used-252.8ms ± 3%54.3ms ± 3%+2.76%(p=0.011 n=7+7)
RuntimeTransaction/reference_tx-235.8ms ± 5%36.5ms ± 2%~(p=0.101 n=7+6)
RuntimeTransaction/convert_int_to_string-237.5ms ± 4%38.6ms ± 3%~(p=0.073 n=7+7)
RuntimeTransaction/convert_int_to_string_and_concatenate_it-239.3ms ± 5%40.3ms ± 3%~(p=0.128 n=7+7)
RuntimeTransaction/get_signer_address-236.7ms ± 5%37.3ms ± 5%~(p=0.259 n=7+7)
RuntimeTransaction/get_account_and_get_available_balance-2537ms ± 3%546ms ± 3%~(p=0.259 n=7+7)
RuntimeTransaction/get_account_and_get_storage_capacity-2489ms ± 6%503ms ± 4%~(p=0.138 n=7+6)
RuntimeTransaction/get_signer_receiver-257.1ms ± 4%57.8ms ± 2%~(p=0.234 n=7+6)
RuntimeTransaction/transfer_tokens-2207ms ± 4%207ms ± 3%~(p=0.902 n=7+7)
RuntimeTransaction/load_and_save_empty_string_on_signers_address-244.9ms ± 3%45.5ms ± 1%~(p=0.445 n=7+6)
RuntimeTransaction/load_and_save_long_string_on_signers_address-290.7ms ± 3%92.6ms ± 2%~(p=0.097 n=7+7)
RuntimeTransaction/create_new_account-21.32s ± 3%1.34s ± 1%~(p=0.234 n=7+6)
RuntimeTransaction/call_empty_contract_function-242.0ms ± 3%42.5ms ± 1%~(p=0.534 n=7+6)
RuntimeTransaction/emit_event-257.8ms ± 2%58.1ms ± 4%~(p=0.710 n=7+7)
RuntimeNFTBatchTransfer-2135ms ± 2%137ms ± 2%~(p=0.128 n=7+7)
 

@kc1116 kc1116 changed the title refactor epochs related contract deployment transactions in the fvm b… Refactor epochs related contract deployments, use blueprints pattern Feb 24, 2022
Copy link
Member

@jordanschalm jordanschalm left a comment

Choose a reason for hiding this comment

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

Could we move over registerNodeTransaction as well? Otherwise looks great 🧹🧹

@kc1116 kc1116 requested review from jordanschalm and removed request for jordanschalm February 28, 2022 22:53
@codecov-commenter
Copy link

codecov-commenter commented Mar 1, 2022

Codecov Report

Merging #2057 (9c8f2c0) into master (a3a7ad8) will increase coverage by 0.07%.
The diff coverage is 78.04%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #2057      +/-   ##
==========================================
+ Coverage   57.25%   57.32%   +0.07%     
==========================================
  Files         633      633              
  Lines       36836    36745      -91     
==========================================
- Hits        21089    21065      -24     
+ Misses      13095    13026      -69     
- Partials     2652     2654       +2     
Flag Coverage Δ
unittests 57.32% <78.04%> (+0.07%) ⬆️

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

Impacted Files Coverage Δ
fvm/bootstrap.go 84.56% <78.04%> (+9.75%) ⬆️
...s/hotstuff/votecollector/staking_vote_processor.go 85.96% <0.00%> (-3.51%) ⬇️
consensus/hotstuff/eventloop/event_loop.go 68.29% <0.00%> (ø)
...sus/approvals/assignment_collector_statemachine.go 47.11% <0.00%> (+4.80%) ⬆️

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 a3a7ad8...9c8f2c0. Read the comment docs.

@kc1116 kc1116 requested a review from joshuahannan March 6, 2022 14:23
"github.com/onflow/flow-go/model/bootstrap"

"github.com/onflow/flow-go/module/epochs"

"github.com/onflow/flow-go/model/flow"
)

Copy link
Member

Choose a reason for hiding this comment

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

Is is possible to use templates from the core contracts repo and SDK here instead of hard-coding the transaction text?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It looks like most of the hardcoded transactions either don't have a corresponding tx in templates or they are different slightly.

Copy link
Member

Choose a reason for hiding this comment

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

that could mean it might be worth adding some of them, but I guess that make sense

@kc1116
Copy link
Contributor Author

kc1116 commented Mar 7, 2022

bors merge

@bors
Copy link
Contributor

bors bot commented Mar 7, 2022

Canceled.

@kc1116
Copy link
Contributor Author

kc1116 commented Mar 8, 2022

bors merge

bors bot added a commit that referenced this pull request Mar 8, 2022
2057: Refactor epochs related contract deployments, use blueprints pattern r=kc1116 a=kc1116

This PR updates the transactions related to epochs in the fvm bootstrap procedure, it moves these transactions to the blueprints package. 

Co-authored-by: Khalil Claybon <khalil.claybon@dapperlabs.com>
@bors
Copy link
Contributor

bors bot commented Mar 8, 2022

Build failed:

@kc1116 kc1116 merged commit f04541c into master Mar 8, 2022
@kc1116 kc1116 deleted the khalil/5586-refactor-epochs-fvt-txs branch March 8, 2022 22:17
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

4 participants