fix(db_lookup_times): rework core logic of benchmark #2159

rymnc · 2024-09-04T01:07:05Z

Note

This PR follows up #2142 with comments after merge

Linked Issues/PRs

Description

refactors the test database to use a custom set of columns that will not create any new tables/columns in fuel-core
refactors the functions to return Results instead of unwrapping everywhere
automated cleaning up of benchmark databases
KeyValueMutate impl of RocksDb is now behind the test-helpers feature flag
moved the db_lookup_times_utils to the src directory in the benches crate so we can test our insertion/fetching logic

Checklist

Breaking changes are clearly marked as such in the PR description and changelog
New behavior is reflected in tests
The specification matches the implemented behavior (link update PR if changes are needed)

Before requesting review

I have reviewed the code myself
I have created follow-up issues caused by this PR and linked them here

After merging, notify other teams

[Add or remove entries as needed]

Rust SDK
Sway compiler
Platform documentation (for out-of-organization contributors, the person merging the PR will do this)
Someone else?

xgreenx · 2024-09-05T07:56:11Z

benches/benches/db_lookup_times.rs

+    let multi_get_cleaner = multi_get_lookup(c).unwrap();
+    let full_block_cleaner = full_block_lookup(c).unwrap();
+
+    if should_clean() {


If you interrupt the benchmark or it fails with panic, it will leave the trash on the disk.

It would be nice if you used ShallowTempDir to be sure that it will be cleaned up=)

given that we open rocksdb to seed, close and then re-open for benchmarking, we'd have to ensure that the ShallowTempDir stays alive through it.
refactored to conditonally drop in 7aaacf1

nevermind ~ as per our discussion, #2168 handles it. This PR is blocked upon merge.

unblocked, rebased and addressed in 13b9a71

xgreenx · 2024-09-05T08:02:50Z

benches/src/db_lookup_times_utils/seed.rs

+        .iter()
+        .map(|tx| -> DbLookupBenchResult<(Bytes32, Vec<u8>)> {
+            let tx_id = tx.id(&chain_id());
+            let raw_tx = postcard::to_allocvec(tx)?.to_vec();


Since you already defined your database, I think you can just define 3 tables as well and use storage traits to serialize and deserialize values. Plus, you can create InMemoryTransaction and push changed at once. It should speed up seeding of the database

created issue for follow up PR: #2169

## Linked Issues/PRs  - #2159 (comment) ## Description  Trying to split up #2159 into smaller PRs, here we move `ShallowTempDir` which was only used in benchmarks to the benches crate, where it is more appropriate. Additionally a follow up PR will be created to make it conditionally drop databases/directories based on an env var. ## Checklist - [x] Breaking changes are clearly marked as such in the PR description and changelog - [x] New behavior is reflected in tests - [x] [The specification](https://github.com/FuelLabs/fuel-specs/) matches the implemented behavior (link update PR if changes are needed) ### Before requesting review - [x] ] I have reviewed the code myself - [x] I have created follow-up issues caused by this PR and linked them here ### After merging, notify other teams [Add or remove entries as needed] - [ ] [Rust SDK](https://github.com/FuelLabs/fuels-rs/) - [ ] [Sway compiler](https://github.com/FuelLabs/sway/) - [ ] [Platform documentation](https://github.com/FuelLabs/devrel-requests/issues/new?assignees=&labels=new+request&projects=&template=NEW-REQUEST.yml&title=%5BRequest%5D%3A+) (for out-of-organization contributors, the person merging the PR will do this) - [ ] Someone else?

xgreenx

LGTM=)

xgreenx · 2024-09-06T06:40:11Z

benches/src/db_lookup_times_utils/utils.rs

+    BlockHeight::from(rng.gen_range(0..block_count.into()))
+}
+
+pub struct ConditionalTempDir {


Would be nice to use ShallowTmpDir instead=)

yup, I'm rebasing rn

addressed in 13b9a71

xgreenx · 2024-09-06T06:42:11Z

benches/src/db_lookup_times_utils/matrix.rs

+pub const BLOCK_COUNT_MATRIX: [u32; 1] = [4];
+pub const TX_COUNT_MATRIX: [u32; 1] = [1];


Maybe we want to have bigger values here=)

addressed in 4202d9e

xgreenx · 2024-09-06T06:42:43Z

benches/src/db_lookup_times_utils/full_block_table.rs

+    Transactions = 6,
+    /// See [`FuelBlocks`](crate::tables::FuelBlocks)
+    FuelBlocks = 7,
+    FullFuelBlocks = 10902,
+    Metadata = 10903,


Maybe 0, 1, 2, 3 will be simpler=D

