perf(katana): optimisations + benchmark setup #2900
Conversation
glihm
changed the title
remybar
force-pushed
the
katana-core-parallelized
branch
from
e2dacee to
5519e6b
Compare
There was a problem hiding this comment.
This benchmark includes data setup within the measured function (ie Uncommitted::commit). We should move the preparation of the test data (all the arguments for Uncommitted::new) outside the benchmark routine to ensure we're measuring only the commit operation's performance, not including the setup overhead.
We can use the Arbitrary trait to derive random values for the test data.
I'm not sure what's the best block composition but maybe we can start with something like 20 transactions, each with 2 events, and a state update of size 100.
|
I think what we should do, is have two |
remybar
force-pushed
the
katana-core-parallelized
branch
from
5519e6b to
3728e6d
Compare
|
Hi @kariy ! So, I've kept 2 versions of optimized functions (original + I used Maybe it would be a good idea to have 2 sets of data for these benches:
WDYT ? |
remybar
force-pushed
the
katana-core-parallelized
branch
2 times, most recently
from
e9aaea6 to
347b9b0
Compare
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #2900 +/- ##
==========================================
- Coverage 57.10% 57.04% -0.07%
==========================================
Files 429 429
Lines 56868 56939 +71
==========================================
+ Hits 32475 32481 +6
- Misses 24393 24458 +65 ☔ View full report in Codecov by Sentry. |
remybar
force-pushed
the
katana-core-parallelized
branch
from
347b9b0 to
82a0abb
Compare
Oups, I didn't receive any notification for your message 😕 |
|
Ohayo sensei! WalkthroughThis pull request updates dependency management and introduces benchmarking and parallel processing enhancements. It adds and modifies Changes
Sequence Diagram(s)sequenceDiagram
participant Bench as Benchmark Suite
participant Block as UncommittedBlock
participant Backend as Backend Processing
Bench->>Block: build_block(config)
Bench->>Block: commit() / commit_parallel()
alt Serial Commit
Block->>Backend: commit()
else Parallel Commit
Block->>Backend: commit_parallel()
Backend->>Backend: compute_event_commitment_parallel()
Backend->>Backend: compute_receipt_commitment_parallel()
end
Possibly related PRs
🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
Documentation and Community
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
crates/katana/core/src/backend/mod.rs (2)
367-367: Ohayo sensei! Clarify this comment or remove if it’s unneeded.
“optimisation 1” can be more descriptive. Consider including why this optimization is important.
484-503: Ohayo sensei! Great use of par_bridge for events.
Spawning parallel tasks like this can handle many events effectively. Just be mindful of overhead if event counts are small.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (5)
Cargo.toml(1 hunks)crates/katana/core/Cargo.toml(3 hunks)crates/katana/core/benches/commit.rs(1 hunks)crates/katana/core/src/backend/mod.rs(5 hunks)crates/katana/executor/Cargo.toml(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: ensure-windows
- GitHub Check: test
🔇 Additional comments (14)
crates/katana/core/src/backend/mod.rs (3)
24-24: Ohayo sensei! Nice addition of the rayon prelude.
This import sets the stage for parallel iterators, which supercharges concurrency.
398-441: Ohayo sensei! Bravo on the parallel commit method.
Using rayon’s scope with carefully separated variables is safe here. Each closure writes to a distinct variable, eliminating data races. If you expect many large blocks, this concurrency should boost throughput.
453-457: Ohayo sensei! Parallel receipt commitment looks good.
Parallelizing over receipts can reduce total computation time significantly.crates/katana/core/benches/commit.rs (4)
16-24: Ohayo sensei! BlockConfig is a neat approach.
Using a small struct to bundle block parameters keeps things organized and testable.
46-52: Ohayo sensei! Good separation of serial vs. parallel commit.
Explicitly having two methods (commit vs. commit_parallel) makes benchmark comparisons more transparent.
94-128: Ohayo sensei! Great job building random transactions and receipts.
This is a succinct way to generate test data for the benchmarks. Remember to watch out for any potential fuzz issues if the random values might cause edge cases.
130-195: Ohayo sensei! Solid benchmark definitions for small and big blocks.
These distinct setups will help confirm that parallel processing overhead remains minimal for small inputs, while yielding performance gains for large ones.crates/katana/executor/Cargo.toml (1)
39-40: Ohayo sensei! Transitioning pprof and rayon to workspace dependencies.
This unifies version management, simplifies maintenance, and helps ensure consistency across crates. Nicely done!crates/katana/core/Cargo.toml (5)
14-14: Ohayo sensei: Enhanced Dependency for katana-primitives
Including the "arbitrary" feature in the katana-primitives dependency is a solid move to support random value generation in benchmarks and tests. Just double-check its compatibility with other components relying on this dependency.
29-29: Ohayo sensei: Introducing Rayon for Parallelism
Addingrayon.workspace = trueis a welcome enhancement to enable parallel processing. This change directly supports the new parallel commit implementations and should help meet performance goals.
52-52: Ohayo sensei: Criterion for Benchmarking
The newcriterion.workspace = truedev-dependency is an excellent addition for setting up performance benchmarks. It will help you precisely measure the benefits of parallelized processing.
57-57: Ohayo sensei: pprof for Profiling
Integratingpprof.workspace = trueas a development tool supports detailed performance profiling and work in tandem with Criterion—and that's pretty neat!
62-65: Ohayo sensei: Benchmark Configuration Added
The new benchmark definition withname = "commit"andharness = falseis well configured and aligns with the goal of comparing sequential versus parallelized commits. Ensure that the corresponding benchmark implementation incrates/katana/core/benches/commit.rscorrectly exercises both commit methods.Cargo.toml (1)
261-263: Ohayo sensei: pprof Dependency for Profiling in Root Cargo.toml
The introduction ofpprof = { version = "0.13.0", features = [ "criterion", "flamegraph" ] }in the root Cargo.toml reinforces profiling support across the project. Please verify that this version meshes well with Criterion and your overall benchmarking setup.
|
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
crates/katana/primitives/src/block.rs (1)
45-56: Ohayo sensei! “PartialHeader” is well structured.
This design fosters flexibility in block creation, especially for partial or intermediate states. Keep an eye on consistent usage across the codebase to avoid confusion with “Header.”
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
crates/katana/core/benches/commit.rs(1 hunks)crates/katana/primitives/src/block.rs(1 hunks)crates/katana/primitives/src/receipt.rs(1 hunks)crates/katana/primitives/src/transaction.rs(1 hunks)
🔇 Additional comments (8)
crates/katana/core/benches/commit.rs (4)
45-51: Ohayo sensei! Great introduction of parallel commits.
It’s awesome to see parallelization integrated for performance gains. Ensure that internal concurrency usage in "commit_parallel()" handles potential data races or synchronization overhead.Would you like deeper assistance analyzing concurrency aspects within the broader codebase? I can craft scripts that search for lock usage or concurrency patterns that may need more robust synchronization.
133-159: Ohayo sensei! Consider more descriptive benchmark names.
Currently, "Serial" and "Parallel" are used, but it might be beneficial to mirror the benchmark group’s naming for clarity, e.g., "Commit.Small.Serial" and "Commit.Small.Parallel."-c.bench_function("Serial", |b| { +c.bench_function("Commit.Small.Serial", |b| {-c.bench_function("Parallel", |b| { +c.bench_function("Commit.Small.Parallel", |b| {
161-185: Ohayo sensei! Mirror naming for consistency on big-block benchmarks too.
Likewise, standardizing names to "Commit.Big.Serial" and "Commit.Big.Parallel" better differentiates each scenario and aligns them with the group label.-c.bench_function("Serial", |b| { +c.bench_function("Commit.Big.Serial", |b| {-c.bench_function("Parallel", |b| { +c.bench_function("Commit.Big.Parallel", |b| {
192-196: Ohayo sensei! This matches a past suggestion exactly.
The Criterion group configuration with PProf looks great and has already been suggested by Kariy previously. Nicely done!crates/katana/primitives/src/receipt.rs (2)
180-190: Ohayo sensei! Ingenious extension with “ReceiptWithTxHash.”
Attaching the transaction hash to the receipt makes debugging and tracking easier. Good job applying both Deref and AsRef—this seamlessly wraps underlying receipt logic.
203-246: Ohayo sensei! Double-check revert reason hashing.
The revert reason is hashed using “starknet_keccak,” but ensure no hidden edge cases with unusual characters or empty strings.crates/katana/primitives/src/transaction.rs (2)
654-664: Ohayo! Clean and well-structured implementation, sensei!The
TxWithHashstruct is well-designed with appropriate trait derivations and documentation.
666-676: LGTM! Excellent handling of ownership semantics, sensei!Both
Fromimplementations are clean and efficient:
- One for owned
ExecutableTxWithHash- One for referenced
ExecutableTxWithHashThis provides flexibility while maintaining performance.
|
For now, let's keep using the serial version in the main flow. Assuming most usages of |
Yes, no problem 👍 Thanks 🙏 |
Description
Parallelize some block processing and add some benches to monitor the performance of sequential/parallelized commits.
Related issue
#2719
Tests
Added to documentation?
Checklist
scripts/prettier.sh,scripts/rust_fmt.sh,scripts/cairo_fmt.sh)scripts/clippy.sh,scripts/docs.sh)Summary by CodeRabbit
New Features
Refactor