perf(stages): Adds benchmark to `TransactionLookupStage` #1130

joshieDo · 2023-02-02T08:56:50Z

Tries to tackle #1092

Added one benchmark, but to be honest the difference I'm seeing seems too small ( 2 - 4 %). I was expecting a bigger difference (for 20k blocks) and I'm not sure if it can't be explained by my machine instability.

One way to test the stage before (tx.put) and after (pre-sort + crsr.append):

$ git checkout dec44093c679d650be5fce04e23f22855b99b357
$ cargo bench --package reth-stages --bench criterion --features test-utils
$ git checkout joshie/txlookup
$ cargo bench --package reth-stages --bench criterion --features test-utils

To test insertion of tables with hash keys :

$ cargo bench --package reth-db --bench hash_keys

Note: sorting is benchmarked on insert_sorted and put_sorted.

crates/stages/src/stages/tx_lookup.rs

rakita · 2023-02-02T09:41:27Z

crates/stages/src/stages/tx_lookup.rs

+        let last = cursor_txhash.last()?;
+        let mut append = last.is_none();
+        if let (false, Some((last_txhash, _))) = (tx_list.is_empty(), last) {
+            append = last_txhash < tx_list[0].0;


We are comparing it with the first in the list, should tx_list be sorted at this pont?

gakonst · 2023-02-03T01:03:56Z

crates/stages/src/test_utils/test_db.rs

        self.query(|tx| {
            let last = tx.cursor_read::<T>()?.last()?;
            Ok(last.is_none())
        })
    }

    /// Return full table as Vec
-    pub(crate) fn table<T: Table>(&self) -> Result<Vec<(T::Key, T::Value)>, DbError>
+    pub fn table<T: Table>(&self) -> Result<Vec<(T::Key, T::Value)>, DbError>


let's clippy allow this

gakonst · 2023-02-03T06:26:33Z

crates/stages/benches/criterion.rs

+    let tx = prepare_blocks(NUM_BLOCKS).unwrap();
+
+    measure_txlookup_stage(&mut group, tx);
+}


i would add manual implementations of various access patterns in a loop, in addition to testing the stage itself. might be easier to compare specific parts if e.g. you have a bench that does X puts vs sorted w/ cursor etc.

For append we know it is fastest if table is empty, question is what is better if we already have some values with random keys in the table and want to insert an additional batch?

yeah i should add those scenarios, however I'll probably put those elsewhere since they're more db specific? this seems like it should be only for stage benchmarking? (eg. reading from different tables and aggregating into one; mem vs disk read -> write, etc)

Added it on 100cea. There is a clear difference between presorted insert and unsorted insert.

cargo bench --package reth-db --bench hash_keys

Results

However, it's not that "visible" in the stage itself. I think because we're allocating all the data into a list before sorting+inserting it. Whereas, in the current state, we're just passing the value into put. But will need to run some more benches I guess

gakonst · 2023-02-05T00:49:14Z

Pulled the parts about stage benchmarking here: #1171. Let's keep investigating the DB-specific patterns here.

gakonst · 2023-02-06T01:26:49Z

TransactionLookup bench on main:

Stages/TransactionLookup
                        time:   [1.2050 s 1.2212 s 1.2390 s]
                        change: [+141.37% +145.11% +149.12%] (p = 0.00 < 0.05)
                        Performance has regressed.

And this PR:

Stages/TransactionLookup
                        time:   [492.02 ms 499.36 ms 507.76 ms]
                        change: [-59.921% -59.109% -58.261%] (p = 0.00 < 0.05)
                        Performance has improved.

Pretty significant improvement! Nice. What else do we need to review this?

However, it's not that "visible" in the stage itself. I think because we're allocating all the data into a list before sorting+inserting it. Whereas, in the current state, we're just passing the value into put. But will need to run some more benches I guess

^Is this still relevant? @joshieDo

joshieDo · 2023-02-06T07:42:33Z

Just leaving here some results and thoughts for inserting into a table with keys as hashes.
b6cf12c

Notes:

preload: preloads the database with X rows.
Each size category uses the same dataset for insert_*, put_* and append_*.
append_all should be used only to compare table sizes.

Benchmarks

10_000 rows
100_000 rows
1_000_000 rows

Thoughts

append improves speed (append_size) and table size (append_all). So if a node has been up for some weeks/months, it can be beneficial to copy the data to disk, delete the table and append from disk again, I guess.
insert_sorted is faster than put_sorted but table size is the same.
insert_sorted is faster than insert_unsorted but for some reason the table size is bigger.
The bigger the number of rows, the more noticeable the difference is. (before I was just testing small sizes...)

The table size being bigger for insert_sorted is the more curious aspect.

onbjerg

smol nits

onbjerg · 2023-02-06T07:51:17Z

crates/stages/src/stages/tx_lookup.rs

-        let mut tx_cursor = tx.cursor_write::<tables::Transactions>()?;
+        let mut cursor_tx = tx.cursor_write::<tables::Transactions>()?;


