-
Notifications
You must be signed in to change notification settings - Fork 194
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
bench(katana): class compilation #3010
Conversation
Ohayo, sensei! WalkthroughThe changes incorporate two new dependencies, criterion and pprof, into the package configuration with the workspace flag enabled. A new benchmark configuration section has been added to the configuration file, and a benchmark module has been introduced to measure the performance of contract class compilation. This module uses the Criterion library to set up and run benchmarks, including group registration and a main entry point for benchmark execution. Changes
Sequence Diagram(s)sequenceDiagram
participant Runner as Criterion Runner
participant Group as Benchmark Group
participant Func as class_compilation()
participant FS as File System
Runner->>Group: Start benchmark execution
Group->>Func: Invoke class_compilation benchmark
Func->>FS: Read contract JSON file
FS-->>Func: Return JSON data
Func->>Func: Parse JSON and compile ContractClass
Func-->>Group: Return benchmark timing data
Group-->>Runner: Report benchmark results
📜 Recent review detailsConfiguration used: .coderabbit.yaml 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
⏰ Context from checks skipped due to timeout of 90000ms (1)
🪧 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
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 1
🧹 Nitpick comments (1)
crates/katana/primitives/benches/class.rs (1)
7-16
: Nice benchmark implementation, sensei!The benchmark setup looks good with proper use of
iter_with_large_drop
to prevent GC interference. However, consider adding more test cases with different class sizes to get a better performance profile.Would you like me to help create additional benchmark cases with different contract sizes?
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
Cargo.lock
is excluded by!**/*.lock
📒 Files selected for processing (2)
crates/katana/primitives/Cargo.toml
(2 hunks)crates/katana/primitives/benches/class.rs
(1 hunks)
🧰 Additional context used
🪛 GitHub Actions: ci
crates/katana/primitives/benches/class.rs
[warning] 1-1: Code formatting issue: Import statements should be on separate lines.
🔇 Additional comments (3)
crates/katana/primitives/benches/class.rs (1)
18-22
: Excellent profiler configuration, sensei!The criterion group configuration with flamegraph output will be very helpful for performance analysis. The 200ms warm-up time is a reasonable default.
crates/katana/primitives/Cargo.toml (2)
41-42
: Ohayo! Nice choice of dependencies, sensei!The addition of criterion and pprof with workspace inheritance is perfect for benchmarking needs.
57-59
: Clean benchmark configuration!The bench configuration is properly set up with
harness = false
as required for criterion benchmarks.
use std::{str::FromStr, time::Duration}; | ||
|
||
use criterion::{criterion_group, criterion_main, Criterion}; | ||
use katana_primitives::class::ContractClass; | ||
use pprof::criterion::{Output, PProfProfiler}; | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ohayo sensei! Let's fix the import formatting.
The pipeline is warning about imports being on one line. Let's split them for better readability:
-use std::{str::FromStr, time::Duration};
+use std::str::FromStr;
+use std::time::Duration;
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
use std::{str::FromStr, time::Duration}; | |
use criterion::{criterion_group, criterion_main, Criterion}; | |
use katana_primitives::class::ContractClass; | |
use pprof::criterion::{Output, PProfProfiler}; | |
use std::str::FromStr; | |
use std::time::Duration; | |
use criterion::{criterion_group, criterion_main, Criterion}; | |
use katana_primitives::class::ContractClass; | |
use pprof::criterion::{Output, PProfProfiler}; |
🧰 Tools
🪛 GitHub Actions: ci
[warning] 1-1: Code formatting issue: Import statements should be on separate lines.
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #3010 +/- ##
=======================================
Coverage 56.40% 56.40%
=======================================
Files 431 431
Lines 57162 57162
=======================================
Hits 32242 32242
Misses 24920 24920 ☔ View full report in Codecov by Sentry. |
Bench class compilation for the default class. This doesn't really give us a good representation of what the average compilation time for contract classes, as it depends on the size of the class itself. But it can give us a rough idea of how long it takes for an ~average size class to be compiled.
Summary by CodeRabbit