Portable js benchmarking#191
Merged
Merged
Conversation
Contributor
|
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 moves several module-level utility functions into exported classes with static methods (Inst, IntMath, OutcomeData), updates call sites across the assembly codebase to use the new class-qualified APIs (e.g., Inst.reg, IntMath.minI32, OutcomeData.ok), refactors instruction return helpers into OutcomeData static methods, adjusts many instruction implementations and tests to use Inst accessors, and adds a pluggable PVM API with dynamic backend loading in benchmarking and trace-replay tooling. Possibly related PRs
Suggested reviewers
🚥 Pre-merge checks | ❌ 3❌ Failed checks (2 warnings, 1 inconclusive)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
Runs fib(n) for n=10k..10M to measure raw interpreter loop throughput.
tomusdrw
force-pushed
the
td-benchmark-improvements
branch
from
8338229 to
b394a6a
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
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)
376-376:⚠️ Potential issue | 🟡 MinorHandle top-level async failures explicitly.
Line 376 invokes
main()without a rejection handler. Add a terminal.catch(...)so CLI failures produce deterministic exit behavior and error reporting.🔧 Proposed fix
-main(); +void main().catch((error) => { + console.error(error); + process.exit(1); +});🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@bench/run.ts` at line 376, The top-level invocation main() lacks a rejection handler; update the call site that currently calls main() to append a .catch(...) which logs the error (using console.error or the existing logger) and terminates the process with a non-zero exit code (e.g., process.exit(1)); specifically modify the main() invocation so any rejected promise from main is caught, the error is reported (include error.stack/message), and process.exit(1) is called to ensure deterministic CLI failure behavior.
🤖 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 69-75: The portable-path double-cast in loadPvm masks export-shape
mismatches and defers failures until use; update loadPvm to import the module
into a temporary const (from "../dist/build/js/portable-bundle.js"), validate
that the imported object exposes the required PvmApi members (compare presence
and types of functions/properties expected by PvmApi, e.g., methods used
elsewhere on wasmPvm), and throw a clear Error if validation fails instead of
casting; also harden top-level async termination by changing the unhandled call
to main() to a handled one (e.g., await main() inside an async IIFE or call
main().catch(err => { console.error(..., err); process.exit(1); })) so any
rejected promise logs the error and exits with a deterministic non-zero code.
---
Outside diff comments:
In `@bench/run.ts`:
- Line 376: The top-level invocation main() lacks a rejection handler; update
the call site that currently calls main() to append a .catch(...) which logs the
error (using console.error or the existing logger) and terminates the process
with a non-zero exit code (e.g., process.exit(1)); specifically modify the
main() invocation so any rejected promise from main is caught, the error is
reported (include error.stack/message), and process.exit(1) is called to ensure
deterministic CLI failure behavior.
ℹ️ 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 (27)
assembly/arguments.tsassembly/instructions/bit.test.tsassembly/instructions/bit.tsassembly/instructions/branch.test.tsassembly/instructions/branch.tsassembly/instructions/jump.tsassembly/instructions/load.tsassembly/instructions/logic.test.tsassembly/instructions/logic.tsassembly/instructions/math.test.tsassembly/instructions/math.tsassembly/instructions/misc.tsassembly/instructions/mov.tsassembly/instructions/outcome.tsassembly/instructions/rot.test.tsassembly/instructions/rot.tsassembly/instructions/set.tsassembly/instructions/shift.tsassembly/instructions/store.tsassembly/instructions/utils.tsassembly/interpreter.tsassembly/math.tsassembly/memory.tsassembly/program-build.tsassembly/program.tsbench/run.tsbin/src/trace-replay.ts
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
bench/run.ts (1)
69-75:⚠️ Potential issue | 🟠 MajorHarden backend loading and top-level async error handling.
Line 72 suppresses module-shape validation with a double cast, and Line 377 leaves async failures unhandled (
main()without.catch). This can fail late and non-deterministically.🔧 Proposed fix
+function assertPvmApi(mod: unknown): PvmApi { + const candidate = ((mod as { default?: unknown }).default ?? mod) as Record<string, unknown>; + const required = [ + "HasMetadata", + "InputKind", + "prepareProgram", + "runProgram", + "pvmStart", + "pvmDestroy", + "pvmResume", + "pvmReadMemory", + "pvmWriteMemory", + "pvmSetRegisters", + ] as const; + for (const key of required) { + if (!(key in candidate)) { + throw new Error(`Invalid PVM module: missing "${key}" export`); + } + } + return candidate as PvmApi; +} + async function loadPvm(): Promise<PvmApi> { if (values.portable) { - // The portable build exposes the same API surface but has different nominal types. - return (await import("../dist/build/js/portable-bundle.js")) as unknown as PvmApi; + return assertPvmApi(await import("../dist/build/js/portable-bundle.js")); } - return wasmPvm; + return assertPvmApi(wasmPvm); }-main(); +main().catch((err) => { + console.error(err); + process.exit(1); +});#!/bin/bash set -euo pipefail # 1) Verify risky cast and unhandled async entrypoint are still present. rg -n 'as unknown as PvmApi|^main\(\);$' bench/run.ts # 2) If portable bundle exists, verify required API keys are present in bundle text. if fd 'portable-bundle\.js$' dist >/dev/null 2>&1; then bundle="$(fd 'portable-bundle\.js$' dist | head -n1)" rg -n 'HasMetadata|InputKind|prepareProgram|runProgram|pvmStart|pvmDestroy|pvmResume|pvmReadMemory|pvmWriteMemory|pvmSetRegisters' "$bundle" fiAlso applies to: 377-377
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@bench/run.ts` around lines 69 - 75, Replace the unsafe double-cast in loadPvm by validating the imported module actually implements the PvmApi surface (check for required exports/methods like HasMetadata, InputKind, prepareProgram, runProgram, pvmStart, pvmDestroy, pvmResume, pvmReadMemory, pvmWriteMemory, pvmSetRegisters) and throw a descriptive error if any are missing rather than returning as unknown as PvmApi; additionally, change the top-level startup call to handle rejections from main() (replace the bare main() call with a promise handler that logs the error and exits non‑zero) so async failures are deterministic and reported.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@assembly/instructions/misc.ts`:
- Around line 20-25: The sbrk instruction can return a stale fault because the
shared faultRes isn't cleared before calling memory.sbrk; before invoking
memory.sbrk in the sbrk function, reset or reinitialize faultRes (e.g., clear
faultRes.isFault / set faultRes = { isFault: false, ... } or construct a fresh
FaultResult) so memory.sbrk cannot read a sticky fault, then call
memory.sbrk(...) and continue using OutcomeData.okOrFault(r, faultRes) as
before; update the code around sbrk, memory.sbrk, faultRes,
OutcomeData.okOrFault, and Inst.reg to ensure faultRes is freshly cleared each
call.
---
Duplicate comments:
In `@bench/run.ts`:
- Around line 69-75: Replace the unsafe double-cast in loadPvm by validating the
imported module actually implements the PvmApi surface (check for required
exports/methods like HasMetadata, InputKind, prepareProgram, runProgram,
pvmStart, pvmDestroy, pvmResume, pvmReadMemory, pvmWriteMemory, pvmSetRegisters)
and throw a descriptive error if any are missing rather than returning as
unknown as PvmApi; additionally, change the top-level startup call to handle
rejections from main() (replace the bare main() call with a promise handler that
logs the error and exits non‑zero) so async failures are deterministic and
reported.
ℹ️ 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 (26)
assembly/arguments.tsassembly/instructions/bit.test.tsassembly/instructions/bit.tsassembly/instructions/branch.test.tsassembly/instructions/branch.tsassembly/instructions/jump.tsassembly/instructions/load.tsassembly/instructions/logic.test.tsassembly/instructions/logic.tsassembly/instructions/math.test.tsassembly/instructions/math.tsassembly/instructions/misc.tsassembly/instructions/mov.tsassembly/instructions/outcome.tsassembly/instructions/rot.test.tsassembly/instructions/rot.tsassembly/instructions/set.tsassembly/instructions/shift.tsassembly/instructions/store.tsassembly/instructions/utils.tsassembly/math.tsassembly/memory.tsassembly/program-build.tsassembly/program.tsbench/run.tsbin/src/trace-replay.ts
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
bench/compare.ts (1)
368-390:⚠️ Potential issue | 🟡 MinorCombined regressions/improvements arrays lose "worst first" ordering.
When merging
fibRegswithregs(andfibImpswithimps), each array is individually sorted, but the combined result is not re-sorted. If both Fibonacci and regular benchmarks have regressions, they won't appear in true "worst first" order.🔧 Proposed fix to maintain sort order
// Regressions (combined) const allRegs = [...fibRegs, ...regs]; + allRegs.sort((a, b) => b.diffPercent - a.diffPercent); if (allRegs.length > 0) { md += "\n<details><summary>Regressions (worst first)</summary>\n\n";// Improvements (combined) const allImps = [...fibImps, ...imps]; + allImps.sort((a, b) => a.diffPercent - b.diffPercent); if (allImps.length > 0) {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@bench/compare.ts` around lines 368 - 390, The combined arrays lose their intended sort order because you simply concatenate fibRegs+regs and fibImps+imps without re-sorting; after creating allRegs and allImps, sort them by diffPercent (e.g., allRegs.sort((a,b)=>b.diffPercent - a.diffPercent) to preserve "worst first" and allImps.sort((a,b)=>b.diffPercent - a.diffPercent) to show largest improvements first) before iterating to build md; apply these sorts to the variables allRegs and allImps in compare.ts (the locations where fibRegs, regs, fibImps, and imps are merged).
🤖 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/compare.ts`:
- Around line 103-157: The Fibonacci comparison block duplicates logic used for
trace comparisons; extract a reusable helper (e.g., compareTraces or
compareBenchmarks) that accepts baseline and current arrays plus threshold and
returns { comparisons, regressions, improvements }; replace the in-place logic
that builds maps and computes diffMs/diffPercent, handles baseline medianMs ===
0, sets status, and sorts results with a call to that helper for both the
fibonacci and trace sections (referencing fibComparisons, fibRegressions,
fibImprovements and the equivalent trace variables) so the shared computation is
centralized and reused.
---
Outside diff comments:
In `@bench/compare.ts`:
- Around line 368-390: The combined arrays lose their intended sort order
because you simply concatenate fibRegs+regs and fibImps+imps without re-sorting;
after creating allRegs and allImps, sort them by diffPercent (e.g.,
allRegs.sort((a,b)=>b.diffPercent - a.diffPercent) to preserve "worst first" and
allImps.sort((a,b)=>b.diffPercent - a.diffPercent) to show largest improvements
first) before iterating to build md; apply these sorts to the variables allRegs
and allImps in compare.ts (the locations where fibRegs, regs, fibImps, and imps
are merged).
ℹ️ 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)
bench/compare.ts
The compare tool now includes fibonacci micro-benchmark results in both console output and the PR markdown comment.
tomusdrw
force-pushed
the
td-benchmark-improvements
branch
from
eb5b308 to
05d5455
Compare
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
bench/compare.ts (2)
380-390:⚠️ Potential issue | 🟡 MinorCombined improvements array not re-sorted.
Same issue as regressions—the combined array should be re-sorted to maintain consistent ordering.
🔧 Proposed fix to re-sort combined improvements
// Improvements (combined) - const allImps = [...fibImps, ...imps]; + const allImps = [...fibImps, ...imps].sort((a, b) => a.diffPercent - b.diffPercent); if (allImps.length > 0) {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@bench/compare.ts` around lines 380 - 390, The combined improvements array allImps (created from fibImps and imps) is not re-sorted, causing inconsistent ordering; update the code after constructing allImps to sort it the same way other combined lists are sorted (e.g., sort allImps by diffPercent descending or by the same comparator used for regressions) before iterating and rendering in the Improvements details block so that entries appear in a consistent, expected order.
263-280:⚠️ Potential issue | 🟡 MinorConsole output inconsistency: Fibonacci regressions/improvements not listed in detail sections.
The
allRegressionsandallImprovementscounts (lines 263-264) include Fibonacci results, but the detailed "Regressions" and "Improvements" sections (lines 268-280) only iterate over trace-specific arrays. This means the summary might show "Regressions: 5" while the detailed listing only shows 3 entries.The markdown output correctly combines both (lines 369, 381), so the console output should match.
🔧 Proposed fix to include Fibonacci results in detailed console sections
-if (regressions.length > 0) { +const allRegsConsole = [...fibRegressions, ...regressions].sort((a, b) => b.diffPercent - a.diffPercent); +if (allRegsConsole.length > 0) { console.log("\n--- Regressions (worst first) ---\n"); - for (const r of regressions) { + for (const r of allRegsConsole) { console.log(` ${r.name.padEnd(40)} ${r.currentMs.toFixed(1).padStart(8)}ms (+${r.diffPercent.toFixed(1)}%)`); } } -if (improvements.length > 0) { +const allImpsConsole = [...fibImprovements, ...improvements].sort((a, b) => a.diffPercent - b.diffPercent); +if (allImpsConsole.length > 0) { console.log("\n--- Improvements ---\n"); - for (const i of improvements) { + for (const i of allImpsConsole) { console.log(` ${i.name.padEnd(40)} ${i.currentMs.toFixed(1).padStart(8)}ms (${i.diffPercent.toFixed(1)}%)`); } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@bench/compare.ts` around lines 263 - 280, The console summary counts use regressions + fibRegressions and improvements + fibImprovements but the detailed sections only iterate regressions and improvements; update the detailed "Regressions" and "Improvements" console blocks to include Fibonacci entries by iterating over combined arrays (e.g., use regressions.concat(fibRegressions) and improvements.concat(fibImprovements) or merge them into new arrays before printing), keeping the existing formatting and headers (and preserve ordering/“worst first” logic for the combined regressions).
🤖 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/compare.ts`:
- Around line 368-378: The combined regressions array allRegs (created from
fibRegs and regs) is not re-sorted, so the "worst first" requirement is
violated; after concatenation, sort allRegs in-place by descending diffPercent
(e.g., use a comparator that orders by b.diffPercent - a.diffPercent) before
iterating to build the markdown so entries like r.name, r.baselineMs,
r.currentMs and r.diffPercent are emitted in worst-to-best order.
---
Outside diff comments:
In `@bench/compare.ts`:
- Around line 380-390: The combined improvements array allImps (created from
fibImps and imps) is not re-sorted, causing inconsistent ordering; update the
code after constructing allImps to sort it the same way other combined lists are
sorted (e.g., sort allImps by diffPercent descending or by the same comparator
used for regressions) before iterating and rendering in the Improvements details
block so that entries appear in a consistent, expected order.
- Around line 263-280: The console summary counts use regressions +
fibRegressions and improvements + fibImprovements but the detailed sections only
iterate regressions and improvements; update the detailed "Regressions" and
"Improvements" console blocks to include Fibonacci entries by iterating over
combined arrays (e.g., use regressions.concat(fibRegressions) and
improvements.concat(fibImprovements) or merge them into new arrays before
printing), keeping the existing formatting and headers (and preserve
ordering/“worst first” logic for the combined regressions).
ℹ️ 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)
bench/compare.ts
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
bench/compare.ts (1)
332-346:⚠️ Potential issue | 🟡 MinorRe-sort combined markdown lists after concatenation.
Combined arrays are not globally ordered after merge. Regressions should remain worst-first, and improvements should remain best-first.
🔧 Suggested fix
-const allRegs = [...fibRegs, ...regs]; +const allRegs = [...fibRegs, ...regs].sort((a, b) => b.diffPercent - a.diffPercent); @@ -const allImps = [...fibImps, ...imps]; +const allImps = [...fibImps, ...imps].sort((a, b) => a.diffPercent - b.diffPercent);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@bench/compare.ts` around lines 332 - 346, The merged arrays allRegs = [...fibRegs, ...regs] and allImps = [...fibImps, ...imps] can be out of order after concatenation; sort allRegs by r.diffPercent descending (worst-first) and sort allImps by r.diffPercent ascending (best-first) immediately after creating them so the markdown loop emits correctly ordered rows; update the code around the allRegs/allImps declarations to call .sort((a,b)=>b.diffPercent-a.diffPercent) for regressions and .sort((a,b)=>a.diffPercent-b.diffPercent) for improvements before generating the md.
🤖 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/compare.ts`:
- Around line 164-167: The current fibResult ternary silently skips comparison
when fibonacci data is present in only one suite; change it to explicitly handle
three cases: (1) if both baselineSuite.fibonacci and resultsSuite.fibonacci
exist, call compareBenchmarks(baselineSuite.fibonacci, resultsSuite.fibonacci,
threshold) as before; (2) if only baselineSuite.fibonacci exists, produce a
fibResult that lists each baseline Fibonacci benchmark as a "missing in results"
comparison (populate comparisons and regressions arrays accordingly) and (3) if
only resultsSuite.fibonacci exists, produce a fibResult that lists each result
Fibonacci benchmark as a "new in results" comparison (populate comparisons and
improvements arrays accordingly); keep the fibResult shape ({ comparisons,
regressions, improvements }) so downstream code consumes it unchanged and
reference compareBenchmarks, baselineSuite.fibonacci, resultsSuite.fibonacci and
fibResult when making the changes.
---
Duplicate comments:
In `@bench/compare.ts`:
- Around line 332-346: The merged arrays allRegs = [...fibRegs, ...regs] and
allImps = [...fibImps, ...imps] can be out of order after concatenation; sort
allRegs by r.diffPercent descending (worst-first) and sort allImps by
r.diffPercent ascending (best-first) immediately after creating them so the
markdown loop emits correctly ordered rows; update the code around the
allRegs/allImps declarations to call .sort((a,b)=>b.diffPercent-a.diffPercent)
for regressions and .sort((a,b)=>a.diffPercent-b.diffPercent) for improvements
before generating the md.
ℹ️ 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 (2)
assembly/memory.tsbench/compare.ts
mateuszsikora
approved these changes
Mar 4, 2026
Benchmark Results✅ No regressions (improvements detected)
Improvements
All traces
|
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
2 participants
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.
No description provided.