perf: route safeDiv through divLimbsDirect (not the builtin u256 /)#107
Conversation
safeDiv fell back to the builtin u256 '/', which lowers to a slow software long-division on aarch64 (~330ns for a 256/128-bit divide). The library already ships the fast limb path -- divLimbsDirect (Knuth D over [4]u64 with div128by64 = 2 hardware UDIVs), used by mulDiv and the dex math. Route safeDiv through it so every caller of the public division helper gets the fast path (tens of ns). No behavior change: divLimbsDirect returns the same quotient as '/'. Full suite passes (856 tests) incl. the safeDiv + divLimbsDirect cases.
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
📝 WalkthroughWalkthrough
ChangesSafe division
Estimated code review effort: 2 (Simple) | ~10 minutes Possibly related issues
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
There was a problem hiding this comment.
🧹 Nitpick comments (1)
src/uint256.zig (1)
90-92: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winAdd multi-limb regression coverage for
safeDiv.The current tests only exercise the single-limb divisor path (
6 / 3). Add parity cases against builtina / bcovering 2–4-limb operands,a < b,a == b,MAX, and divisors near limb boundaries so regressions indiv2by2orknuthDivCoreare detected.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/uint256.zig` around lines 90 - 92, Add regression tests for safeDiv covering 2–4-limb operands and comparing results with builtin a / b, including a < b, a == b, MAX, and divisors near limb boundaries. Reuse the existing test structure and ensure cases exercise both div2by2 and knuthDivCore paths.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@src/uint256.zig`:
- Around line 90-92: Add regression tests for safeDiv covering 2–4-limb operands
and comparing results with builtin a / b, including a < b, a == b, MAX, and
divisors near limb boundaries. Reuse the existing test structure and ensure
cases exercise both div2by2 and knuthDivCore paths.
What
uint256.safeDivwasreturn a / b;— the builtinu256division, which lowers to a slow software long-division on aarch64 (measured ~330 ns for a 256/128-bit divide). This routes it through the limb-baseddivLimbsDirectthe library already ships and thatmulDiv/ the dex math already use (Knuth Algorithm D over[4]u64withdiv128by64= 2 hardware UDIVs).Why it matters
safeDivis the public division helper, so every caller silently paid the slow path while the fast implementation sat one call away. Found while chasing a Uniswap TickMath hotspot in a downstream SDK: the positive-tickmaxInt/ratiodivide dominated the function, and swapping the builtin/fordivLimbsDirectwas a ~3× win on that op alone (and ~6.5× on the whole function combined with limb multiply).Safety
No behavior change —
divLimbsDirectreturns the same quotient as/(it's already the tested primitive behindmulDiv). The full suite passes: 856/856, including thesafeDivanddivLimbsDirectcases.zig fmt --checkclean.Follow-up (not in this PR)
The internal
divLimbs(used nowhere on the public path now) still callsknuthDivCoredirectly, skippingdivLimbsDirect's 2-limb register fast path. Worth aligning if it gains callers.Summary by CodeRabbit