perf(evm): relax jit fallback thresholds for benchmark corpus#430
Open
abmcar wants to merge 2 commits intoDTVMStack:mainfrom
Open
perf(evm): relax jit fallback thresholds for benchmark corpus#430abmcar wants to merge 2 commits intoDTVMStack:mainfrom
abmcar wants to merge 2 commits intoDTVMStack:mainfrom
Conversation
Keep the bytecode size cap unchanged while raising the MIR and RA pattern limits. This keeps the current evmone benchmark corpus on the JIT path, including the pathological signextend micro benchmark.
Contributor
There was a problem hiding this comment.
Pull request overview
This PR adjusts the EVM JIT precompile fallback thresholds in EVMAnalyzer to keep the existing evmone benchmark corpus on the JIT path (while preserving the bytecode-size cap).
Changes:
- Raised MIR-estimate and RA-pattern fallback thresholds used to decide when to force interpreter execution.
- Updated the in-header documentation describing the intent behind these thresholds.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Comment on lines
+114
to
+117
| /// Thresholds for JIT suitability fallback. Keep the bytecode size cap intact, | ||
| /// but raise the MIR / RA pattern limits high enough that the current evmone | ||
| /// benchmark corpus stays on the JIT path (including the pathological | ||
| /// signextend micro benchmark). |
There was a problem hiding this comment.
The new threshold comment hard-codes a policy goal tied to the current evmone benchmark corpus (and a specific micro-benchmark). That’s likely to go stale and makes it harder to reason about the safety goal of this fallback (avoiding pathological JIT compile-time blowups). Consider rewriting this comment to describe the invariant being protected (e.g., cap compile-time / RA complexity) and, if needed, reference the benchmark corpus in a higher-level tuning doc or commit/benchmark notes rather than as the normative requirement in a public header.
Suggested change
| /// Thresholds for JIT suitability fallback. Keep the bytecode size cap intact, | |
| /// but raise the MIR / RA pattern limits high enough that the current evmone | |
| /// benchmark corpus stays on the JIT path (including the pathological | |
| /// signextend micro benchmark). | |
| /// Thresholds for JIT suitability fallback. These limits bound bytecode size | |
| /// and estimated MIR / RA complexity to avoid pathological JIT compile-time | |
| /// blowups (e.g., superlinear register allocation on dense RA-expensive | |
| /// instruction patterns), while keeping typical contracts on the JIT path. |
Comment on lines
118
to
+122
| static constexpr size_t MAX_JIT_BYTECODE_SIZE = 0x6000; | ||
| static constexpr size_t MAX_JIT_MIR_ESTIMATE = 50000; | ||
| static constexpr size_t MAX_CONSECUTIVE_RA_EXPENSIVE = 128; | ||
| static constexpr size_t MAX_BLOCK_RA_EXPENSIVE = 256; | ||
| static constexpr size_t MAX_DUP_FEEDBACK_PATTERN = 64; | ||
| static constexpr size_t MAX_JIT_MIR_ESTIMATE = 0x50000; | ||
| static constexpr size_t MAX_CONSECUTIVE_RA_EXPENSIVE = 0x3000; | ||
| static constexpr size_t MAX_BLOCK_RA_EXPENSIVE = 0x3000; | ||
| static constexpr size_t MAX_DUP_FEEDBACK_PATTERN = 0x3000; |
There was a problem hiding this comment.
These new limits (0x3000 RA-expensive ops per block / consecutive run, and 0x50000 MIR estimate) are orders of magnitude above the “hundreds cause compilation times to explode” rationale described earlier in this file. With MAX_JIT_BYTECODE_SIZE still at 0x6000, this effectively allows extremely RA-dense single-block bytecode to stay on the JIT path, which undermines the fallback’s purpose and can increase worst-case compile-time (and potential DoS surface) for attacker-controlled contracts. Can we (a) justify these specific magnitudes with data in a comment, and/or (b) tie these thresholds to MAX_JIT_BYTECODE_SIZE or a measured compile-time budget so the guard remains meaningful as workloads change?
⚡ Performance Regression Check Results✅ Performance Check Passed (interpreter)Performance Benchmark Results (threshold: 25%)
Summary: 194 benchmarks, 0 regressions ✅ Performance Check Passed (multipass)Performance Benchmark Results (threshold: 25%)
Summary: 194 benchmarks, 0 regressions |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
micro/signextendValidation
fallback_contracts=0cmake --build build -j4external/totalbenchmark comparison against the old thresholds+0.28%+0.69%memory_grow_mstore/*improves, whilesignextend/*andsnailtracerregress slightly