-
Notifications
You must be signed in to change notification settings - Fork 3.7k
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
Various variants of "simplified/dummy" executors #15152
Conversation
⏱️ 16h 4m total CI duration on this PR
|
a9a56e8
to
ef4d774
Compare
66a9235
to
fd3a91a
Compare
}, | ||
} | ||
|
||
impl NativeTransaction { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In my remote sharding experiments, native vm was not able to handle this one txn in the beginning. You have changed the code since then, but it might worthwhile to run an "unsharded execution" in the executor-benchmark
2024-11-16T00:33:08.305178Z [main] INFO storage/aptosdb/src/state_store/mod.rs:451 Initializing BufferedState. {"latest_snapshot_version":null,"num_transactions":0}
2024-11-16T00:33:08.305256Z [main] INFO storage/aptosdb/src/state_store/mod.rs:547 StateStore initialization finished. {"latest_in_memory_root_hash":"5350415253455f4d45524b4c455f504c414345484f4c4445525f484153480000","latest_in_memory_version":null,"latest_snapshot_root_hash":"5350415253455f4d45524b4c455f504c414345484f4c4445525f484153480000","latest_snapshot_version":null}
2024-11-16T00:33:08.305735Z [main] INFO aptos-move/aptos-vm/src/aptos_vm.rs:2557 Executing block, transaction count: 1 {"name":"miscellaneous","txn_idx":0}
@@ -1,4 +1,8 @@ | |||
// Copyright (c) Aptos Foundation | |||
// SPDX-License-Identifier: Apache-2.0 | |||
|
|||
pub mod aptos_vm_uncoordinated; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this the best place to have 'native' module ??
How about having the native module in "aptos-move/aptos-vm/src/block_executor/" ?
Native is used inside of "aptos-move/aptos-vm/src/..." and I suppose it will be used more from 'lower layers'. executor-benchmark seems to be too high level for this. Even in this code, probably need for 'vm_wrapper' comes from native being in executor-benchmark
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
vm_wrapper is one line, so the tradeoff seems right
we don't want to use it in prod, only for the benchmark. we can create a separate native project if needed later, but this sounds reasonable to me.
putting it in aptos-move/aptos-vm/src/... seems poluting it there.
)) | ||
} | ||
|
||
// fn to_abort(status: TransactionStatus) -> TransactionOutput { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
dead code. maybe remove
total_supply: AtomicU64, | ||
} | ||
|
||
// impl CommonNativeRawTransactionExecutor for NativeNoStorageRawTransactionExecutor { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
dead code
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Left some comments..
But overall, seems good to me
4a8b8ed
to
8a854ac
Compare
8a854ac
to
4e0e2c9
Compare
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
4e0e2c9
to
dee9376
Compare
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
✅ Forge suite
|
✅ Forge suite
|
✅ Forge suite
|
Self::get_value(account_key, state_view) | ||
} | ||
|
||
pub fn get_coin_store( | ||
pub fn get_fa_store( | ||
store_key: &StateKey, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
how does this work?
fa store is in the rg.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In db, yes. But that is StateView's implementation detail - StateView handles the translation
Description
Adding all these executor configurations, for understanding upper bounds on different approaches/optimizations:
How Has This Been Tested?
Key Areas to Review
Type of Change
Which Components or Systems Does This Change Impact?
Checklist