feat(l1): add --no-precompile-cache flag for benchmarking#6562
feat(l1): add --no-precompile-cache flag for benchmarking#6562
Conversation
Adds a CLI flag (env ETHREX_NO_PRECOMPILE_CACHE, default off) that disables the per-block precompile result cache for the full-node entry point and the import-bench subcommand, leaving every other call site cached by default. Implementation: BlockchainOptions.precompile_cache_enabled (default true) is threaded into CachingDatabase, which now stores Option<PrecompileCache> so nothing is allocated when disabled. Database::precompile_cache() returns self.precompile_cache.as_ref(); execute_precompile already handles None.
ilitteri
requested review from
a team,
ManuelBilbao and
avilagaston9
as code owners
🤖 Kimi Code ReviewGeneral Assessment Specific Issues
Positive Feedback
Automated review by Kimi (Moonshot AI) · kimi-k2.5 · custom prompt |
🤖 Claude Code ReviewNow I have a complete picture of the changes. Let me write the review. PR Review:
|
Lines of code reportTotal lines added: Detailed view |
Greptile SummaryThis PR adds a Confidence Score: 5/5Safe to merge — opt-in benchmarking flag with no behavior change on the default path. No P0 or P1 issues found. The None-cache path in execute_precompile is correctly guarded with cache.and_then/let Some(cache) patterns, all call sites are updated, default remains cache-on, and both exhaustive struct literals compile correctly with the new field. No files require special attention.
|
| Filename | Overview |
|---|---|
| crates/vm/levm/src/db/mod.rs | Changes precompile_cache field to Option, using .then(PrecompileCache::new) for conditional allocation; precompile_cache() trait impl correctly returns self.precompile_cache.as_ref() |
| crates/blockchain/blockchain.rs | Adds precompile_cache_enabled: bool to BlockchainOptions (default true) and passes it to CachingDatabase::new; exhaustive struct literal in init_l1 covers all fields |
| cmd/ethrex/cli.rs | Adds --no-precompile-cache clap arg (env ETHREX_NO_PRECOMPILE_CACHE) to Options; wired to BlockchainOptions for ImportBench; Default impl updated correctly |
| cmd/ethrex/initializers.rs | Passes !opts.no_precompile_cache to BlockchainOptions for full-node startup; exhaustive struct literal remains complete with all fields |
| cmd/ethrex/l2/initializers.rs | Explicitly sets precompile_cache_enabled: true to keep existing L2 behavior; updated exhaustive struct literal compiles cleanly |
Flowchart
%%{init: {'theme': 'neutral'}}%%
flowchart TD
A["CLI: --no-precompile-cache\n(ETHREX_NO_PRECOMPILE_CACHE)"] --> B{Options.no_precompile_cache}
B -->|Full node init_l1| C["BlockchainOptions\nprecompile_cache_enabled = !no_precompile_cache"]
B -->|ImportBench subcommand| D["BlockchainOptions\nprecompile_cache_enabled = !no_precompile_cache"]
E["L2 init_l2\n(always true)"] --> F["BlockchainOptions\nprecompile_cache_enabled = true"]
G["Other entry points\n(import, ef-tests, benches...)"] --> H["BlockchainOptions::default()\nprecompile_cache_enabled = true"]
C & D & F & H --> I["Blockchain::execute_block_pipeline"]
I --> J["CachingDatabase::new(store, precompile_cache_enabled)"]
J -->|true| K["precompile_cache: Some(PrecompileCache::new())"]
J -->|false| L["precompile_cache: None"]
K & L --> M["execute_precompile(...)"]
M -->|Some cache| N["cache.get / cache.insert\n(fast path)"]
M -->|None cache| O["compute directly\n(no cache read/write)"]
Reviews (1): Last reviewed commit: "feat(l1): add --no-precompile-cache flag..." | Re-trigger Greptile
🤖 Codex Code Review
Aside from that wiring bug, the I couldn’t complete a local Automated review by OpenAI Codex · gpt-5.4 · custom prompt |
There was a problem hiding this comment.
Pull request overview
Adds an opt-in CLI switch to disable the per-block precompile result cache to enable A/B benchmarking of block execution performance, while keeping the default cached behavior unchanged for existing entry points.
Changes:
- Introduces
--no-precompile-cache/ETHREX_NO_PRECOMPILE_CACHEand wires it into L1 full-node startup andimport-bench. - Threads a new
precompile_cache_enabledflag throughBlockchainOptionsintoCachingDatabase. - Makes
CachingDatabase’s precompile cache optional to avoid allocating it when disabled.
Reviewed changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
crates/vm/levm/src/db/mod.rs |
Makes precompile_cache optional and updates CachingDatabase construction / trait exposure accordingly. |
crates/blockchain/blockchain.rs |
Adds precompile_cache_enabled to BlockchainOptions (default true) and passes it into CachingDatabase. |
cmd/ethrex/cli.rs |
Adds the --no-precompile-cache CLI flag/env var; applies it for ImportBench. |
cmd/ethrex/initializers.rs |
Applies the flag to L1 full-node BlockchainOptions. |
cmd/ethrex/l2/initializers.rs |
Explicitly keeps the cache enabled for L2 initialization (behavior unchanged). |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| /// Shared precompile result cache (warmer populates, executor reuses) | ||
| precompile_cache: PrecompileCache, | ||
| /// Shared precompile result cache (warmer populates, executor reuses). | ||
| /// `None` when the cache is disabled via `BlockchainOptions::precompile_cache_enabled = false`. |
| pub fn new(inner: Arc<dyn Database>, precompile_cache_enabled: bool) -> Self { | ||
| Self { | ||
| inner, | ||
| accounts: RwLock::new(FxHashMap::default()), | ||
| storage: RwLock::new(FxHashMap::default()), |
| /// If true (default), per-block execution caches precompile results between the | ||
| /// warmer thread and the executor. Set to false (via `--no-precompile-cache`) to | ||
| /// disable the cache for benchmarking purposes. | ||
| pub precompile_cache_enabled: bool, |
Benchmark Results ComparisonNo significant difference was registered for any benchmark run. Detailed ResultsBenchmark Results: BubbleSort
Benchmark Results: ERC20Approval
Benchmark Results: ERC20Mint
Benchmark Results: ERC20Transfer
Benchmark Results: Factorial
Benchmark Results: FactorialRecursive
Benchmark Results: Fibonacci
Benchmark Results: FibonacciRecursive
Benchmark Results: ManyHashes
Benchmark Results: MstoreBench
Benchmark Results: Push
Benchmark Results: SstoreBench_no_opt
|
pablodeymo
left a comment
pablodeymo
left a comment
There was a problem hiding this comment.
The code is righr t.
One nit, the new CLI arg should be documented in: docs/CLI.md
Motivation
The per-block precompile result cache (
PrecompileCacheincrates/vm/levm/src/precompiles.rs) is always on during block execution. To benchmark how much the cache contributes to block-execution time, we need an opt-in switch to disable it without otherwise altering the execution pipeline.Description
Adds a
--no-precompile-cacheCLI flag (envETHREX_NO_PRECOMPILE_CACHE, default off) that disables the per-block precompile result cache for the full-node entry point and theimport-benchsubcommand. Default behavior is preserved everywhere else.BlockchainOptionsgains a newprecompile_cache_enabled: boolfield (defaults totrue), whichBlockchain::execute_block_pipelinepasses intoCachingDatabase::new.CachingDatabase::precompile_cachebecomesOption<PrecompileCache>so that, when disabled, no cache is allocated.Database::precompile_cache()returnsself.precompile_cache.as_ref().execute_precompilealready short-circuits cleanly onNone.BlockchainOptionsconstruction sites:cmd/ethrex/initializers.rs— full-node startupcmd/ethrex/cli.rs—Subcommand::ImportBenchimport, L2, ef-tests, migrations, state_v2, benches) keep their existing cached behavior viaDefault.Files changed
crates/vm/levm/src/db/mod.rsCachingDatabase::precompile_cacheis nowOption<PrecompileCache>;newtakesprecompile_cache_enabled: bool; trait impl returnsas_ref()crates/blockchain/blockchain.rsprecompile_cache_enabled: boolonBlockchainOptions(defaulttrue); passed toCachingDatabase::newinexecute_block_pipelinecmd/ethrex/cli.rs--no-precompile-cacheclap arg;Subcommand::ImportBenchreads itcmd/ethrex/initializers.rsBlockchainOptionsreads!opts.no_precompile_cachecmd/ethrex/l2/initializers.rsprecompile_cache_enabled: true(struct literal spelled exhaustively; behavior unchanged — L2 keeps cache on)Usage
The flag sits on the parent
Optionsstruct, so it goes before the subcommand (mirroring--no-migrate).Checklist
STORE_SCHEMA_VERSION(crates/storage/lib.rs) if the PR includes breaking changes to theStorerequiring a re-sync. — N/A, noStorechanges.