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

chore(avm-simulator): refactor of AVM Simulator to more closely match YP #4407

Closed
wants to merge 14 commits into from
Closed

chore(avm-simulator): refactor of AVM Simulator to more closely match YP #4407

wants to merge 14 commits into from

Conversation

dbanks12
Copy link
Collaborator

@dbanks12 dbanks12 commented Feb 4, 2024

No description provided.

@dbanks12 dbanks12 added the C-avm Component: AVM related tickets (aka public VM) label Feb 4, 2024
@dbanks12 dbanks12 self-assigned this Feb 4, 2024
@AztecBot
Copy link
Collaborator

AztecBot commented Feb 4, 2024 •
edited
Loading

Benchmark results

Metrics with a significant change:

  • note_history_trial_decrypting_time_in_ms (10): 403 (+125%)
Detailed results

All benchmarks are run on txs on the Benchmarking contract on the repository. Each tx consists of a batch call to create_note and increment_balance, which guarantees that each tx has a private call, a nested private call, a public call, and a nested public call, as well as an emitted private note, an unencrypted log, and public storage read and write.

This benchmark source data is available in JSON format on S3 here.

Values are compared against data from master at commit 8569e7c0 and shown if the difference exceeds 1%.

L2 block published to L1

Each column represents the number of txs on an L2 block published to L1.

Metric 8 txs 32 txs 128 txs
l1_rollup_calldata_size_in_bytes 45,636 180,516 720,036
l1_rollup_calldata_gas 222,504 870,504 3,465,396
l1_rollup_execution_gas 314,991 988,244 3,685,053
l2_block_processing_time_in_ms 1,154 4,330 (-1%) 17,157
note_successful_decrypting_time_in_ms 302 (-1%) 962 (-1%) 3,589
note_trial_decrypting_time_in_ms 16.4 89.8 (-10%) 143 (+2%)
l2_block_building_time_in_ms 17,682 69,928 281,037
l2_block_rollup_simulation_time_in_ms 13,038 51,551 207,605
l2_block_public_tx_process_time_in_ms 4,618 18,306 73,185

L2 chain processing

Each column represents the number of blocks on the L2 chain where each block has 16 txs.

Metric 5 blocks 10 blocks
node_history_sync_time_in_ms 13,846 26,319
note_history_successful_decrypting_time_in_ms 2,502 (+4%) 4,699 (-2%)
note_history_trial_decrypting_time_in_ms 86.4 (-8%) ⚠️ 403 (+125%)
node_database_size_in_bytes 17,317,968 33,050,704
pxe_database_size_in_bytes 29,923 59,478

Circuits stats

Stats on running time and I/O sizes collected for every circuit run across all benchmarks.

Circuit circuit_simulation_time_in_ms circuit_input_size_in_bytes circuit_output_size_in_bytes
private-kernel-init 310 44,481 26,193
private-kernel-ordering 193 44,513 14,929
base-rollup 1,406 128,970 881
root-rollup 82.6 4,088 677
private-kernel-inner 415 71,640 26,193
public-kernel-private-input 240 32,615 26,193
public-kernel-non-first-iteration 239 32,657 26,193
merge-rollup 7.63 2,608 881

Tree insertion stats

The duration to insert a fixed batch of leaves into each tree type.

Metric 1 leaves 2 leaves 8 leaves 16 leaves 32 leaves 128 leaves 64 leaves 512 leaves 1024 leaves 2048 leaves 8192 leaves
batch_insert_into_append_only_tree_16_depth_ms 10.2 10.6 (-5%) 12.6 (-4%) 17.0 (+1%) 23.4 (+2%) 63.7 N/A N/A N/A N/A N/A
batch_insert_into_append_only_tree_16_depth_hash_count 16.9 17.5 23.0 31.6 47.0 143 N/A N/A N/A N/A N/A
batch_insert_into_append_only_tree_16_depth_hash_ms 0.593 0.591 (-5%) 0.538 (-4%) 0.526 0.490 (+2%) 0.440 N/A N/A N/A N/A N/A
batch_insert_into_append_only_tree_32_depth_ms N/A N/A N/A N/A N/A 74.2 47.5 238 463 (+1%) 899 3,545
batch_insert_into_append_only_tree_32_depth_hash_count N/A N/A N/A N/A N/A 159 96.0 543 1,055 2,079 8,223
batch_insert_into_append_only_tree_32_depth_hash_ms N/A N/A N/A N/A N/A 0.459 0.488 0.436 0.433 (+1%) 0.427 0.426
batch_insert_into_indexed_tree_20_depth_ms N/A N/A N/A N/A N/A 103 56.6 350 691 (+1%) 1,354 (-2%) 5,398
batch_insert_into_indexed_tree_20_depth_hash_count N/A N/A N/A N/A N/A 197 104 691 1,363 2,707 10,771
batch_insert_into_indexed_tree_20_depth_hash_ms N/A N/A N/A N/A N/A 0.491 0.496 0.474 (-1%) 0.475 (+1%) 0.470 (-1%) 0.471
batch_insert_into_indexed_tree_40_depth_ms N/A N/A N/A 56.5 N/A N/A N/A N/A N/A N/A N/A
batch_insert_into_indexed_tree_40_depth_hash_count N/A N/A N/A 94.1 N/A N/A N/A N/A N/A N/A N/A
batch_insert_into_indexed_tree_40_depth_hash_ms N/A N/A N/A 0.578 N/A N/A N/A N/A N/A N/A N/A

