Bench modules named on the command line - #31
Conversation
Comparing this package against itself — a published build, a branch, the working tree — meant swapping dist/ in place between runs, which mutates the tree and puts each build in its own process, out of reach of the counterbalanced ordering. Naming the files instead keeps every build in one rotation, and setup() gives an adapter somewhere to load them. Co-authored-by: Claude <noreply@anthropic.com>
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
|
@codex review |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: dd2edc7f75
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| hash(data: string | Uint8Array | ArrayBufferView): string { | ||
| const createHash = this.loaded | ||
| if (!createHash) throw new Error(`${this.path}: setup() not awaited`) | ||
| const hash = createHash() |
There was a problem hiding this comment.
When two or more paths are benchmarked, every DynamicModule instance shares this prototype method, so the shuffled measurements send different modules' createHash functions—and differently shaped hash objects through the later update/digest accesses—through the same V8 feedback sites. Unlike the per-adapter closures described above, this makes those sites polymorphic or megamorphic, so results can include cross-module inline-cache/deoptimization effects and vary with the number or order of compared files rather than isolating each build's hashing cost. Give each loaded module its own benchmark/hash closure so these hot call sites remain monomorphic.
Useful? React with 👍 / 👎.
The overloads name the three input shapes one at a time, so a caller holding a `string | Uint8Array` cannot hand it over: it has to narrow the value back down first, which the implementation then does again. Declare the union as well, and the extra branch inside the benchmark adapter goes away with it. Co-authored-by: Claude <noreply@anthropic.com>
One class instantiated per path shares its methods, so every module's createHash and the differently shaped hashes it returns meet at the same call sites — the megamorphic case the benchmark closures already avoid by building one per adapter. Follow that rule here too. The effect is below the noise at these input sizes, but the rule is cheap to keep. Co-authored-by: Claude <noreply@anthropic.com>
|
Superseded by #32, which carries the same feature on a fresh branch. This branch had grown a commit that widened |
2 participants
Comparing this package against itself — a published build, a branch build, the
working tree — used to mean copying a
.mjsoverdist/, running, and copyingit back. That mutates the tree, leaves a foreign build behind if the run is
interrupted, and puts each version in its own process, so the runner's
counterbalanced shuffle never sees them together.
Naming the files on the command line instead keeps every build in one rotation:
Paths outrank
TARGETand replace the comparison rather than filtering it, soevery cell measures a build of this package and only the code differs. The
header says
FILES=nin place ofTARGET=, since naming a target would be aclaim the run is not making.
Loading
Adaptergains asetup()hook, awaited once before any cell is built. Amodule load is a cost of the import, not of a digest, so it stays outside every
timed window. The default does nothing, so the existing adapters are unchanged.
The adapter expects the documented
createHash()entry point and says so if themodule lacks it. It is deliberately not a general adapter for arbitrary modules.
Correctness comes for free: the measured closures already assert each digest
against
crypto, so a bad build fails before the timer starts.Browsers
node:urljoinsnode:assertandnode:cryptoin the alias lists of bothbrowser configs, pointing at a shim that throws rather than returning a
plausible URL — the CLI-only path never runs in a browser, and a fake result
would hide a broken split instead of reporting one. Both bundles build without
warnings and carry no reference to
node:urlbeyond that shim's own comment.Verified on Node v24.19.0 and Chromium 151:
npm testgreen,browser/tests.html61 passes / 0 failures, and the four failure modes behave —a missing path, a module without
createHash, a module returning a wrongdigest, and a normal three-way comparison.