addressed in 73cdc4d

netrome

A few nits and suggestions, but nothing blocking.

benches/src/db_lookup_times_utils/utils.rs

@rymnc

## Version v0.36.0 ### Added - [2135](#2135): Added metrics logging for number of blocks served over the p2p req/res protocol. - [2151](#2151): Added limitations on gas used during dry_run in API. - [2188](#2188): Added the new variant `V2` for the `ConsensusParameters` which contains the new `block_transaction_size_limit` parameter. - [2163](#2163): Added runnable task for fetching block committer data. - [2204](#2204): Added `dnsaddr` resolution for TLD without suffixes. ### Changed #### Breaking - [2199](#2199): Applying several breaking changes to the WASM interface from backlog: - Get the module to execute WASM byte code from the storage first, an fallback to the built-in version in the case of the `FUEL_ALWAYS_USE_WASM`. - Added `host_v1` with a new `peek_next_txs_size` method, that accepts `tx_number_limit` and `size_limit`. - Added new variant of the return type to pass the validation result. It removes block serialization and deserialization and should improve performance. - Added a V1 execution result type that uses `JSONError` instead of postcard serialized error. It adds flexibility of how variants of the error can be managed. More information about it in FuelLabs/fuel-vm#797. The change also moves `TooManyOutputs` error to the top. It shows that `JSONError` works as expected. - [2145](#2145): feat: Introduce time port in PoA service. - [2155](#2155): Added trait declaration for block committer data - [2142](#2142): Added benchmarks for varied forms of db lookups to assist in optimizations. - [2158](#2158): Log the public address of the signing key, if it is specified - [2188](#2188): Upgraded the `fuel-vm` to `0.57.0`. More information in the [release](https://github.com/FuelLabs/fuel-vm/releases/tag/v0.57.0). ## What's Changed * chore(p2p_service): add metrics for number of blocks requested over p2p req/res protocol by @rymnc in #2135 * Weekly `cargo update` by @github-actions in #2149 * Debug V1 algorightm and use more realistic values in gas price analysis by @MitchTurner in #2129 * feat(gas_price_service): include trait declaration for block committer data by @rymnc in #2155 * Convert gas price analysis tool to CLI by @MitchTurner in #2156 * chore: add benchmarks for varied forms of lookups by @rymnc in #2142 * Add label nochangelog on weekly cargo update by @AurelienFT in #2152 * Log consensus-key signer address if specified by @acerone85 in #2158 * chore(rocks_db): move ShallowTempDir to benches crate by @rymnc in #2168 * chore(benches): conditional dropping of databases in benchmarks by @rymnc in #2170 * feat: Introduce time port in PoA service by @netrome in #2145 * Get DA costs from predefined data by @MitchTurner in #2157 * chore(shallow_temp_dir): panic if not panicking by @rymnc in #2172 * chore: Add initial CODEOWNERS file by @netrome in #2179 * Weekly `cargo update` by @github-actions in #2177 * fix(db_lookup_times): rework core logic of benchmark by @rymnc in #2159 * Add verification on transaction dry_run that they don't spend more than block gas limit by @AurelienFT in #2151 * bug: fix algorithm overflow issues by @MitchTurner in #2173 * feat(gas_price_service): create runnable task for expensive background polling for da metadata by @rymnc in #2163 * Weekly `cargo update` by @github-actions in #2197 * Fix bug with gas price factor in V1 algorithm by @MitchTurner in #2201 * Applying several breaking changes to the WASM interface from backlog by @xgreenx in #2199 * chore(p2p): dnsaddr recursive resolution by @rymnc in #2204 ## New Contributors * @acerone85 made their first contribution in #2158 **Full Changelog**: v0.35.0...v0.36.0

rymnc self-assigned this Sep 4, 2024

rymnc added the no changelog Skip the CI check of the changelog modification label Sep 4, 2024

rymnc mentioned this pull request Sep 4, 2024

chore: add benchmarks for varied forms of lookups #2142

Merged

9 tasks

rymnc force-pushed the fix/db-benchmarks-pmr branch 5 times, most recently from 791c99d to 59aceec Compare September 4, 2024 12:29

rymnc marked this pull request as ready for review September 5, 2024 06:52

rymnc requested a review from xgreenx September 5, 2024 06:52

xgreenx reviewed Sep 5, 2024

View reviewed changes

This was referenced Sep 5, 2024

chore(rocks_db): move ShallowTempDir to benches crate #2168

Merged

chore(db_lookup_times): Modify benchmarks to use TableBlueprint to avoid manual serde #2169

Open

xgreenx reviewed Sep 6, 2024

View reviewed changes

