Skip to content

bench: Add reindex_tx_graph benchmark#1968

Merged
ValuedMammal merged 3 commits intobitcoindevkit:masterfrom
ValuedMammal:bench/reindex-tx-graph
Jul 3, 2025
Merged

bench: Add reindex_tx_graph benchmark#1968
ValuedMammal merged 3 commits intobitcoindevkit:masterfrom
ValuedMammal:bench/reindex-tx-graph

Conversation

@ValuedMammal
Copy link
Copy Markdown
Collaborator

@ValuedMammal ValuedMammal commented May 26, 2025

This PR contains the following changes:

  • Add benchmark reindex_tx_graph. Run with cargo bench -p bdk_chain --bench indexer.
  • Add unit test to keychain_txout module to test behavior of spk-cache.
  • Fixup a few doc comments.

Checklists

All Submissions:

  • I've signed all my commits
  • I followed the contribution guidelines
  • I ran cargo +nightly fmt and cargo clippy before committing

@ValuedMammal
Copy link
Copy Markdown
Collaborator Author

This is what I found compared to the same test (but no cache) on master:

	 Running benches/indexer.rs (target/release/deps/index-d7b15ff0fa7d9bb0)
reindex_tx_graph        time:   [649.59 us 656.47 us 667.48 us]                             
                        change: [-99.043% -99.038% -99.032%] (p = 0.00 < 0.05)
                        Performance has improved.

@notmandatory notmandatory moved this to In Progress in BDK Chain May 28, 2025
@ValuedMammal ValuedMammal force-pushed the bench/reindex-tx-graph branch from ea64aef to bb6d7c1 Compare May 30, 2025 17:48
@ValuedMammal ValuedMammal marked this pull request as ready for review May 30, 2025 17:48
@ValuedMammal ValuedMammal moved this from In Progress to Needs Review in BDK Chain May 30, 2025
Copy link
Copy Markdown
Collaborator

@oleonardolima oleonardolima left a comment

Choose a reason for hiding this comment

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

tACK bb6d7c1

Thanks for the documentation fixup, got similar results when running locally:

Gnuplot not found, disabling plotting
reindex_tx_graph        time:   [320.04 us 320.93 us 322.03 us]
                        change: [-99.087% -99.084% -99.081%] (p = 0.00 < 0.05)
                        Performance has improved.
Found 9 outliers among 100 measurements (9.00%)
  4 (4.00%) high mild
  5 (5.00%) high severe

Gnuplot not found, disabling plotting

Copy link
Copy Markdown
Contributor

@LagginTimes LagginTimes left a comment

Choose a reason for hiding this comment

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

Looking good! I get the same benchmark results:

reindex_tx_graph        time:   [300.23 us 301.44 us 302.84 us]                             
                        change: [-99.286% -99.278% -99.268%] (p = 0.00 < 0.05)
                        Performance has improved.
Found 16 outliers among 100 measurements (16.00%)
  7 (7.00%) high mild
  9 (9.00%) high severe

Just one minor suggestion:

assert_eq!(did, desc.descriptor_id());
for (&i, cached_spk) in cached_spks {
// Cached spk matches derived
assert_eq!(exp_spks.get(&i), Some(cached_spk));
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Suggested change
assert_eq!(exp_spks.get(&i), Some(cached_spk));
let exp_spk = desc.at_derivation_index(i).unwrap().script_pubkey();
assert_eq!(&exp_spk, cached_spk);

Might be cleaner to remove the exp_spks on line 1138 entirely and do a direct lookup inside the loop.

@ValuedMammal ValuedMammal moved this from Needs Review to In Progress in BDK Chain Jun 13, 2025
@ValuedMammal ValuedMammal force-pushed the bench/reindex-tx-graph branch from bb6d7c1 to cb55c51 Compare June 13, 2025 16:24
@ValuedMammal
Copy link
Copy Markdown
Collaborator Author

Thanks @LagginTimes

@ValuedMammal ValuedMammal moved this from In Progress to Needs Review in BDK Chain Jun 13, 2025
Copy link
Copy Markdown
Contributor

@LagginTimes LagginTimes left a comment

Choose a reason for hiding this comment

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

ACK cb55c51

- Test cached spk matches derived
- Recover state of spk cache from changeset
Fixed parameter names to match the function or struct definition,
and correct some spelling mistakes.
@ValuedMammal ValuedMammal force-pushed the bench/reindex-tx-graph branch from cb55c51 to f51f5b5 Compare July 1, 2025 16:09
@ValuedMammal
Copy link
Copy Markdown
Collaborator Author

Rebased this on #1978.

ACK f51f5b5

@ValuedMammal ValuedMammal merged commit 5050537 into bitcoindevkit:master Jul 3, 2025
19 checks passed
@github-project-automation github-project-automation Bot moved this from Needs Review to Done in BDK Chain Jul 3, 2025
@ValuedMammal ValuedMammal deleted the bench/reindex-tx-graph branch July 3, 2025 14:25
@oleonardolima oleonardolima mentioned this pull request Jul 31, 2025
16 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

Archived in project

Development

Successfully merging this pull request may close these issues.

4 participants