JSBigInt: sub-quadratic multiply, divide, and toString - #354
Conversation
Large BigInt decimal conversion is quadratic: toStringGeneric repeatedly
divides by a one-digit chunk divisor, so an n-digit value takes ~n
divideSingle passes of ~n work each. Multiplication and exponentiation are
also quadratic because the only multi-digit multiply is schoolbook.
This adds three pieces:
multiplyKaratsuba: a three-way Karatsuba split that falls back to the
existing column-wise schoolbook below a 32-digit threshold, and handles
unbalanced inputs by halving the larger operand until the pair is
balanced. Wired into multiplyImpl and multiplyDigits so * and ** take
advantage immediately.
divideBurnikelZiegler: the Burnikel-Ziegler recursive division (1998),
entered via the same quotient/remainder-span interface as
divideTextbook. The public entry point pads the divisor up to a
multiple of a power of two and normalizes its top bit, so the mutual
recursion (bzD2n1n / bzD3n2n) halves cleanly down to a schoolbook base
case and never hits an odd width above the threshold. The n-by-n
product inside bzD3n2n goes through Karatsuba.
A divide-and-conquer toString path for inputs of 64 digits or more:
precompute chunkDivisor^(2^k) by repeated squaring, then split the
value into a quotient/remainder pair against the largest power that
fits, recursing on each half and finishing with the existing
schoolbook loop at the leaves. Characters are written right-to-left
into a pre-sized buffer so leading zeros are just a cursor adjustment.
With these in place multiplication, exponentiation and non-power-of-two
toString are all O(n^1.585) instead of O(n^2).
Release jsc, Linux x64, before/after (node v26 for reference):
before after node
200k-digit toString 365 ms 8.8 ms 13 ms
100k-digit toString 92 ms 2.9 ms 6.1 ms
100k x 100k multiply 26 ms 3.5 ms 1.9 ms
The scaling per doubling drops from x4.0 to x3.0 as expected for the
Karatsuba exponent.
|
Warning Review limit reached
Next review available in: 1 minute Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Organization UI Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (3)
Comment |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
Beyond the inline note, the review also examined whether qBufs[level] in toStringEmit can be undersized when the quotient branch recurses at the same level repeatedly — it can't, because qs is bounded by digits.size() - divisor.size() + 1 and each level's buffer is sized to dLen + 2 for that level's divisor. This is a large algorithmic change to core BigInt arithmetic with hand-managed scratch buffers and recursion invariants, so it warrants a human pass regardless.
Extended reasoning...
The only reported finding is a scope-clarification nit (Burnikel-Ziegler not wired into / and %). Separately, a finder raised a concern that qBufs could be undersized when toStringEmit's quotient branch stays at the same level across recursive calls; verification showed the quotient span from divideBurnikelZiegler is at most digits.size() - divisor.size() + 1 and the per-level buffer of dLen + 2 covers it, so that was ruled out. Given ~450 lines of new Karatsuba / Burnikel-Ziegler / divide-and-conquer-toString code with manual scratch-space bounds, span aliasing, and carry/borrow propagation in the JSC runtime, deferring to a human reviewer.
| // Drop-in for divideTextbook that switches to Burnikel-Ziegler above the | ||
| // threshold. Accepts the same optional-q / optional-r convention. | ||
| std::tuple<std::span<JSBigInt::Digit>, std::span<JSBigInt::Digit>> JSBigInt::divideBurnikelZiegler(std::span<Digit> q, std::span<Digit> r, std::span<const Digit> a, std::span<const Digit> b) | ||
| { | ||
| size_t s = b.size(); | ||
| ASSERT(s >= 2); | ||
| ASSERT(a.size() >= s); | ||
| if (s < burnikelZieglerThreshold) | ||
| return divideTextbook(q, r, a, b); |
There was a problem hiding this comment.
🟡 The comment describes divideBurnikelZiegler as a "drop-in for divideTextbook", but it's only called from toStringEmit — divideImpl (line 2008), remainderImpl (line 2473), and divideDigits (line 2136) still call divideTextbook directly, so a / b and a % b on large BigInts remain O(n²). Since multiplyKaratsuba was wired into multiplyImpl/multiplyDigits, was leaving these on the schoolbook path intentional scoping or an oversight? (No correctness issue either way — just checking whether the PR title's "divide" was meant to cover the operators too.)
Extended reasoning...
What this is
divideBurnikelZiegler is introduced with the comment "Drop-in for divideTextbook that switches to Burnikel-Ziegler above the threshold. Accepts the same optional-q / optional-r convention." It has the identical signature to divideTextbook and internally falls back to divideTextbook when s < burnikelZieglerThreshold, so it is safe to substitute at any call site. However, only one caller actually uses it.
Call-site walkthrough
Grepping for divideTextbook( on the PR head:
divideImplat JSBigInt.cpp:2008 — backs the JS/operator:auto [qSpan, rSpan] = divideTextbook(q.mutableSpan(), { }, xSpan, ySpan);remainderImplat JSBigInt.cpp:2473 — backs the JS%operator:rSpan = std::get<1>(divideTextbook({ }, r.mutableSpan(), xSpan, ySpan));divideDigitsat JSBigInt.cpp:2136 — the span-level helper analogous tomultiplyDigits:auto [quotientSpan, remainderSpan] = divideTextbook(quotient, { }, x, y);cachedModMakeInverseat line 2254 — divisor is capped atmaxCachedModDivisorSize = 32digits, so BZ would never trigger; irrelevant.bzD2n1nbase case at line 1820 and thedivideBurnikelZieglersmall-input fallback at line 1848 — intentional schoolbook base cases.
The only caller of divideBurnikelZiegler is toStringEmit at line 3802.
Why this looks like it might be an oversight
The PR wires the new multiply into both the operator path and the span helper: multiplyImpl gained a y.size() >= karatsubaThreshold branch, and multiplyDigits gained the same. The symmetric change for divide — swapping divideTextbook for divideBurnikelZiegler in divideImpl, remainderImpl, and divideDigits — was not made, even though the new function is expressly a drop-in and self-dispatches below the threshold. Combined with the PR title "sub-quadratic multiply, divide, and toString", this reads like the operator wiring for divide may have been forgotten.
Why it might be intentional
The PR description body is more careful than the title: it says "multiplication, exponentiation and non-power-of-two toString are all O(n^1.585)" — it does not claim / and % are sub-quadratic, and the benchmark table has no division row. So the author may have deliberately scoped BZ to toString (where the divisor sizes are controlled by the repeated-squaring ladder and thoroughly exercised by the new stress test) and left operator division for a follow-up. In that reading, "divide" in the title refers to the divide algorithm being added, not to the / operator being accelerated.
Impact
None on correctness. divideTextbook produces correct results; a / b and a % b on large BigInts continue to work exactly as before, just at O(n²). If the omission is intentional, the only actionable item is possibly tightening the PR title. If it was an oversight, the fix is a three-line swap of divideTextbook → divideBurnikelZiegler at lines 2008, 2136, and 2473 (plus a couple of / and % cases in the stress test).
Severity
nit — this is a missed-optimization / scope-clarification question, not a bug. Nothing breaks if the PR merges as-is.
Preview Builds
|
2 participants
Large-BigInt decimal conversion is quadratic:
toStringGenericrepeatedly divides by a one-digit chunk divisor, so an n-digit value takes ~ndivideSinglepasses of ~n work each. Multiplication and exponentiation are also quadratic because the only multi-digit multiply is schoolbook.This adds three pieces:
multiplyKaratsuba: a three-way Karatsuba split that falls back to the existing column-wise schoolbook below a 32-digit threshold, and handles unbalanced inputs by halving the larger operand until the pair is balanced. Wired intomultiplyImplandmultiplyDigitsso*and**take advantage immediately.divideBurnikelZiegler: the Burnikel-Ziegler recursive division (1998), entered via the same quotient/remainder-span interface asdivideTextbook. The public entry point pads the divisor up to a multiple of a power of two and normalizes its top bit, so the mutual recursion (bzD2n1n/bzD3n2n) halves cleanly down to a schoolbook base case and never hits an odd width above the threshold. The n-by-n product insidebzD3n2ngoes through Karatsuba.Divide-and-conquer
toStringfor inputs of 64 digits or more: precomputechunkDivisor^(2^k)by repeated squaring, split the value into a quotient/remainder pair against the largest power that fits, recurse on each half, and finish with the existing schoolbook loop at the leaves. Characters are written right-to-left into a pre-sized buffer so leading zeros are just a cursor adjustment.With these in place multiplication, exponentiation and non-power-of-two
toStringare all O(n^1.585) instead of O(n^2).Release jsc, Linux x64, best of three (node v26.3.0 for reference):
toString(10)toString(10)toString(10)The scaling per doubling drops from x4.0 to x2.9 as expected for the Karatsuba exponent. V8's multiply is still ahead because it also has Toom-3 and FFT above Karatsuba.
JSTests/stress/big-int-subquadratic-stringify-multiply.jsexercises the new paths: decimal and arbitrary-radix round-trips across the 32/64-digit thresholds, algebraic multiply identities with balanced and unbalanced operands, and power-of-ten/power-of-two inputs whose long zero runs hit the remainder-is-zero handling inside Burnikel-Ziegler.