Bench modules named on the command line - #32
Conversation
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
a10320c to
f46df6a
Compare
|
@codex review |
|
Codex Review: Didn't find any major issues. More of your lovely PRs please. Reviewed commit: ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
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". |
There was a problem hiding this comment.
🟡 Changes recommended
The dynamic module loader currently uses pathToFileURL(path) directly, which throws on relative paths, making common CLI invocations fail.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Pull request overview
Updates the benchmark runner so it can compare multiple builds of this package in a single rotation by naming module files on the command line, avoiding mutation of dist/ and keeping all variants in one counterbalanced shuffle.
Changes:
- Add a CLI mode that benchmarks explicit module paths (paths override
TARGET) and updates the run header to reportFILES=n. - Introduce an
Adapter.setup()hook (awaited once before measuring) and a dynamic adapter that loads a module-by-path and uses its documentedcreateHash()export. - Add a browser shim + rollup aliases for
node:urlto keep browser bundles self-contained.
File summaries
| File | Description |
|---|---|
| test/utils/adapters.ts | Adds Adapter.setup(), simplifies own adapter chaining, and introduces dynamicModule(path) to load benchmark targets from file paths. |
| builder/bench.cli.ts | Accepts module paths from process.argv, preloads adapters via setup(), and switches header output to FILES= when paths are provided. |
| builder/rollup-test.config.ts | Aliases node:url to a local shim for browser test bundling. |
| builder/rollup-bench.config.ts | Aliases node:url to a local shim for browser benchmark bundling. |
| builder/node-url.shim.ts | Adds a throwing browser stand-in for pathToFileURL. |
Review details
- Files reviewed: 5/5 changed files
- Comments generated: 2
- Review effort level: Lite
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
| override async setup(): Promise<void> { | ||
| const module = await import(pathToFileURL(path).href) as {createHash?: ICreateHash} |
| export const pathToFileURL = (): never => { | ||
| throw new Error("pathToFileURL() not supported") | ||
| } |
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, each behind a class of its own so the compared builds do not meet at shared call sites. Co-authored-by: Claude <noreply@anthropic.com>
f46df6a to
2f7a53b
Compare
Every argument is a module to import, so `-h` was resolved as a filename and reported as a missing module. Print the invocation and the three environment variables instead, and refuse anything else that leads with a dash rather than trying to load it. Co-authored-by: Claude <noreply@anthropic.com>
Co-authored-by: Claude <noreply@anthropic.com>
|
@codex review |
There was a problem hiding this comment.
🔵 Needs a closer look
Relative module paths passed on the command line can be converted into incorrect root-based file URLs in dynamicModule(), causing imports to fail for valid paths like dist/....
Review details
Suppressed comments (1)
test/utils/adapters.ts:290
- dynamicModule() builds the import specifier with pathToFileURL(path). For relative paths (e.g. the PR description’s example
dist/sha256-uint8array.mjs), this becomes a root-based file URL (e.g.file:///dist/...) rather than resolving against the current working directory, so the import can fail even when the file exists in the repo.
override async setup(): Promise<void> {
let module = await import(pathToFileURL(path).href)
if (!hasCreateHash(module)) module = module?.default
- Files reviewed: 5/5 changed files
- Comments generated: 0 new
- Review effort level: Lite
|
Codex Review: Didn't find any major issues. Bravo. Reviewed commit: ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
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". |
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.
One class per path
Each path builds a class of its own, for the same reason the benchmark closures
are built per adapter: a single class instantiated per path would share its
methods, and every module's
createHash— along with the differently shapedhashes it returns — would meet at the same call sites. The effect measures below
the noise at these input sizes, but the rule already applies a few lines above
and is free to keep.
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.update()is published one input shape at a time, so this file describes thesame function with the union it already accepts at run time, rather than
narrowing at every call site. That widens only this file's view — the package
keeps publishing the overloads it always has, and neither
lib/nortypes/istouched.
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 failure modes behave — amissing path, a module without
createHash, and a module returning a wrongdigest.