rymnc force-pushed the fix/db-benchmarks-pmr branch from 7aaacf1 to d09740c Compare September 6, 2024 07:10

rymnc requested review from xgreenx and a team September 6, 2024 07:12

rymnc added 12 commits September 6, 2024 13:43

fix: remove blueprint definition for FullFuelBlocks

08f5b40

stage: 1

f8f017c

stage: 2

3777ebf

stage: 3

51ccf67

stage: 4

6fa4488

fix: lint

4f900a1

fix: test helper

1198846

fix: clippy

d8209d1

chore: cut scope

1a9cf44

fix: unused imports under test-helpers flag

197ab91

fix: generic verbose name

6ac0ea4

fix: env var name

4feac27

rymnc added 7 commits September 6, 2024 13:43

chore: move utils to src so we can test them

aaf1e96

fix: minor dep, perf changes

7b8f7a8

fix: conditional drop, safe on panic unwinds

a05bf9a

fix: use ShallowTempDir in tests and benches

13b9a71

fix: matrix

4202d9e

fix: column indices

73cdc4d

fix: remove tempfile dep

7a5555b

rymnc force-pushed the fix/db-benchmarks-pmr branch from 0109564 to 7a5555b Compare September 6, 2024 08:13

Merge branch 'master' into fix/db-benchmarks-pmr

daf1b6e

xgreenx requested review from Dentosal and MitchTurner as code owners September 9, 2024 10:18

xgreenx previously approved these changes Sep 9, 2024

View reviewed changes

netrome previously approved these changes Sep 9, 2024

View reviewed changes

benches/src/db_lookup_times_utils/utils.rs Outdated Show resolved Hide resolved

benches/src/db_lookup_times_utils/utils.rs Outdated Show resolved Hide resolved

benches/src/db_lookup_times_utils/utils.rs Outdated Show resolved Hide resolved

rymnc added 3 commits September 9, 2024 17:50

fix: remove chain id import

875c91b

fix: shallow temp dir explicit usage

3f93854

fix: anyhow error

a73f839

rymnc dismissed stale reviews from netrome and xgreenx via a73f839 September 9, 2024 12:25

rymnc requested review from xgreenx and netrome September 9, 2024 12:25

xgreenx previously approved these changes Sep 9, 2024

View reviewed changes

fix: imports

3c8a9cb

rymnc dismissed xgreenx’s stale review via 3c8a9cb September 9, 2024 12:33

xgreenx approved these changes Sep 9, 2024

View reviewed changes

netrome approved these changes Sep 9, 2024

View reviewed changes

rymnc enabled auto-merge (squash) September 9, 2024 13:02

rymnc merged commit 877b52b into master Sep 9, 2024
35 checks passed

rymnc deleted the fix/db-benchmarks-pmr branch September 9, 2024 13:10

xgreenx mentioned this pull request Sep 17, 2024

Release v0.36.0 #2213

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix(db_lookup_times): rework core logic of benchmark #2159

fix(db_lookup_times): rework core logic of benchmark #2159

rymnc commented Sep 4, 2024 •

edited

Loading

xgreenx Sep 5, 2024

rymnc Sep 5, 2024

rymnc Sep 5, 2024

rymnc Sep 6, 2024 •

edited

Loading

xgreenx Sep 5, 2024

rymnc Sep 5, 2024

xgreenx left a comment

xgreenx Sep 6, 2024

rymnc Sep 6, 2024

rymnc Sep 6, 2024 •

edited

Loading

xgreenx Sep 6, 2024

rymnc Sep 6, 2024 •

edited

Loading

xgreenx Sep 6, 2024

rymnc Sep 6, 2024 •

edited

Loading

netrome left a comment

		pub const BLOCK_COUNT_MATRIX: [u32; 1] = [4];
		pub const TX_COUNT_MATRIX: [u32; 1] = [1];

fix(db_lookup_times): rework core logic of benchmark #2159

fix(db_lookup_times): rework core logic of benchmark #2159

Conversation

rymnc commented Sep 4, 2024 • edited Loading

Linked Issues/PRs

Description

Checklist

Before requesting review

After merging, notify other teams

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rymnc Sep 6, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

xgreenx left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rymnc Sep 6, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rymnc Sep 6, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rymnc Sep 6, 2024 • edited Loading

Choose a reason for hiding this comment

netrome left a comment

Choose a reason for hiding this comment

rymnc commented Sep 4, 2024 •

edited

Loading

rymnc Sep 6, 2024 •

edited

Loading

rymnc Sep 6, 2024 •

edited

Loading

rymnc Sep 6, 2024 •

edited

Loading

rymnc Sep 6, 2024 •

edited

Loading