Skip to content

Thv/benchmark wallet state update #587

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.

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

Merged
merged 2 commits into from
May 28, 2025

Conversation

Sword-Smith
Copy link
Member

Huge performance boost (2x) from the assert to debug_assert change. But I'm mostly interested in feedback on the benchmark, and how to handle state there.

@Sword-Smith Sword-Smith requested a review from dan-da May 12, 2025 17:41
@dan-da
Copy link
Collaborator

dan-da commented May 13, 2025

seems like a good speedup. :-)

I don't think the benchmarks.rs belongs in src/api as the code seems quite specific to that benchmark.

Also I think the benchmark can leverage the new api stuff to be more concise.

It's easier just to demonstrate though, so I've modified the wallet_state bench, and also made a few more things pub in neptune_core.

I will push a branch shortly with the suggested changes.

@Sword-Smith Sword-Smith force-pushed the thv/benchmark-wallet-state-update branch from 45e888d to e87e212 Compare May 13, 2025 07:18
@dan-da
Copy link
Collaborator

dan-da commented May 13, 2025

here is a branch with suggested changes. At minimum I don't think benchmark.rs belongs in src/api.

The rest of the changes demonstrate an alternate way to write the benchmark, based on 45e888d, and a few tweaks to neptune-core code to facilitate.

danda/benchmark-wallet-state-update (in this repo)

@Sword-Smith Sword-Smith force-pushed the thv/benchmark-wallet-state-update branch from e87e212 to abfb2d4 Compare May 14, 2025 15:08
Copy link
Collaborator

@dan-da dan-da left a comment

Choose a reason for hiding this comment

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

Since I see more commits that do not yet address my previous comments, I will re-iterate the request to remove benchmarks.rs from src/api with some additional context/rationale, and make this a more official "request changes". rationale:

  1. I'm not convinced that benchmark related code belongs in src/api at all, even general purpose types. A case could be made for that, but so far has not been advanced.
  2. the contents seem tailored to the needs of wallet_state.rs benchmark, not general purpose.
  3. the content and location of benchmarks.rs is inconsistent with other types in src/api, ie not following convention. If we were to support a benchmarks api, it should be in its own sub-directory and probably should define a Benchmark type, and crate::api::Api should have a method for instantiating it.
  4. it violates the rules of src/api module, as specified in src/api/README-dev.md. In particular rules 3, 4, 6, and 8 which can be summarized as "no panics", "return real error types", "doc-comment all pub items", and "make integration test".

I can suggest a few alternative (and easier) approaches:

  1. move contents of benchmarks.rs into the wallet_state.rs benchmark. Then make the methods it calls from neptune_cash pub as necessary. Probably the benchmark is just highlighting things that should be pub anyway, eg for 3rd party wallet usage.
  2. move the contents of benchmarks.rs elsewhere under src.
  3. use the approach I demonstrated in branch: danda/benchmark-wallet-state-update

@Sword-Smith Sword-Smith force-pushed the thv/benchmark-wallet-state-update branch from abfb2d4 to f639d7c Compare May 14, 2025 16:07
@Sword-Smith
Copy link
Member Author

Since I see more commits that do not yet address my previous comments ...

Just rebasing, no new commits. The code from api/benchmarks is being moved.

@Sword-Smith Sword-Smith force-pushed the thv/benchmark-wallet-state-update branch from f639d7c to 5e778f6 Compare May 14, 2025 16:25
@Sword-Smith Sword-Smith requested a review from dan-da May 14, 2025 16:26
@Sword-Smith
Copy link
Member Author

There's a lot of code being made public with this PR. I'm not a fan of that since it means that we will not be warned about unused code.

@dan-da
Copy link
Collaborator

dan-da commented May 15, 2025

There's a lot of code being made public with this PR. I'm not a fan of that since it means that we will not be warned about unused code.

yeah... there are quite a few things that are presently pub(crate) in the codebase that will likely need to be made pub for the wallet competition. And this benchmark is probably finding some of those. But then it may be exposing others that really needn't be pub, except for the benchmark. So that's a tricky line to walk.

A few thoughts:

  1. @jan-ferdinand had suggested a way to mark a pub fn as not visible in docs somehow, so it is not very discoverable and kind of a hint not to use it. edit: it is #[doc(hidden)]

  2. consider the other approach I suggested: "move the contents of benchmarks.rs elsewhere under src". Then we need not make anything else pub. My concern was mainly with putting it in src/api, a carefully curated module. But perhaps it could live under src/util_types/benchmarks or something. It could also be marked with the attribute referenced in (1).

  3. The branch I created was an attempt to make the benchmark without need to expose many new APIs. It definitely has some warts, but shows it is mostly possible.

Personally I would lean towards (2) at this point, but I don't feel strongly about it.

@jan-ferdinand and @aszepieniec might have opinions to offer.


