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

[Executor] Unify views used for block execution #5731

Merged
merged 1 commit into from Dec 18, 2022
Merged

[Executor] Unify views used for block execution #5731

merged 1 commit into from Dec 18, 2022

Conversation

gelash
Copy link
Contributor

@gelash gelash commented Nov 30, 2022

This makes the interface difference between sequential and parallel execution an implementation detail of block executor.
It's better to be an implementation detail, for example tomorrow we could delete sequential execution altogether and run parallel with 1 thread (thought experiment, I'm not proposing we do this), and outside world would not need to care.

Other benefits:
(0) Ability to access storage values from block_executor - before this was only possible in the wrapper, that would find data by combining view from the block with data from storage (depending on statekey). Having access to storage values is important e.g. to resolve aggregator deltas more efficiently, and to not expose delta_resolver to the wrapper - but this is coming next as a separate PR (see b423013)
(1) no more ugly leaky interfaces like execute_mvhashmap_view and execute_btreemap view
(2) delete unused StateViewCache only used in tests as a move resolver, instead resolve directly
(3) delete VersionedView from the wrapper, instead provide wrapping view implementations at one place inside block_executor (view.rs)

@gelash gelash added the CICD:run-e2e-tests when this label is present github actions will run all land-blocking e2e tests from the PR label Nov 30, 2022
@github-actions

This comment has been minimized.

@github-actions

This comment has been minimized.

@github-actions

This comment has been minimized.

@github-actions

This comment has been minimized.

Copy link
Contributor

@zekun000 zekun000 left a comment

Choose a reason for hiding this comment

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

this is very confusing to me, there're so many similar things with slightly different names, we probably can find a better way.

there're StateView vs DataView, the only difference I can tell is that StateView hardcoded the StateKey, I'd rather unify them to just support a generic key.

there're VersionedView and AptosVersionedView, I'm not sure I understand why we need AptosVersionedView instead of just impl StateView for VersionedView.

aptos-move/aptos-vm/src/block_executor/storage_wrapper.rs Outdated Show resolved Hide resolved
aptos-move/block-executor/src/task.rs Outdated Show resolved Hide resolved
aptos-move/block-executor/src/proptest_types/types.rs Outdated Show resolved Hide resolved
aptos-move/block-executor/src/view.rs Outdated Show resolved Hide resolved
@gelash
Copy link
Contributor Author

gelash commented Nov 30, 2022

Yes, it's really ugly, we should definitely improve. I needed some ideas.
Most of this ugliness is due to having generic ParallelExecutor (which we do use to easily generate different types of keys for testing), due to orphan rules in wrapper (types and block_executor being different crates), and in fact, due to StateView not being generic.

@zekun000: do you think it's actually okay to support generic key in StateView itself? I thought it's used everywhere but now I see it's actually not, mainly an aptos-move construct.
If we can do that, hopefully can get rid of DataView altogether, and also not have a stupid wrapper over SpeculativeView which already wraps on top of the base view.

@github-actions

This comment has been minimized.

@github-actions

This comment has been minimized.

@github-actions

This comment has been minimized.

@github-actions

This comment has been minimized.

@gelash gelash force-pushed the seqparviews branch 2 times, most recently from 1bc9df6 to 2acca0f Compare December 4, 2022 02:22
@gelash
Copy link
Contributor Author

gelash commented Dec 4, 2022

Comments must be addressed - all ugly wrappers are gone and StateView is now generic on key.

@github-actions

This comment has been minimized.

@github-actions

This comment has been minimized.

@github-actions

This comment has been minimized.

@gelash
Copy link
Contributor Author

gelash commented Dec 10, 2022

My general question is where should we define this TStateView? IMHO it is used more for unifying the two views in the parallel executor so it seems that defining it in AptosVM seems to be the more appropriate location? Then we don't need to change anything outside? IMHO we shouldn't leak that out.

Unfortunately the problem here would be a circular dependency between AptosVM and block_executor. We have to work it out at some point, but not clear if it's within easy refactoring reach yet.

@github-actions

This comment has been minimized.

@github-actions

This comment has been minimized.

@github-actions

This comment has been minimized.

@github-actions

This comment has been minimized.

@github-actions

This comment has been minimized.

@github-actions

This comment has been minimized.

@github-actions

This comment has been minimized.

@github-actions

This comment has been minimized.

@github-actions

This comment has been minimized.

@github-actions

This comment has been minimized.

@github-actions

This comment has been minimized.

@github-actions

This comment has been minimized.

Copy link
Contributor

@runtian-zhou runtian-zhou left a comment

Choose a reason for hiding this comment

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

Thanks for refactoring!

@github-actions

This comment has been minimized.

@github-actions

This comment has been minimized.

@github-actions
Copy link
Contributor

✅ Forge suite land_blocking success on 7a3e7fb1ea2a85003f9a4dfd3aa54e4908adae3f

performance benchmark with full nodes : 6342 TPS, 6264 ms latency, 9000 ms p99 latency,(!) expired 140 out of 2708340 txns
Test Ok

@github-actions
Copy link
Contributor

✅ Forge suite compat success on testnet_2d8b1b57553d869190f61df1aaf7f31a8fc19a7b ==> 7a3e7fb1ea2a85003f9a4dfd3aa54e4908adae3f

Compatibility test results for testnet_2d8b1b57553d869190f61df1aaf7f31a8fc19a7b ==> 7a3e7fb1ea2a85003f9a4dfd3aa54e4908adae3f (PR)
1. Check liveness of validators at old version: testnet_2d8b1b57553d869190f61df1aaf7f31a8fc19a7b
compatibility::simple-validator-upgrade::liveness-check : 7259 TPS, 5339 ms latency, 8600 ms p99 latency,no expired txns
2. Upgrading first Validator to new version: 7a3e7fb1ea2a85003f9a4dfd3aa54e4908adae3f
compatibility::simple-validator-upgrade::single-validator-upgrade : 4387 TPS, 9238 ms latency, 12000 ms p99 latency,no expired txns
3. Upgrading rest of first batch to new version: 7a3e7fb1ea2a85003f9a4dfd3aa54e4908adae3f
compatibility::simple-validator-upgrade::half-validator-upgrade : 4320 TPS, 9433 ms latency, 12800 ms p99 latency,no expired txns
4. upgrading second batch to new version: 7a3e7fb1ea2a85003f9a4dfd3aa54e4908adae3f
compatibility::simple-validator-upgrade::rest-validator-upgrade : 6449 TPS, 6191 ms latency, 10400 ms p99 latency,no expired txns
5. check swarm health
Compatibility test for testnet_2d8b1b57553d869190f61df1aaf7f31a8fc19a7b ==> 7a3e7fb1ea2a85003f9a4dfd3aa54e4908adae3f passed
Test Ok

@gelash gelash merged commit 2bab2aa into main Dec 18, 2022
@gelash gelash deleted the seqparviews branch December 18, 2022 00:02
@Markuze Markuze mentioned this pull request Dec 26, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
CICD:run-e2e-tests when this label is present github actions will run all land-blocking e2e tests from the PR
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants