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.

Sign up for GitHub

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

Jump to bottom

Remove delta view usage in fvm/ tests #4150

Merged
merged 1 commit into from
Apr 5, 2023
Merged

Remove delta view usage in fvm/ tests #4150

merged 1 commit into from
Apr 5, 2023

Conversation

pattyshack
Copy link
Contributor

No description provided.

@github-actions
Copy link
Contributor

github-actions bot commented Apr 3, 2023 •
edited
Loading

FVM Benchstat comparison

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

The command (for i in {1..7}; 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
ComputeBlock/16/cols/128/txes-26.11s ± 0%6.07s ± 0%~(p=1.000 n=1+1)
 
us/txdelta
ComputeBlock/16/cols/128/txes-22.98k ± 0%2.97k ± 0%~(p=1.000 n=1+1)
 
alloc/opdelta
ComputeBlock/16/cols/128/txes-21.20GB ± 0%1.22GB ± 0%~(p=1.000 n=1+1)
 
allocs/opdelta
ComputeBlock/16/cols/128/txes-217.1M ± 0%17.1M ± 0%~(p=1.000 n=1+1)
 

@codecov-commenter
Copy link

codecov-commenter commented Apr 3, 2023 •
edited
Loading

Codecov Report

Merging #4150 (db376cb) into master (ad864f3) will decrease coverage by 0.02%.
The diff coverage is 30.64%.

@@            Coverage Diff             @@
##           master    #4150      +/-   ##
==========================================
- Coverage   53.52%   53.51%   -0.02%     
==========================================
  Files         837      837              
  Lines       78436    78414      -22     
==========================================
- Hits        41985    41965      -20     
+ Misses      33100    33094       -6     
- Partials     3351     3355       +4
Flag Coverage Δ
unittests 53.51% <30.64%> (-0.02%) ⬇️

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

Impacted Files Coverage Δ
cmd/execution_builder.go 0.00% <0.00%> (ø)
cmd/scaffold.go 14.63% <0.00%> (-0.09%) ⬇️
cmd/utils.go 14.58% <ø> (+1.85%) ⬆️
cmd/verification_builder.go 0.00% <0.00%> (ø)
engine/common/follower/compliance_core.go 74.85% <ø> (+0.41%) ⬆️
engine/consensus/compliance/core.go 85.71% <0.00%> (ø)
engine/execution/state/bootstrap/bootstrap.go 36.84% <0.00%> (ø)
fvm/environment/contract_updater.go 66.77% <ø> (ø)
fvm/runtime/reusable_cadence_runtime.go 49.45% <0.00%> (ø)
module/metrics/herocache.go 0.00% <0.00%> (ø)
... and 9 more

... and 7 files with indirect coverage changes

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

SaveTheRbtz
SaveTheRbtz approved these changes Apr 3, 2023
View reviewed changes
Copy link
Contributor

@SaveTheRbtz SaveTheRbtz left a comment

Choose a reason for hiding this comment

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

nit: lint error in test

@pattyshack pattyshack force-pushed the patrick/rm-delta-view-from-fvm-tests branch from 3b617b2 to 6aeedd4 Compare April 4, 2023 17:01
@pattyshack
Copy link
Contributor Author

bors merge

bors bot added a commit that referenced this pull request Apr 4, 2023
@bors @pattyshack
Merge #4150
4cb6540 
4150: Remove delta view usage in fvm/ tests r=pattyshack a=pattyshack



Co-authored-by: Patrick Lee <patrick.lee@dapperlabs.com>
@bors
Copy link
Contributor

bors bot commented Apr 4, 2023

Build failed:

@pattyshack pattyshack force-pushed the patrick/rm-delta-view-from-fvm-tests branch from 6aeedd4 to b7e156a Compare April 4, 2023 17:17
@pattyshack
Copy link
Contributor Author

bors merge

@pattyshack pattyshack force-pushed the patrick/rm-delta-view-from-fvm-tests branch from b7e156a to db376cb Compare April 5, 2023 16:48
@pattyshack
Copy link
Contributor Author

bors merge

bors bot added a commit that referenced this pull request Apr 5, 2023
@bors @pattyshack
Merge #4130 #4150 #4160
28c45c6 
4130: Clean up script query executor delta usage r=pattyshack a=pattyshack



4150: Remove delta view usage in fvm/ tests r=pattyshack a=pattyshack



4160: Remove bad test r=pattyshack a=pattyshack

This test does not actually test the computer.  The account status was populate into the view during account creation, before the block is executed.

Co-authored-by: Patrick Lee <patrick.lee@dapperlabs.com>
@bors
Copy link
Contributor

bors bot commented Apr 5, 2023

Build failed (retrying...):

@pattyshack pattyshack force-pushed the patrick/rm-delta-view-from-fvm-tests branch from db376cb to 35a4b4b Compare April 5, 2023 17:04
@bors
Copy link
Contributor

bors bot commented Apr 5, 2023

Canceled.

@pattyshack
Copy link
Contributor Author

bors merge

@bors
Copy link
Contributor

bors bot commented Apr 5, 2023

Build succeeded:

@bors bors bot merged commit c271a58 into master Apr 5, 2023
@bors bors bot deleted the patrick/rm-delta-view-from-fvm-tests branch April 5, 2023 17:53
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@janezpodhostnik janezpodhostnik janezpodhostnik approved these changes

@zhangchiqing zhangchiqing zhangchiqing approved these changes

@SaveTheRbtz SaveTheRbtz SaveTheRbtz approved these changes

@koko1123 koko1123 Awaiting requested review from koko1123

@ramtinms ramtinms Awaiting requested review from ramtinms

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@pattyshack @codecov-commenter @SaveTheRbtz @zhangchiqing @janezpodhostnik