fn update_wallet_with_block2(bencher: Bencher) {
let rt = tokio::runtime::Runtime::new().unwrap();
let mut wallet_state = rt.block_on(devops_wallet_state_genesis(Network::Main));
Copy link
Collaborator

Choose a reason for hiding this comment

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

style: consider a single block_on() with a single async block.

(except for the bench closure which needs its own)

@jan-ferdinand
Copy link
Member

  • The annotation to hide an item from the public API is #[doc(hidden]. It is more than a hint: items marked this way are not part of the public API. Tools like cargo-semver-check will correctly pick this up.
  • It might be possible to use a crate like visibility to modify an item's visibility based on the compilation profile. I haven't tested it, but the annotation #[cfg_attr(bench, visibility::make(pub))] on a private item might achieve the desired effect.

@aszepieniec
Copy link
Contributor

It might be possible to use a crate like visibility to modify an item's visibility based on the compilation profile. I haven't tested it, but the annotation #[cfg_attr(bench, visibility::make(pub))] on a private item might achieve the desired effect.

OOooh, I like it!

@jan-ferdinand
Copy link
Member

I haven't tested it, but the annotation #[cfg_attr(bench, visibility::make(pub))] on a private item might achieve the desired effect.

Unfortunately, this doesn't work as easily. The bench attribute does not exist in this form. What you can do instead (and this time, I have tried it):

  • declare a new feature in the Cargo.toml, for example benchmarking = []
  • annotate the item in question with #[cfg_attr(feature = "benchmarking", visibility::make(pub))]
  • use the annotated item in a benchmark
  • run cargo bench --features benchmarking

The downside is that compilation will fail with a simple cargo bench, since the items in question will have the wrong visibility.

@Sword-Smith Sword-Smith force-pushed the thv/benchmark-wallet-state-update branch from 5e778f6 to 96151d4 Compare May 22, 2025 11:01
@Sword-Smith Sword-Smith requested a review from dan-da May 22, 2025 11:09
@Sword-Smith
Copy link
Member Author

I managed to better encapsulate the stuff needed for this benchmark. So now only two functions go from pub(crate) to pub. I'm very happy with where this has landed.

Also: This 2x speedup in wallet state update is something we need to roll out ASAP. State updates to the wallet are extremely slow, and by an order of magnitude the bottleneck for state updates, as evidenced by the below log showing a node with ~400 UTXOs applying a block state update.

2025-05-22T11:11:55.831666305Z DEBUG ThreadId(01) neptune_cash::models::state::wallet::wallet_state: Found valid mp for UTXO with leaf index: 64276
2025-05-22T11:11:55.831678729Z DEBUG ThreadId(01) neptune_cash::models::state::wallet::wallet_state: doing maintenance on 407/407 monitored UTXOs
2025-05-22T11:11:56.548856821Z DEBUG ThreadId(01) neptune_cash::models::state::wallet::wallet_state: Block has 0 removal records
2025-05-22T11:11:56.567658622Z DEBUG ThreadId(01) neptune_cash::models::state::wallet::wallet_state: Number of mutated membership proofs: 1
2025-05-22T11:11:57.429364122Z DEBUG ThreadId(01) neptune_cash::models::state: Flushed all databases
2025-05-22T11:11:57.429395871Z DEBUG ThreadId(01) neptune_cash: executed set_new_tip_internal() in 1.6940187899999999 secs.  location: /home/thv/repos/neptune-core/src/models/state/mod.rs:1409:9
2025-05-22T11:11:57.429418544Z DEBUG ThreadId(01) neptune_cash::main_loop: Storing block 08338635382424590278,11913466815894687866,17487415327791684952,09982352714201000328,00000000000000110312 in database. Height: 14865, Mined: 2025-05-22T11:21:54.067+02:0

@dan-da
Copy link
Collaborator

dan-da commented May 22, 2025

Also: This 2x speedup in wallet state update is something we need to roll out ASAP.

If urgent, then I would suggest backporting this specific feature onto a branch at v0.2.2, and releasing an update of that.

I think we are still some weeks away from being able to properly release something from master:

  1. there are still some open issues tagged as release-blocker that need resolving.
  2. once a release-candidate is tagged, we are targetting a two week testing period on testnet-mock (and anywhere else)
  3. we also need to write a basic automated test harness for the release-candidate running on testnet-mock. (can happen during the 2 week period)

When syncing on a node with many UTXOs, the wallet state update is **by
far** the slowest part of the state update. So having a benchmark for
this critical step is critical.

Adds a module `bench_helpers` to encapsulate code needed for benchmarks.
Since benchmarks are not part of the main crate, they can only use items
that are `pub`. So this `bench_helpers` module prevents us from marking
half the repo as `pub`, as all functions needed for benchmarks can be
neatly encapsulated here.
This small change gives a 2x speedup on the wallet-updating process,
as evidenced by the wallet-state update benchmark which.

The wallet state update is *by far* the bottleneck when doing state
updates with new blocks.

**Before this commit**
Timer precision: 20 ns
wallet_state                   fastest       │ slowest       │ median        │ mean          │ samples │ iters
╰─ maintain_membership_proofs                │               │               │               │         │
   ╰─ maintain_100_100                       │               │               │               │         │
      ╰─ apply_block2          172.2 ms      │ 184.5 ms      │ 175.6 ms      │ 176.2 ms      │ 10      │ 10

**After this commit**
Timer precision: 20 ns
wallet_state                   fastest       │ slowest       │ median        │ mean          │ samples │ iters
╰─ maintain_membership_proofs                │               │               │               │         │
   ╰─ maintain_100_100                       │               │               │               │         │
      ╰─ apply_block2          91.05 ms      │ 102.8 ms      │ 91.97 ms      │ 92.91 ms      │ 10      │ 10
@Sword-Smith Sword-Smith force-pushed the thv/benchmark-wallet-state-update branch from 8bfe31d to e8772e7 Compare May 28, 2025 15:24
@Sword-Smith Sword-Smith merged commit ac85b22 into master May 28, 2025
13 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants