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

[vm] Separation of resources, modules and aggregators #9256

Merged

georgemitenkov merged 15 commits into main from george/split-writes

Aug 1, 2023

Contributor

georgemitenkov commented Jul 23, 2023 •

edited

Loading

Description

TLDR; Cherrypicks some changes from #8227.

Motivation:
In Move VM we know what is a resource, what is a module. On top of
that adapter has aggregators. All these are merged in a single WriteSet.

Now, we split write sets into 3:

Resources
Modules
Aggregators

Why this is good:

Allows for better APIs in the future ti query different types, e.g. Move
values or compiled module.
Aligns with mvhashmap having 3 different paths for resources, modules
and aggregators.
No need to deserialize the state key to understand if this is code or not.
Aggregator write sets can be just u128, no need to have an actual WriteOp!
(Note that this PR does not add this)
No need for WriteSetMut, etc. and other storage abstractions.

Test Plan

georgemitenkov requested review from vusirikala, gelash, ziaptos and igor-aptos

July 24, 2023 19:47

georgemitenkov marked this pull request as ready for review

July 24, 2023 19:48

georgemitenkov requested review from davidiw, movekevin, zekun000, sasha8, danielxiangzl, wrwg and vgao1996 as code owners

July 24, 2023 19:48

georgemitenkov added 3 commits

July 24, 2023 22:19


          Initial split of modules, resurces & aggregators

0fe4f36


          Refactoring

61e8c80


          Refactoring pt 2

b46105a

georgemitenkov force-pushed the george/split-writes branch from 8b27a5c to b46105a Compare

July 24, 2023 20:21


          Fix rebase + format

0d5f8bb

gelash requested review from zekun000 and removed request for zekun000

July 26, 2023 23:50

gelash approved these changes

View reviewed changes

aptos-move/aptos-vm-types/src/change_set.rs Outdated Show resolved Hide resolved

aptos-move/aptos-vm-types/src/change_set.rs Outdated Show resolved Hide resolved

aptos-move/aptos-vm-types/src/change_set.rs Outdated Show resolved Hide resolved

aptos-move/aptos-vm-types/src/change_set.rs Show resolved Hide resolved

aptos-move/aptos-vm-types/src/change_set.rs

