Skip to content

chore: set precomp window size by platform#135

Merged
abelmega merged 3 commits into
mainfrom
abel/tune/zkvm-precomp-window-size
May 25, 2026
Merged

chore: set precomp window size by platform#135
abelmega merged 3 commits into
mainfrom
abel/tune/zkvm-precomp-window-size

Conversation

@abelmega
Copy link
Copy Markdown
Contributor

Summary

This PR centralizes the default precomputation window size used by Committer initialization and makes it platform-aware.

The main change is:

  • use 3 on zkvm + riscv32
  • keep 11 on all other platforms

Changes

  • add banderwagon::platform::DEFAULT_PRECOMP_WINDOW_SIZE
  • export platform from banderwagon
  • update salt::trie to consume the shared platform default instead of keeping a local constant

Motivation

PRECOMP_WINDOW_SIZE is a platform tuning parameter rather than trie-local logic. Moving it into banderwagon makes the ownership clearer and avoids duplicating platform-specific configuration in salt.

This also allows zkVM to use a smaller window size without changing the default behavior on non-zkVM platforms.

@abelmega abelmega requested review from deepld, flyq and yilongli May 22, 2026 09:42
@github-actions
Copy link
Copy Markdown

github-actions Bot commented May 22, 2026

Performance Benchmark Comparison

Compared 5 benchmark(s) against the latest main baseline.

Detailed Comparison
Benchmark Baseline Throughput (Kelem/s) New Throughput (Kelem/s) Change
update 10000 KVs/1 threads 76.21 78.64 +3.18%
update 10000 KVs/2 threads 142.73 143.67 +0.66%
update 10000 KVs/4 threads 264.68 268.40 +1.41%
update 10000 KVs/8 threads 450.43 432.70 -3.94%
update 10000 KVs/16 threads 612.24 596.93 -2.50%

Copy link
Copy Markdown
Member

@flyq flyq left a comment

Choose a reason for hiding this comment

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

Other call sites still hardcode 11

After this PR, Committer::new(..., 11) still appears at:

  • ipa-multipoint/src/main.rs:14 — benchmark binary
  • banderwagon/src/salt_committer.rs:55 — doc comment
  • banderwagon/src/salt_committer.rs:519,561 — tests

The test/benchmark sites are defensible (they're measuring fixed parameters), but if the centralization story matters, at least update the doc comment in salt_committer.rs:55 to mention platform::DEFAULT_PRECOMP_WINDOW_SIZE. The ipa-multipoint benchmark is your call — leave it pinned to 11 for stable measurements, or migrate it for consistency.

Comment thread banderwagon/src/platform.rs
@abelmega
Copy link
Copy Markdown
Contributor Author

Other call sites still hardcode 11

After this PR, Committer::new(..., 11) still appears at:

  • ipa-multipoint/src/main.rs:14 — benchmark binary
  • banderwagon/src/salt_committer.rs:55 — doc comment
  • banderwagon/src/salt_committer.rs:519,561 — tests

The test/benchmark sites are defensible (they're measuring fixed parameters), but if the centralization story matters, at least update the doc comment in salt_committer.rs:55 to mention platform::DEFAULT_PRECOMP_WINDOW_SIZE. The ipa-multipoint benchmark is your call — leave it pinned to 11 for stable measurements, or migrate it for consistency.

Updated the remaining Committer::new(..., 11) call sites to use platform::DEFAULT_PRECOMP_WINDOW_SIZE as well.

This includes:

  • the salt_committer doc example
  • the fixed-parameter tests in banderwagon
  • the ipa-multipoint benchmark binary

I also extended the existing correctness sweep to cover window_size = 3.

@abelmega abelmega requested a review from flyq May 25, 2026 02:42
@abelmega abelmega merged commit 4ac8b35 into main May 25, 2026
9 checks passed
@abelmega abelmega deleted the abel/tune/zkvm-precomp-window-size branch May 25, 2026 02:54
@flyq flyq mentioned this pull request Jun 2, 2026
3 tasks
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.

3 participants