Fixed benchmarks for the `0.25.3` #1870

xgreenx · 2024-04-29T11:03:55Z

The PR updates benchmarks to work properly with new VM rules and fixes the issue with non-optimal InMemoryTransaction behavior for the size_of_value method.

Because of the new VM rules: the memory that we are accessing should be initialized. The cfe opcode in the benchmarks expands the stack to allocate the memory that will later be used by the instruction that we are benchmarking.

Before requesting review

I have reviewed the code myself

…ture/benchmarks-0.25.2 # Conflicts: # benches/benches/vm_set/blockchain.rs # benches/src/lib.rs

# Conflicts: # benches/benches/vm_set/blockchain.rs # benches/src/lib.rs

# Conflicts: # bin/fuel-core/chainspec/dev-testnet/state_transition_bytecode.wasm # bin/fuel-core/chainspec/testnet/state_transition_bytecode.wasm

bvrooman

Just a small optional correct and question

CHANGELOG.md

bvrooman · 2024-04-29T21:04:00Z

benches/benches/vm_set/flow.rs

@@ -70,7 +70,7 @@ pub fn run(c: &mut Criterion) {
            format!("{i}"),
            VmBench::contract(rng, op::retd(RegId::ONE, 0x10))
                .unwrap()
-                .with_post_call(vec![op::movi(0x10, *i)])
+                .with_post_call(vec![op::movi(0x10, *i), op::cfe(0x10)])


I'm not sure I understand the significance of adding cfe here or why it uses these specific registers. Can you elaborate on why we need this change? Is this just to make sure our benchmarks account for the worst case scenario?

Because of the new VM rules: the memory that we are accessing should be initialized. The cfe opcode in the benchmarks expands the stack to allocate the memory that will later be used by the instruction that we are benchmarking.

I think I understand better. In this example, we have retd(RegId::ONE, 0x10), where we return the memory in the range from *ONE to *0x10. So we need to extend the call stack by the value pointed by 0x10.

Co-authored-by: Brandon Vrooman <[email protected]>

MitchTurner · 2024-04-29T21:17:48Z

benches/benches/vm_initialization.rs

@@ -85,6 +87,9 @@ pub fn vm_initialization(c: &mut Criterion) {
        if size as u64 > consensus_params.script_params().max_script_data_length() {
            break
        }
+        if 2 * size as u64 > consensus_params.tx_params().max_size() {


Can we put this into a helper with the name that explains why this should break? Same with the above break.

if `something_size_is_foo_bar_baz(size, consensus_params)` { break }

It's not clear to me what the significance of size (size of what?) being half as big as the max tx size.

MitchTurner · 2024-04-29T21:22:28Z

benches/benches/vm_initialization.rs

+
+                // Initialize the VM and require the allocation of the whole memory
+                // to charge for the worst possible case.
+                black_box({


I could say a the same for a lot of this. Can we move everthing in these curlies into some helper?

MitchTurner · 2024-04-29T21:29:07Z

crates/storage/src/transactional.rs

@@ -249,6 +283,31 @@ where
            self.storage.get(key, column)
        }
    }
+
+    fn read(


I'm confused. Did we not have implementations for these methods before on InMemoryTransaction?

We had a default implementation that was calling a get method which is less optimal in the case of RocksDB.

…feature/benchmarks-0.25.2

MitchTurner · 2024-04-30T07:13:18Z

benches/benches/vm_initialization.rs

+                let vm = black_box(
                    Interpreter::<_, Script, NotSupportedEcal>::with_memory_storage(),
                );
-                let ready_tx = tx.clone().test_into_ready();
-                black_box(vm.init_script(ready_tx))
-                    .expect("Should be able to execute transaction");
+
+                // Initialize the VM and require the allocation of the whole memory
+                // to charge for the worst possible case.
+                black_box(initialize_vm(black_box(tx.clone()), vm));


This is even more confusing from before. I'm not sure where the extra black_box came from either.

Can we just have a method that says what we want, and then hide the implementation behind it? We probably can get rid of the comment in that case too.

unoptimized_and_fully_saturated_vm or something like that. Then have the internal code create the vm and wrap the vm and the tx in black_box?

…feature/benchmarks-0.25.2

@MitchTurner

## Version v0.26.0 ### Fixed #### Breaking - [#1868](#1868): Include the `event_inbox_root` in the header hash. Changed types of the `transactions_count` to `u16` and `message_receipt_count` to `u32` instead of `u64`. Updated the application hash root calculation to not pad numbers. - [#1866](#1866): Fixed a runtime panic that occurred when restarting a node. The panic happens when the relayer database is already populated, and the relayer attempts an empty commit during start up. This invalid commit is removed in this PR. - [#1871](#1871): Fixed `block` endpoint to return fetch the blocks from both databases after regenesis. - [#1856](#1856): Replaced instances of `Union` with `Enum` for GraphQL definitions of `ConsensusParametersVersion` and related types. This is needed because `Union` does not support multiple `Version`s inside discriminants or empty variants. - [#1870](#1870): Fixed benchmarks for the `0.25.3`. - [#1870](#1870): Improves the performance of getting the size of the contract from the `InMemoryTransaction`. - [#1851](#1851): Provided migration capabilities (enabled addition of new column families) to RocksDB instance. ### Added - [#1853](#1853): Added a test case to verify the database's behavior when new columns are added to the RocksDB database. - [#1860](#1860): Regenesis now preserves `FuelBlockIdsToHeights` off-chain table. ### Changed - [#1847](#1847): Simplify the validation interface to use `Block`. Remove `Validation` variant of `ExecutionKind`. - [#1832](#1832): Snapshot generation can be cancelled. Progress is also reported. - [#1837](#1837): Refactor the executor and separate validation from the other use cases ## What's Changed * Weekly `cargo update` by @github-actions in #1850 * Refactor/separate validation from other executions by @MitchTurner in #1837 * fix: Use `Enum` for `ConsensusParametersVersion` and related types by @bvrooman in #1856 * feat: snapshot generation graceful shutdown by @segfault-magnet in #1832 * regenesis: migrate FuelBlockIdsToHeights by @Dentosal in #1860 * Weekly `cargo update` by @github-actions in #1869 * Refactor/Simplify validation logic by @MitchTurner in #1847 * Fixed `block` endpoint to return fetch the blocks from both databases after regenesis by @xgreenx in #1871 * Add Eq and Partial Eq to tx response and status by @MujkicA in #1872 * test: restart with relayer data by @bvrooman in #1866 * Fix `BlockHeader` hash by @MitchTurner in #1868 * Added a test for the case of adding new columns into the existing RocksDB database by @xgreenx in #1853 * Fixed benchmarks for the `0.25.3` by @xgreenx in #1870 **Full Changelog**: v0.25.3...v0.26.0

xgreenx and others added 24 commits April 20, 2024 02:11

Fixed benchmarks to follow new memory rule

2852f7a

WIP: refactor commit_changes_with_height_update

57a9619

Make it actually work

2a503bd

Merge branch 'master' into dento/regenesis-migrate-FuelBlockIdsToHeights

3e24828

Revert changes to commit

46d6c8a

Metadata using RwLock

ba36f01

Add genesis flag to db handler

562bca3

More docs

336378a

Add changelog entry

9c0fd52

Merge branch 'master' into dento/regenesis-migrate-FuelBlockIdsToHeights

1c91490

Cleanup

d50083e

Simplify locking

8c861e0

Merge branch 'master' into feature/benchmarks-0.25.2

ee3c832

Fixed csiz benchmark and updated VM initialization benchmark

8e91767

Fixed something

d8fd3dd

Temporary fixed benchmarks

7fcd31d

Use UncheckedDatabase type for genesis imports

df13a4a

Restore TransactableStorage::commit height parameter

451f8be

Make Database use UncheckedDatabase internally

28de830

Restore height fetch on db creation

8ba1fc2

Used stages to reuse most of the functionality

97d681c

Merge branch 'dento/regenesis-migrate-FuelBlockIdsToHeights' into fea…

2136866

…ture/benchmarks-0.25.2 # Conflicts: # benches/benches/vm_set/blockchain.rs # benches/src/lib.rs

Fixed benchmarks with a proper fix from #1860

2bf802f

Merge branch 'master' into feature/benchmarks-0.25.2

56859ad

# Conflicts: # benches/benches/vm_set/blockchain.rs # benches/src/lib.rs

xgreenx requested a review from a team April 29, 2024 11:03

xgreenx self-assigned this Apr 29, 2024

xgreenx and others added 3 commits April 29, 2024 13:28

Reset some unnecessary changes

f7946bc

Merge branch 'master' into feature/benchmarks-0.25.2

9978427

Make CI happy

59fc1a1

xgreenx marked this pull request as ready for review April 29, 2024 11:42

xgreenx and others added 4 commits April 29, 2024 18:48

Merge branch 'master' into feature/benchmarks-0.25.2

d9c2d8f

Merge branch 'master' into feature/benchmarks-0.25.2

7738ffa

Merge branch 'master' into feature/benchmarks-0.25.2

298685d

# Conflicts: # bin/fuel-core/chainspec/dev-testnet/state_transition_bytecode.wasm # bin/fuel-core/chainspec/testnet/state_transition_bytecode.wasm

Applied master

43756c4

bvrooman previously approved these changes Apr 29, 2024

View reviewed changes

Update CHANGELOG.md

ba11d13

Co-authored-by: Brandon Vrooman <[email protected]>

xgreenx dismissed bvrooman’s stale review via ba11d13 April 29, 2024 21:05

xgreenx requested review from bvrooman and a team April 29, 2024 21:05

bvrooman previously approved these changes Apr 29, 2024

View reviewed changes

MitchTurner reviewed Apr 29, 2024

View reviewed changes

xgreenx added 2 commits April 30, 2024 08:26

Improved redeability of the vm initialization

b9eb596

Merge remote-tracking branch 'origin/feature/benchmarks-0.25.2' into …

6397e25

…feature/benchmarks-0.25.2

xgreenx dismissed bvrooman’s stale review via 6397e25 April 30, 2024 06:26

Merge branch 'master' into feature/benchmarks-0.25.2

9891916

xgreenx requested review from MitchTurner, bvrooman and a team April 30, 2024 06:28

MitchTurner reviewed Apr 30, 2024

View reviewed changes

xgreenx added 2 commits April 30, 2024 09:22

Hope

31fdbb0

Merge remote-tracking branch 'origin/feature/benchmarks-0.25.2' into …

7be5108

…feature/benchmarks-0.25.2

xgreenx requested review from MitchTurner and a team April 30, 2024 07:23

MitchTurner approved these changes Apr 30, 2024

View reviewed changes

xgreenx enabled auto-merge (squash) April 30, 2024 07:26

xgreenx merged commit f12ee07 into master Apr 30, 2024
30 checks passed

xgreenx deleted the feature/benchmarks-0.25.2 branch April 30, 2024 07:33

xgreenx mentioned this pull request Apr 30, 2024

Release v0.26.0 #1874

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fixed benchmarks for the `0.25.3` #1870

Fixed benchmarks for the `0.25.3` #1870

xgreenx commented Apr 29, 2024 •

edited

Loading

bvrooman left a comment

bvrooman Apr 29, 2024

xgreenx Apr 29, 2024 •

edited

Loading

bvrooman Apr 29, 2024

MitchTurner Apr 29, 2024

MitchTurner Apr 29, 2024

MitchTurner Apr 29, 2024

xgreenx Apr 30, 2024

MitchTurner Apr 30, 2024

Fixed benchmarks for the 0.25.3 #1870

Fixed benchmarks for the 0.25.3 #1870

Conversation

xgreenx commented Apr 29, 2024 • edited Loading

Before requesting review

bvrooman left a comment

Choose a reason for hiding this comment

bvrooman Apr 29, 2024

Choose a reason for hiding this comment

xgreenx Apr 29, 2024 • edited Loading

Choose a reason for hiding this comment

bvrooman Apr 29, 2024

Choose a reason for hiding this comment

MitchTurner Apr 29, 2024

Choose a reason for hiding this comment

MitchTurner Apr 29, 2024

Choose a reason for hiding this comment

MitchTurner Apr 29, 2024

Choose a reason for hiding this comment

xgreenx Apr 30, 2024

Choose a reason for hiding this comment

MitchTurner Apr 30, 2024

Choose a reason for hiding this comment

Fixed benchmarks for the `0.25.3` #1870

Fixed benchmarks for the `0.25.3` #1870

xgreenx commented Apr 29, 2024 •

edited

Loading

xgreenx Apr 29, 2024 •

edited

Loading