+                  /// A useful method to iterate over all writes.
+                  /// TODO(aggregator): With Aggregator V2, we would have to revisit this
+                  /// because we no longer iterate over state items.
+                  pub fn write_set_iter(&self) -> impl Iterator<Item = (&StateKey, &WriteOp)> {

Contributor

gelash Jul 27, 2023

let's make sure someone with a sense of what's acceptable in the codebase looks at it xD @zekun000 ?

Contributor

zekun000 Jul 27, 2023

looks beautiful

aptos-move/aptos-vm-types/src/change_set.rs Outdated Show resolved Hide resolved

aptos-move/aptos-vm-types/src/change_set.rs Outdated Show resolved Hide resolved

ziaptos reviewed

View reviewed changes

Contributor

ziaptos left a comment

Minor contribution from me from now (as I don't know a lot of background details).

A general comment is to try to avoid unnecessary and redundant comments - that makes maintaining the code harder in the future (one has to change both)

aptos-move/aptos-aggregator/src/delta_change_set.rs Outdated Show resolved Hide resolved

aptos-move/aptos-vm-types/src/change_set.rs Outdated Show resolved Hide resolved

aptos-move/aptos-vm-types/src/change_set.rs Outdated Show resolved Hide resolved

aptos-move/aptos-vm-types/src/change_set.rs Outdated Show resolved Hide resolved

aptos-move/aptos-vm-types/src/change_set.rs Outdated Show resolved Hide resolved

aptos-move/aptos-vm-types/src/change_set.rs Outdated Show resolved Hide resolved

aptos-move/aptos-vm-types/src/change_set.rs Outdated Show resolved Hide resolved

aptos-move/aptos-vm-types/src/change_set.rs Outdated Show resolved Hide resolved

aptos-move/aptos-vm-types/src/output.rs Outdated Show resolved Hide resolved

aptos-move/block-executor/src/proptest_types/baseline.rs Outdated Show resolved Hide resolved

georgemitenkov added 4 commits

July 27, 2023 10:56


          addressed a first set of comments

a15fc70


          RIP delta change set


          style changes for transaction output conversions

156be36


          tests improvements

e57013d

igor-aptos requested changes

View reviewed changes

Contributor

igor-aptos left a comment

there are bunch of comments marked resolved, but code not yet changed - do you have some pending code locally to update the PR with?
requesting changes for you to update the PR with that code.

aptos-move/aptos-vm-types/src/change_set.rs Outdated Show resolved Hide resolved

aptos-move/aptos-vm-types/src/change_set.rs Outdated Show resolved Hide resolved

aptos-move/aptos-vm-types/src/change_set.rs Show resolved Hide resolved

aptos-move/aptos-vm-types/src/change_set.rs Outdated Show resolved Hide resolved

aptos-move/aptos-vm-types/src/change_set.rs Outdated Show resolved Hide resolved

aptos-move/aptos-vm-types/src/output.rs Outdated Show resolved Hide resolved

aptos-move/block-executor/src/executor.rs Outdated Show resolved Hide resolved

georgemitenkov added 3 commits

July 27, 2023 18:51


          add missing comment

339d2e5


          test for conversion from/to storage change set

1e46eaa


          Merge branch 'main' into george/split-writes

63a81f5

georgemitenkov added the CICD:run-e2e-tests label

This comment has been minimized.

Sign in to view

This comment has been minimized.

Sign in to view

This comment has been minimized.

Sign in to view

This comment has been minimized.

Sign in to view

This comment has been minimized.

Sign in to view

zekun000 reviewed

View reviewed changes

aptos-move/aptos-vm-types/src/change_set.rs

+                  /// A useful method to iterate over all writes.
+                  /// TODO(aggregator): With Aggregator V2, we would have to revisit this
+                  /// because we no longer iterate over state items.
+                  pub fn write_set_iter(&self) -> impl Iterator<Item = (&StateKey, &WriteOp)> {

Contributor

zekun000 Jul 27, 2023

looks beautiful

aptos-move/aptos-vm-types/src/change_set.rs Outdated Show resolved Hide resolved

aptos-move/aptos-vm/src/move_vm_ext/respawned_session.rs Outdated Show resolved Hide resolved

aptos-move/block-executor/src/executor.rs Outdated Show resolved Hide resolved

igor-aptos approved these changes

View reviewed changes

vgao1996 approved these changes

View reviewed changes

georgemitenkov added 3 commits

August 1, 2023 00:13


          addressed remaining comments

c0b8e5f


          Merge branch 'main' into george/split-writes

629f4c9


          reverted some not needed changes

fcae895

georgemitenkov force-pushed the george/split-writes branch from f2b7888 to fcae895 Compare

July 31, 2023 22:21

georgemitenkov enabled auto-merge (squash)

July 31, 2023 22:23

This comment has been minimized.

Sign in to view

This comment has been minimized.

Sign in to view

This comment has been minimized.

Sign in to view

This comment has been minimized.

Sign in to view

This comment has been minimized.

Sign in to view

This comment has been minimized.

Sign in to view


          Bug fix in respawned session

24272e9

This comment has been minimized.

Sign in to view

This comment has been minimized.

Sign in to view

This comment has been minimized.

Sign in to view

Contributor

github-actions bot commented Aug 1, 2023

✅ Forge suite `compat` success on `aptos-node-v1.5.1` ==> `24272e9f77ea778cf3b3e28b2aef0fd5c506c155`

Compatibility test results for aptos-node-v1.5.1 ==> 24272e9f77ea778cf3b3e28b2aef0fd5c506c155 (PR)
1. Check liveness of validators at old version: aptos-node-v1.5.1
compatibility::simple-validator-upgrade::liveness-check : committed: 4703 txn/s, latency: 7038 ms, (p50: 7500 ms, p90: 9900 ms, p99: 10500 ms), latency samples: 169340
2. Upgrading first Validator to new version: 24272e9f77ea778cf3b3e28b2aef0fd5c506c155
compatibility::simple-validator-upgrade::single-validator-upgrade : committed: 1855 txn/s, latency: 15882 ms, (p50: 18400 ms, p90: 22300 ms, p99: 22600 ms), latency samples: 92760
3. Upgrading rest of first batch to new version: 24272e9f77ea778cf3b3e28b2aef0fd5c506c155
compatibility::simple-validator-upgrade::half-validator-upgrade : committed: 1780 txn/s, latency: 15736 ms, (p50: 18900 ms, p90: 21900 ms, p99: 22600 ms), latency samples: 92600
4. upgrading second batch to new version: 24272e9f77ea778cf3b3e28b2aef0fd5c506c155
compatibility::simple-validator-upgrade::rest-validator-upgrade : committed: 3276 txn/s, latency: 9634 ms, (p50: 10200 ms, p90: 13300 ms, p99: 14000 ms), latency samples: 140880
5. check swarm health
Compatibility test for aptos-node-v1.5.1 ==> 24272e9f77ea778cf3b3e28b2aef0fd5c506c155 passed
Test Ok

Grafana dashboard
Humio Logs
Test runner output
Test run is land-blocking

Contributor

github-actions bot commented Aug 1, 2023

✅ Forge suite `realistic_env_max_load` success on `24272e9f77ea778cf3b3e28b2aef0fd5c506c155`

two traffics test: inner traffic : committed: 6614 txn/s, latency: 5903 ms, (p50: 5400 ms, p90: 7800 ms, p99: 16900 ms), latency samples: 2870480
two traffics test : committed: 100 txn/s, latency: 3190 ms, (p50: 3100 ms, p90: 3800 ms, p99: 7700 ms), latency samples: 1740
Max round gap was 1 [limit 4] at version 1454310. Max no progress secs was 5.314948 [limit 10] at version 1454310.
Test Ok

Grafana dashboard
Humio Logs
Test runner output
Test run is land-blocking

Contributor

github-actions bot commented Aug 1, 2023

✅ Forge suite `framework_upgrade` success on `aptos-node-v1.5.1` ==> `24272e9f77ea778cf3b3e28b2aef0fd5c506c155`

Compatibility test results for aptos-node-v1.5.1 ==> 24272e9f77ea778cf3b3e28b2aef0fd5c506c155 (PR)
Upgrade the nodes to version: 24272e9f77ea778cf3b3e28b2aef0fd5c506c155
framework_upgrade::framework-upgrade::full-framework-upgrade : committed: 3052 txn/s, latency: 7180 ms, (p50: 7500 ms, p90: 10200 ms, p99: 15100 ms), latency samples: 167900
5. check swarm health
Compatibility test for aptos-node-v1.5.1 ==> 24272e9f77ea778cf3b3e28b2aef0fd5c506c155 passed
Test Ok

Grafana dashboard
Humio Logs
Test runner output
Test run is land-blocking

georgemitenkov merged commit 137acee into main

46 checks passed

georgemitenkov deleted the george/split-writes branch

August 1, 2023 08:40

gedigi pushed a commit that referenced this pull request


          [vm] Separation of resources, modules and aggregators (#9256)

167fcec

TLDR; Cherrypicks some changes from #8227.

In Move VM we know what is a resource, what is a module. On top of
that adapter has aggregators. All these have been merged into a single
`WriteSet` previously.

Now, we split write sets into 3:
- Resources
- Modules
- Aggregators

As a result, the code was cleaned-up.

xbtmatt pushed a commit that referenced this pull request


          [vm] Separation of resources, modules and aggregators (#9256)

e53f869

TLDR; Cherrypicks some changes from #8227.

In Move VM we know what is a resource, what is a module. On top of
that adapter has aggregators. All these have been merged into a single
`WriteSet` previously.

Now, we split write sets into 3:
- Resources
- Modules
- Aggregators

As a result, the code was cleaned-up.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

zekun000 zekun000 left review comments

ziaptos ziaptos left review comments

vgao1996 vgao1996 approved these changes

gelash gelash approved these changes

igor-aptos igor-aptos approved these changes

vusirikala Awaiting requested review from vusirikala

davidiw Awaiting requested review from davidiw davidiw is a code owner

movekevin Awaiting requested review from movekevin movekevin is a code owner

sasha8 Awaiting requested review from sasha8 sasha8 is a code owner

danielxiangzl Awaiting requested review from danielxiangzl danielxiangzl is a code owner

wrwg Awaiting requested review from wrwg wrwg is a code owner

runtian-zhou Awaiting requested review from runtian-zhou

Labels

CICD:run-e2e-tests