perf(matmath): hoist EPSILON out of invert_3x3's per-call path - #165
Merged
Conversation
bench/README.md carried matmath.invert_3x3 as an open finding: jsfeat faster in 10 of 10 samples, 1.18-1.40x, never flipping. Profiling attributed it entirely to the singular-matrix check that #120 ADDED -- a line original jsfeat does not have at all, so this was never a porting drift. Isolating the two halves of that line, in ops/s: jsfeatNext as written (Math.abs + JSFEAT_CONSTANTS.EPSILON) 9.35M drop Math.abs only 10.01M (+7%) hoist EPSILON to module scope only 13.36M (+43%) both 14.57M jsfeat (no check at all) 12.47M The object property load dominates, not Math.abs: the surrounding arithmetic is only ~30 float operations, so one property read per call is not negligible. Fixed both -- EPSILON hoisted to a module-scope const, and the two-sided compare `det < EPSILON && det > -EPSILON` replacing `Math.abs(det) < EPSILON`. Semantics are unchanged, including the edge cases: NaN yields 1 under both forms (NaN comparisons are always false), and -0 yields 0 under both. The 265 parity/property tests stay green, unchanged. After the fix, eight idle-machine runs: 1.03 / 2.20 / 1.01 / 1.09 / 1.15* / 1.14 / 1.09 / 1.04 (* = jsfeatNext faster). Seven of eight sit at or below the noise floor and the sign now flips, where before it was 10/10 and never below 1.18. The 2.20 is a single outlier, reported rather than discarded. The residual ~1.05x is method-dispatch overhead the standalone probe did not have: the probe timed a plain function, the shipped path goes through jsfeatNext.matmath.invert_3x3. 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
Optimizes matmath.invert_3x3 while preserving semantics and documents updated benchmark results.
Changes:
- Hoists
EPSILONto module scope. - Replaces
Math.abswith an equivalent two-sided comparison. - Updates profiling and post-fix benchmark documentation.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.
| File | Summary |
|---|---|
src/matmath/matmath.ts |
Optimizes the determinant check; scope the performance claim to the local optimization. |
bench/README.md |
Documents benchmark results; remove stale pre-fix results and wording. |
💡 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 #165. bench/README.md marked invert_3x3 as both resolved and unresolved: the earlier table still tagged it "real" at 1.18-1.36 and the paragraph below still said "ten out of ten, never once flipping ... recorded as an open finding", directly under the box announcing the fix. The table's numbers are still valid as history -- they predate this fix -- so they are kept and relabelled rather than deleted, and the paragraph now states the finding was real and has since been fixed. The source comment claimed the change "makes invert_3x3 faster than original jsfeat". That holds only for the standalone probe; on the shipped path, through the matmath singleton, jsfeat is still marginally ahead (~1.05x) on method-dispatch overhead. Reworded to describe what the change actually does locally -- removing a per-call property load and function call -- and to leave the end-to-end numbers to bench/README.md. 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.mdcarriedmatmath.invert_3x3as an open finding: jsfeat faster in 10 of 10 samples, 1.18–1.40x, never flipping. Profiling attributed it entirely to the singular-matrix check that #120 added — a line original jsfeat does not have at all, so this was never porting drift.Attribution
Isolating the two halves of
return Math.abs(det) < JSFEAT_CONSTANTS.EPSILON ? 0 : 1;:Math.absonlyEPSILONto module scope onlyThe object property load dominates, not
Math.abs. The surrounding arithmetic is only ~30 float operations, so one property read per call is not negligible — and with both fixed the standalone form is faster than jsfeat, which does no singular check at all.The fix
EPSILONhoisted to a module-scopeconst, andMath.abs(det) < EPSILONreplaced by the two-sided comparedet < EPSILON && det > -EPSILON.Semantics unchanged, edge cases included:
NaNyields1under both forms (all NaN comparisons are false),-0yields0under both. The 265 parity/property tests stay green, unchanged.Result
Eight idle-machine runs after the fix:
1.03 / 2.20 / 1.01 / 1.09 / 1.15* / 1.14 / 1.09 / 1.04(* = jsfeatNext faster)Seven of eight sit at or below the ~1.15x noise floor, and the sign now flips — where before it was 10/10 jsfeat and never below 1.18. The 2.20 is a single outlier, reported rather than discarded.
The residual ~1.05x is method-dispatch overhead the standalone probe didn't have: the probe timed a plain function, the shipped path goes through
jsfeatNext.matmath.invert_3x3.Two things this investigation also established
A hypothesis was disproven, not confirmed. The same profiling session tested
bench/README.md's long-standing YAPE hypothesis — that importinghessian_min_eigen_valueacross modules blocks V8 inlining. The profile shows 96.6% ofyape06.detect's time indetectitself and ~0.6% in the helpers, and the two implementations' inner loops are character-for-character identical. The hypothesis does not hold; YAPE's cause is still unknown. (Not changed in this PR — flagging it so the README's wording gets revisited.)A misattribution was caught.
linalg.lu_solve— the finding #159 explicitly did not explain — also readsJSFEAT_CONSTANTS.EPSILONinside a loop, which looked like the same bug. It isn't: jsfeat's equivalent readsjsfeat.EPSILON, also a property load. Both sides pay the same cost there, so it explains nothing.lu_solveremains open.Verification
npm test: 265 passed, unchanged.tsc --noEmit,prettier --check,license-check(93 files) all clean. Benches measured on an idle machine, warm-up discarded, eight samples.Refs #86, #120.
🤖 Generated with Claude Code