Per block gas accounting#185
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughThis PR adds per-block gas accounting: it introduces a new Possibly related PRs
Suggested reviewers
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
bench/run.ts (1)
37-46:⚠️ Potential issue | 🟡 MinorMissing
--block-gasoption in help text.The new
--block-gasCLI option is not documented in the help output.📝 Proposed fix to add help text
console.log(`Usage: tsx bench/run.ts [options] Options: --traces <dir> Directory with ecalli trace .log files --w3f <dir> Directory with W3F test vector .json files --iterations <n> Number of timed iterations (default: 5) --warmup <n> Number of warmup iterations (default: 1) --output <file> Write JSON results to file + --block-gas Enable per-block gas accounting (default: false) -h, --help Show this help`);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@bench/run.ts` around lines 37 - 46, The help text printed in bench/run.ts is missing the new --block-gas CLI option; update the console.log usage block (the multi-line string printed in run.ts) to include a new line documenting "--block-gas <n> Block gas limit to use for simulated blocks" (or similar concise description) so the option appears in the displayed help alongside the existing --traces, --w3f, --iterations, --warmup, and --output entries.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In `@bench/run.ts`:
- Around line 37-46: The help text printed in bench/run.ts is missing the new
--block-gas CLI option; update the console.log usage block (the multi-line
string printed in run.ts) to include a new line documenting "--block-gas <n>
Block gas limit to use for simulated blocks" (or similar concise description) so
the option appears in the displayed help alongside the existing --traces, --w3f,
--iterations, --warmup, and --output entries.
ℹ️ Review info
Configuration used: Path: .coderabbit.yml
Review profile: ASSERTIVE
Plan: Pro
Disabled knowledge base sources:
- Linear integration is disabled
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (8)
assembly/api-debugger.tsassembly/api-internal.tsassembly/api-types.tsassembly/api-utils.tsassembly/interpreter.tsassembly/program.tsbench/run.tsbin/src/trace-replay.ts
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In @.github/workflows/build.yml:
- Around line 78-79: When running the benchmark step that executes "npm run
bench:baseline" the current copy command "cp ./bench/baseline.json
../baseline.json" writes the file to the workspace root, but the later compare
step expects "../baseline.json" relative to the workspace root (i.e. one level
up); update the copy destination in the build.yml step that currently runs "cp
./bench/baseline.json ../baseline.json" so it writes the baseline to the parent
of the workspace (use "../../baseline.json" when still inside the "./baseline"
subdirectory) so the compare step that references "../baseline.json" will find
the file.
ℹ️ Review info
Configuration used: Path: .coderabbit.yml
Review profile: ASSERTIVE
Plan: Pro
Disabled knowledge base sources:
- Linear integration is disabled
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (1)
.github/workflows/build.yml
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@bench/run.ts`:
- Around line 49-50: The code uses parseInt for CLI inputs (e.g., computing
ITERATIONS and WARMUP) which accepts partially numeric strings like "5abc" or
"3.7"; add explicit validation before parsing to reject non-integer strings (use
a strict integer regex like /^\d+$/) for values.iterations, values.warmup and
every other CLI numeric value parsed in this file (the same pattern around the
parseInt usage in the 53-62 block). If validation fails, surface a clear
error/exit early instead of calling parseInt; then call parseInt (or Number)
only on validated strings to compute ITERATIONS, WARMUP, and the other numeric
constants.
ℹ️ Review info
Configuration used: Path: .coderabbit.yml
Review profile: ASSERTIVE
Plan: Pro
Disabled knowledge base sources:
- Linear integration is disabled
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (3)
bench/compare.tsbench/run.tsbiome.jsonc
Benchmark Results✅ No significant changes
All traces
|
|
@coderabbitai create an issue with performance optimisations ideas from PR description and add relevant context and assessment. |
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain🏁 Script executed: Length of output: 1378 🏁 Script executed: Length of output: 46
The issue includes:
You can view it here: #187 |
Results on my machine:
The branch contains a bunch of other performance improvements as well:
Some IDEAS for future optimisations:
StaticArray<u64>containing all required information in the hot loop - three entries contain an instruction, gas, block info and all arguments decoded. We are additionally going to need aStaticArray<u32>that will be a mapping of PC into indices in this new program array.Resolves #183