we use <type>_cursor everywhere else

onbjerg · 2023-02-06T07:52:29Z

crates/stages/src/stages/tx_lookup.rs

+        // Sort before inserting the reverse lookup for hash -> tx_id.
+        tx_list.sort_by(|txa, txb| txa.0.cmp(&txb.0));
+
+        let mut cursor_txhash = tx.cursor_write::<tables::TxHashNumber>()?;


Suggested change

let mut cursor_txhash = tx.cursor_write::<tables::TxHashNumber>()?;

let mut txhash_cursor = tx.cursor_write::<tables::TxHashNumber>()?;

crates/stages/src/stages/tx_lookup.rs

onbjerg · 2023-02-06T07:55:46Z

crates/stages/src/stages/tx_lookup.rs

+        let mut append = last.is_none();
+        if let (false, Some((last_txhash, _))) = (tx_list.is_empty(), last) {
+            append = last_txhash < tx_list[0].0;


Suggested change

let mut append = last.is_none();

if let (false, Some((last_txhash, _))) = (tx_list.is_empty(), last) {

append = last_txhash < tx_list[0].0;

let append = tx_list.first().map(|(first_txhash)| last_txhash < first_txhash).unwrap_or_default();

Co-authored-by: Bjerg <[email protected]>

joshieDo · 2023-02-08T03:34:12Z

Opening for review. The issue of the insert_sorted and insert_unsorted wrt table size can be investigated separately

codecov-commenter · 2023-02-08T04:03:16Z

Codecov Report

Merging #1130 (c6a77d4) into main (ba70b3b) will increase coverage by 0.26%.
The diff coverage is 79.31%.

📣 This organization is not using Codecov’s GitHub App Integration. We recommend you install it so Codecov can continue to function properly for your repositories. Learn more

@@            Coverage Diff             @@
##             main    #1130      +/-   ##
==========================================
+ Coverage   75.29%   75.56%   +0.26%     
==========================================
  Files         335      339       +4     
  Lines       36432    37707    +1275     
==========================================
+ Hits        27433    28492    +1059     
- Misses       8999     9215     +216

Flag	Coverage Δ
unit-tests	`75.56% <79.31%> (+0.26%)`	⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files	Coverage Δ
bin/reth/src/db/mod.rs	`0.00% <0.00%> (ø)`
crates/primitives/src/header.rs	`94.15% <66.66%> (-0.35%)`	⬇️
crates/stages/src/stages/tx_lookup.rs	`92.50% <92.85%> (+0.02%)`	⬆️
crates/primitives/src/block.rs	`92.59% <100.00%> (+0.28%)`	⬆️
crates/primitives/src/hardfork.rs	`25.00% <0.00%> (-11.96%)`	⬇️
crates/stages/src/stages/total_difficulty.rs	`84.48% <0.00%> (-10.79%)`	⬇️
crates/net/dns/src/config.rs	`90.90% <0.00%> (-9.10%)`	⬇️
crates/storage/provider/src/test_utils/noop.rs	`8.88% <0.00%> (-5.93%)`	⬇️
crates/primitives/src/genesis.rs	`81.25% <0.00%> (-4.00%)`	⬇️
crates/rpc/rpc-builder/src/lib.rs	`28.29% <0.00%> (-3.35%)`	⬇️
... and 76 more

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

joshieDo added 8 commits February 1, 2023 06:43

add arbitrary to sealedblock and sealedheader

c7bafce

put stages::test_utils behind a feature flag instead

3781c48

add txlookup stage benchmark wip

30557c2

add unwind to bench setup

969fbe2

use bincode instead for block vectors

78e679d

add Clone to TxLookupStage

dec4409

append if possible on TransactionLookupStage

1bf00c5

add README.md

ac5ecc8

joshieDo added C-enhancement New feature or request A-staged-sync Related to staged sync (pipelines and stages) labels Feb 2, 2023

Merge remote-tracking branch 'origin/main' into joshie/txlookup

98b95c6

joshieDo added C-perf A change motivated by improving speed, memory usage or disk footprint and removed C-enhancement New feature or request labels Feb 2, 2023

rakita reviewed Feb 2, 2023

View reviewed changes

crates/stages/src/stages/tx_lookup.rs Outdated Show resolved Hide resolved

rakita reviewed Feb 2, 2023

View reviewed changes

gakonst reviewed Feb 3, 2023

View reviewed changes

jinsankim mentioned this pull request Feb 3, 2023

test(db): cursor write operations are working properly wherever cursor is #1161

Merged

joshieDo and others added 8 commits February 4, 2023 06:39

sort first

547d7d7

Merge remote-tracking branch 'origin/main' into joshie/txlookup

186d4bd

add db hash_keys benchmark

0100cea

clippy complexity

e73b459

perf: reuse secp256k1 context during sig recovery

b684a5e

feat: allow instantiating TestTransaction from path

eff66a5

bench: refactor and include sender recovery

e973fbc

Merge branch 'main' into joshie/txlookup

ae897b2

gakonst marked this pull request as ready for review February 5, 2023 00:24

gakonst requested review from onbjerg and rkrasiuk as code owners February 5, 2023 00:24

gakonst mentioned this pull request Feb 5, 2023

perf(SendersRecovery): re-use Secp256K1 context for >2x speedup and add benches #1171

Merged

gakonst marked this pull request as draft February 5, 2023 00:48

joshieDo added 5 commits February 5, 2023 07:59

Merge remote-tracking branch 'origin/main' into joshie/txlookup

17c446d

change comment location

2dc7259

use default warm up time for criterion

93cceba

add batches and table stats/sizes to hash_keys benchmark

6a06a05

add description to benchmark

0c3dcdd

append_all and append_size

b6cf12c

onbjerg reviewed Feb 6, 2023

View reviewed changes

joshieDo and others added 4 commits February 8, 2023 02:48

manually impl arbitrary for SealedHeader

f922c5c

should append more idiomatic

229fd91

Update crates/stages/src/stages/tx_lookup.rs

87bfce3

Co-authored-by: Bjerg <[email protected]>

comment clippy

c6a77d4

joshieDo marked this pull request as ready for review February 8, 2023 03:32

joshieDo mentioned this pull request Feb 8, 2023

Use cursor<T>.insert instead of put<T> wherever possible #1217

Closed

gakonst approved these changes Feb 9, 2023

View reviewed changes

gakonst merged commit 7e68373 into main Feb 9, 2023

gakonst deleted the joshie/txlookup branch February 9, 2023 21:10

joshieDo mentioned this pull request Jun 15, 2023

perf: use hashmaps for non-sortable state #3181

Closed

joshieDo mentioned this pull request Mar 1, 2024

Tracking: Add ETL to HashingStages & HistoryStages #6909

Closed

2 tasks

joshieDo mentioned this pull request Mar 20, 2024

feat: add ETL to HistoryStages #7249

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

perf(stages): Adds benchmark to `TransactionLookupStage` #1130

perf(stages): Adds benchmark to `TransactionLookupStage` #1130

joshieDo commented Feb 2, 2023 •

edited

Loading

rakita Feb 2, 2023

gakonst Feb 3, 2023

gakonst Feb 3, 2023

rakita Feb 3, 2023

joshieDo Feb 4, 2023 •

edited

Loading

joshieDo Feb 4, 2023 •

edited

Loading

gakonst commented Feb 5, 2023

gakonst commented Feb 6, 2023 •

edited

Loading

joshieDo commented Feb 6, 2023 •

edited

Loading

onbjerg left a comment

onbjerg Feb 6, 2023

onbjerg Feb 6, 2023

onbjerg Feb 6, 2023

joshieDo commented Feb 8, 2023

codecov-commenter commented Feb 8, 2023

		let mut tx_cursor = tx.cursor_write::<tables::Transactions>()?;
		let mut cursor_tx = tx.cursor_write::<tables::Transactions>()?;

	let mut cursor_txhash = tx.cursor_write::<tables::TxHashNumber>()?;
	let mut txhash_cursor = tx.cursor_write::<tables::TxHashNumber>()?;

perf(stages): Adds benchmark to TransactionLookupStage #1130

perf(stages): Adds benchmark to TransactionLookupStage #1130

Conversation

joshieDo commented Feb 2, 2023 • edited Loading

rakita Feb 2, 2023

Choose a reason for hiding this comment

gakonst Feb 3, 2023

Choose a reason for hiding this comment

gakonst Feb 3, 2023

Choose a reason for hiding this comment

rakita Feb 3, 2023

Choose a reason for hiding this comment

joshieDo Feb 4, 2023 • edited Loading

Choose a reason for hiding this comment

joshieDo Feb 4, 2023 • edited Loading

Choose a reason for hiding this comment

gakonst commented Feb 5, 2023

gakonst commented Feb 6, 2023 • edited Loading

joshieDo commented Feb 6, 2023 • edited Loading

Notes:

Benchmarks

Thoughts

onbjerg left a comment

Choose a reason for hiding this comment

onbjerg Feb 6, 2023

Choose a reason for hiding this comment

onbjerg Feb 6, 2023

Choose a reason for hiding this comment

onbjerg Feb 6, 2023

Choose a reason for hiding this comment

joshieDo commented Feb 8, 2023

codecov-commenter commented Feb 8, 2023

Codecov Report

perf(stages): Adds benchmark to `TransactionLookupStage` #1130

perf(stages): Adds benchmark to `TransactionLookupStage` #1130

joshieDo commented Feb 2, 2023 •

edited

Loading

joshieDo Feb 4, 2023 •

edited

Loading

joshieDo Feb 4, 2023 •

edited

Loading

gakonst commented Feb 6, 2023 •

edited

Loading

joshieDo commented Feb 6, 2023 •

edited

Loading