perf(yape): alias imported helpers to module-scope consts - #166
Merged
Conversation
bench/README.md carried the YAPE detectors as an open finding (8/8 jsfeat, roughly 1.3x) with the hypothesis that importing hessian_min_eigen_value across modules blocked V8 inlining. Profiling refuted that hypothesis: 96.6% of yape06.detect's time is in detect itself and ~0.6% in the helpers, and the two implementations' inner loops are character-for-character identical. A structural probe then replicated detect's body verbatim under four shapes, asserting identical corner counts first. Two effects reproduced across three clean runs: a class method is slower than an object-literal property, and calling an ESM imported binding from a hot loop is slower than calling a plain module-scope const holding the same function -- imported bindings are live, so each access carries an indirection a const does not. The second gained 12-28% every run and is a one-line change; this commit applies it. The result is a split, and only half the finding is fixed: yape 8/8 jsfeat, 1.23-1.53 -> 1.02* / 1.05* / 1.14 / 1.06* RESOLVED yape06 8/8 jsfeat, 1.11-1.45 -> 1.14 / 1.47 / 1.41 / 1.24 unchanged (* = jsfeatNext faster that run.) That split agrees with the profile rather than contradicting it. yape06 calls compute_laplacian once per frame and hessian_min_eigen_value only for candidate pixels -- which is precisely why the helpers measured 0.6%. yape's helpers sit in the per-pixel loop, so aliasing them matters there. yape06's alias is kept for consistency and costs nothing, but it measured no benefit and is not claimed to help. yape06's cause remains unknown; the class-vs-object-literal effect the probe also showed is the untested candidate. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
Contributor
There was a problem hiding this comment.
Pull request overview
This PR targets a measured performance overhead in the YAPE detectors by avoiding repeated access to ESM imported live bindings in hot paths, replacing those calls with module-scope const aliases that reference the same helper functions. It also updates the benchmark documentation to reflect the refuted prior hypothesis and the observed split outcome between yape and yape06.
Changes:
- Aliases imported helper functions to module-scope
constreferences inyapeandyape06, and routes hot-loop calls through those aliases. - Updates
bench/README.mdto document the refuted inlining hypothesis and the post-change benchmark split (yaperesolved,yape06still open).
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| src/yape06/yape06.ts | Adds module-scope helper aliases and switches call sites; introduces a perf rationale comment that needs accuracy fixes. |
| src/yape/yape.ts | Adds module-scope helper aliases and switches call sites; currently splits imports with runtime statements (needs reordering). |
| bench/README.md | Rewrites the YAPE finding section to reflect new measurements and current status. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
Two issues from Copilot review of #166. yape.ts had its import declarations split by the new alias consts, which breaks the repo's grouped-imports pattern and would trip `import/first`. Imports are contiguous again, with the aliases below them. The yape06.ts comment was inaccurate in two ways and contradicted this PR's own README changes: it called both helpers "per-pixel" when compute_laplacian runs once per frame, and it quoted a 12-28% gain that came from the probe -- while bench/README.md states plainly that the alias measured NO benefit for yape06. Rewritten to say what is actually true here: the effect is real where a helper runs per pixel (it resolved yape), it does not apply to yape06's call frequencies, and the alias is kept for consistency rather than speed. Specific deltas are left to bench/README.md so they cannot go stale in a source comment. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
kalwalt
added a commit
that referenced
this pull request
Aug 22, 2026
bench/README.md had grown into a 677-line chronological log: six sections, each with its own status box. Working out "where do things stand today" required reading all of them and assembling the answer, and every new measurement had to be threaded into six places -- which is exactly how it drifted out of date. Added a Current status table at the top, measured over four full-suite runs on merged dev (after #159, #165 and #166). It states plainly that the sections below are the historical record and their numbers are NOT updated, so future measurements touch one table instead of six sections. Three verdicts were stale against the merged-dev measurements: - yape06 was recorded as "unchanged, still open" at 1.14-1.47. On merged dev it measures ~1.04 across four runs. Flagged as resolved but explicitly UNEXPLAINED: #166's alias alone did not produce this, so something in the combination of the three fixes did, and it is not credited to any one of them. - get_gaussian_kernel size 7 was described as "mostly below floor", with a following sentence asserting everything else sat at or below ~1.15x. The later series puts it at 1.15-1.23, four of four favouring jsfeat -- at or just above the floor, not below it. Both spots corrected and it is carried as open. - lu_solve's verdict was right but its numbers were superseded: 1.43-1.49 with a +/-0.03 spread, now the tightest signal in the suite, tighter than YAPE ever was. It has never been profiled, and #159 predicted in advance that its fix could not explain it. Documentation only -- no source or bench code touched. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
This was referenced Aug 22, 2026
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
bench/README.mdcarried the YAPE detectors as an open finding — 8/8 samples favouring jsfeat, roughly 1.3x — with the standing hypothesis that importinghessian_min_eigen_valueacross modules blocked V8 inlining.That hypothesis was wrong. This PR replaces it with a measured cause, fixes half the finding, and says plainly which half is still open.
The hypothesis was refuted, not confirmed
Profiling
yape06.detect(300 iterations over a 640×480 frame, after warm-up):detectitselfand the two implementations' inner scan loops are character-for-character identical. If cross-module imports were blocking inlining, the helpers would dominate. They don't.
What the probe actually found
A throwaway probe replicated
detect's body verbatim under four shapes — asserting all four produced identical corner counts before timing anything:Two effects reproduced across three clean runs (one further run was discarded — every value roughly halved, the signature of CPU contention):
constholding the same function (variant 3 → 4) — +12–28%, every run. Imported bindings are live, so each access carries an indirection aconstdoes not.The second is a one-line change. This PR applies it to
yape06.tsandyape.ts.Result: a split, and only half is fixed
yapeyape06* = jsfeatNext faster in that run.
The split agrees with the profile rather than contradicting it.
yape06callscompute_laplacianonce per frame andhessian_min_eigen_valueonly for candidate pixels — which is exactly why the helpers measured 0.6% there.yape's helpers sit in the per-pixel loop, so aliasing them matters.yape06's alias is kept for consistency and costs nothing, but it measured no benefit and is not claimed to help. Its cause remains unknown; the class-vs-object-literal effect the probe also showed is the untested candidate — and testing it means changing module structure, which is a bigger decision than a one-line alias.Verification
npm test: 265 passed, unchanged — the aliases are pure references, no behavior change.tsc --noEmit,prettier --check,license-check(93 files) all clean. Benches measured on an idle machine, warm-up discarded.Refs #86.
🤖 Generated with Claude Code