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

[Access] Script execution engine interface changes #4668

Merged
merged 9 commits into from
Sep 5, 2023

Conversation

sideninja
Copy link
Contributor

Closes: #4666

This PR makes sure the types used by the script execution engine are optimized so this can be reused on the access node for script execution.

@sideninja sideninja self-assigned this Aug 30, 2023
@sideninja sideninja requested a review from ramtinms as a code owner August 30, 2023 13:40
@github-actions
Copy link
Contributor

github-actions bot commented Aug 30, 2023

FVM Benchstat comparison

This branch with compared with the base branch onflow:master commit 5f9c8a0

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/1/max-concurrency-27.87s ± 0%7.74s ± 0%~(p=1.000 n=1+1)
ComputeBlock/16/cols/128/txes/2/max-concurrency-28.43s ± 0%8.61s ± 0%~(p=1.000 n=1+1)
 
us/txdelta
ComputeBlock/16/cols/128/txes/1/max-concurrency-23.84k ± 0%3.78k ± 0%~(p=1.000 n=1+1)
ComputeBlock/16/cols/128/txes/2/max-concurrency-24.12k ± 0%4.21k ± 0%~(p=1.000 n=1+1)
 
alloc/opdelta
ComputeBlock/16/cols/128/txes/1/max-concurrency-21.66GB ± 0%1.66GB ± 0%~(p=1.000 n=1+1)
ComputeBlock/16/cols/128/txes/2/max-concurrency-21.70GB ± 0%1.70GB ± 0%~(p=1.000 n=1+1)
 
allocs/opdelta
ComputeBlock/16/cols/128/txes/1/max-concurrency-221.9M ± 0%21.9M ± 0%~(p=1.000 n=1+1)
ComputeBlock/16/cols/128/txes/2/max-concurrency-222.6M ± 0%22.6M ± 0%~(p=1.000 n=1+1)
 

@codecov-commenter
Copy link

codecov-commenter commented Aug 30, 2023

Codecov Report

Patch coverage: 68.42% and project coverage change: -1.70% ⚠️

Comparison is base (2528190) 56.25% compared to head (32c5011) 54.55%.
Report is 1036 commits behind head on master.

Additional details and impacted files
@@            Coverage Diff             @@
##           master    #4668      +/-   ##
==========================================
- Coverage   56.25%   54.55%   -1.70%     
==========================================
  Files         653      917     +264     
  Lines       64699    85898   +21199     
==========================================
+ Hits        36396    46862   +10466     
- Misses      25362    35439   +10077     
- Partials     2941     3597     +656     
Flag Coverage Δ
unittests 54.55% <68.42%> (-1.70%) ⬇️

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

Files Changed Coverage Δ
cmd/bootstrap/transit/cmd/utils.go 31.54% <ø> (ø)
cmd/execution_builder.go 0.00% <0.00%> (ø)
cmd/execution_config.go 0.00% <0.00%> (ø)
engine/access/rest/middleware/metrics.go 0.00% <0.00%> (ø)
engine/access/rest/request/request.go 0.00% <0.00%> (ø)
engine/access/rest/routes/execution_result.go 56.75% <0.00%> (ø)
engine/access/rpc/backend/backend_network.go 34.11% <0.00%> (ø)
engine/access/state_stream/engine.go 0.00% <0.00%> (ø)
engine/common/rpc/convert/blocks.go 78.76% <ø> (ø)
engine/execution/state/state.go 33.82% <0.00%> (-1.57%) ⬇️
... and 79 more

... and 272 files with indirect coverage changes

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@sideninja sideninja enabled auto-merge September 1, 2023 09:07
@sideninja sideninja added this pull request to the merge queue Sep 5, 2023
"github.com/onflow/flow-go/model/flow"
"github.com/onflow/flow-go/state/protocol"
)

// ScriptExecutionState is a subset of the `state.ExecutionState` interface purposed to only access the state
// used for script execution and not mutate the execution state of the blockchain.
type ScriptExecutionState interface {
Copy link
Member

Choose a reason for hiding this comment

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

I think the ScriptExecutionState should go here . And since it's a subset of ReadOnlyExecutionState, we could make ReadOnlyExecutionState extends from ScriptExecutionState.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I guess this was automatically merged. Here's another PR that makes those changes. #4685

Merged via the queue into master with commit 1c4fd72 Sep 5, 2023
37 checks passed
@sideninja sideninja deleted the gregor/script-execution/script-engine-interfaces branch September 5, 2023 17:32
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.

[Script Execution] Optimize the script execution engine required interfaces
4 participants