Miscellaneous

Transaction sizes based on how many contracts are deployed in the tx.

Metric 0 deployed contracts 1 deployed contracts
tx_size_in_bytes 15,575 37,974

Transaction processing duration by data writes.

Metric 0 new commitments 1 new commitments
tx_pxe_processing_time_ms 546 1,220
Metric 0 public data writes 1 public data writes
tx_sequencer_processing_time_ms 0.499 (-20%) 572

@dbanks12 dbanks12 marked this pull request as ready for review February 4, 2024 22:41
fcarreiro
fcarreiro reviewed Feb 5, 2024
View reviewed changes
yarn-project/acir-simulator/src/avm/avm_context.ts
*/
export class AvmContext {
/** Contains constant variables provided by the kernel */
private executionEnvironment: AvmExecutionEnvironment;
public environment: AvmExecutionEnvironment = initExecutionEnvironment();
Copy link
Contributor

Choose a reason for hiding this comment

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

I think you don't want initExecutionEnvironment here since you have it in the constructor.

yarn-project/acir-simulator/src/avm/avm_context.ts
import { AvmMachineState, InitialAvmMachineState } from './avm_machine_state.js';
import { AvmContractCallResults } from './avm_message_call_result.js';
import { AvmExecutionError, InvalidProgramCounterError, NoBytecodeForContractError } from './errors.js';
import { initExecutionEnvironment, initInitialMachineState } from './fixtures/index.js';
Copy link
Contributor

Choose a reason for hiding this comment

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

It seems wrong to include test fixture code in non-test code. Do you really need these inits?

yarn-project/acir-simulator/src/avm/avm_context.ts
this.executionEnvironment.address,
selector,
);
setInstructions(instructions: Instruction[]) {
Copy link
Contributor

Choose a reason for hiding this comment

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

This feels strange. Consider having an execute() that calls another (public) execute(Instruction[]) instead.

yarn-project/acir-simulator/src/avm/avm_context.ts
const newExecutionEnvironment = parentEnvironment.deriveEnvironmentForNestedCall(address, calldata);
const forkedState = parentWorldState.fork();
const nestedContext = new AvmContext(forkedState, newExecutionEnvironment, initialMachineState);
await nestedContext.fetchAndDecodeBytecode();
Copy link
Contributor

Choose a reason for hiding this comment

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

Why do you want to do the fetching and decoding here (turning this into async) if execute already does it? It feels more natural to have it in execute and then you can even avoid storing the instructions in the class, you just use them on the spot.

fcarreiro
fcarreiro requested changes Feb 5, 2024
View reviewed changes
Copy link
Contributor

@fcarreiro fcarreiro left a comment

Choose a reason for hiding this comment

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

I like having execute take AvmContext, however I think what makes the most sense is for AvmContext to not handle calls etc and only be a structure (forking/etc is ok). I'll try to come up with a proposal but in any case we can talk about this :)

yarn-project/acir-simulator/src/avm/opcodes/external_calls.ts

const avmContext = AvmContext.prepExternalCallContext(
const nestedContext = await AvmContext.createNestedContractCallContext(
Copy link
Contributor

Choose a reason for hiding this comment

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

Observe that if you add Call and StaticCall to the instruction set, this use of AvmContext still creates the dependency loop. Also note that this could just be a non-static method and it would make a lot of sense.

With those changes, then you can import AvmContext as type only, and then the loop "disappears".

@dbanks12
Copy link
Collaborator Author

dbanks12 commented Feb 5, 2024

Replaced by #4424

@dbanks12 dbanks12 closed this Feb 5, 2024
@dbanks12 dbanks12 deleted the db/avm-simulator-refactor-to-yp branch April 11, 2024 17:54
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@fcarreiro fcarreiro fcarreiro requested changes

@Maddiaa0 Maddiaa0 Awaiting requested review from Maddiaa0

Assignees

@dbanks12 dbanks12

Labels
C-avm Component: AVM related tickets (aka public VM)
Projects
A3
Archived in project
Milestone
AVM Simulator Phase 1 (partial)
Development

Successfully merging this pull request may close these issues.
3 participants
@dbanks12 @AztecBot @fcarreiro