JSBigInt::parseInt: O(n) fast path for power-of-two radix - #353
Conversation
The generic parse loop calls multiplyAdd over the full preallocated digit vector for every lengthLimitForBigInt32-sized group of input characters (n/k groups times O(n) multiplyAdd = O(n^2)). For a power-of-two radix (0b/0o/0x string prefixes, and literal parsing for radix 2/4/8/16/32) no multiplication is needed at all: each character is a fixed number of bits that can be packed directly into the digit vector. This mirrors toStringBasePowerOfTwo, which is already linear in the output direction. 200k hex chars: ~360ms -> ~1ms (release), ~17s -> ~6ms (debug+ASAN). Short inputs that fit in an int32 fall through to the existing path so they keep producing BigInt32 where applicable.
|
Warning Review limit reached
Next review available in: 8 minutes 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 (1)
Comment |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
I didn't find any bugs, but this adds ~60 lines of bit-packing logic to a core JS runtime path (BigInt string parsing), so it's worth a human look.
What was reviewed:
- Traced the carry arithmetic across 64-bit digit boundaries for radix 8 (bitsInDigit 63→66→2, 62→65→1, 61→64→0 cycle) — shifts stay in-range and bits are preserved.
- Checked
digitIndexnever exceedsnumDigits(writes = ⌈totalBits/digitBits⌉ exactly), and that a zero top digit from a split MSB is stripped bynormalize()intryCreateFromImpl. - Confirmed leading-zero skip and the
initialLength > lengthLimitForBigInt32guard keep short inputs on the BigInt32-producing path; error paths match the generic loop.
Extended reasoning...
Overview
Adds an O(n) fast path inside JSBigInt::parseInt for power-of-two radixes (2/4/8/16/32), gated behind USE(BUN_JSC_ADDITIONS). Instead of the existing multiply-add loop (O(n²) over the preallocated digit vector), it packs each character's ctz(radix) bits directly into the Digit vector from LSB to MSB, carrying overflow bits across 64-bit boundaries. Short inputs (initialLength <= lengthLimitForBigInt32) fall through to the existing path so BigInt32 results are unchanged. The result is handed to the same tryCreateFromImpl used by the generic path, which normalizes trailing zero digits.
Security risks
None identified. Input is a bounded std::span<const CharType>; numDigits is capped against maxLength before resultVector.fill; digitIndex is bounded by numDigits (verified: total writes equal ⌈initialLength·bitsPerChar / digitBits⌉ in both the exact-multiple and remainder cases); all shifts are by amounts strictly less than the Digit width (max bitsInDigit before OR is digitBits-1, max right-shift is bitsPerChar-1 ≤ 4). No raw pointer arithmetic beyond what the existing path already does.
Level of scrutiny
High. This is a hot, spec-observable primitive (BigInt(string)) and the bit-carry logic — particularly the octal case where 3-bit chunks straddle 64-bit boundaries — is easy to get subtly wrong. It's confined to Bun builds via the #if guard, and the PR describes roundtrip + reference-construction tests in a companion bun PR, but the correctness of the arithmetic itself deserves human eyes rather than bot approval.
Other factors
I stepped through the carry cycle for radix 8 by hand and it checks out; leading zeros are stripped before initialLength is computed so numDigits isn't inflated, and any residual zero top digit (e.g. octal MSB '1' whose high bits spill into a new digit as 0) is handled by normalize() inside tryCreateFromImpl. Error handling (invalid char → SyntaxError, oversize → OOM) mirrors the generic path. Nothing looks wrong — deferring purely on complexity/criticality grounds.
Preview Builds
|
2 participants
Problem
BigInt("0x…")/BigInt("0b…")/BigInt("0o…")parse time is O(n²) in the string length.JSBigInt::parseIntroutes every radix through a loop that, for eachlengthLimitForBigInt32-character group, callsmultiplyAdd(resultVector.span(), multiplier, digit, …)over the full preallocated digit vector: n/k groups × O(n) = O(n²).The output direction is already linear (
toStringBasePowerOfTwo). V8 hasFromStringBasePowerOfTwofor the parse direction.Fix
For power-of-two radix, each character maps to exactly
ctz(radix)bits. Pack them directly into the digit vector from least-significant char to most-significant, carrying bits across 64-bit digit boundaries for octal. No multiplication. MirrorstoStringBasePowerOfTwo.Inputs short enough to fit in int32 fall through to the existing path so they keep producing BigInt32 where applicable. Invalid characters and the
maxLengthcap throw the same errors as before.Result
debug+ASAN:
Correctness verified by roundtripping
BigInt(prefix+s).toString(radix) === sacross hex/binary/octal at all alignment boundaries (1..70 chars for octal to exercise the 3-bit span across 64-bit digit boundaries), and by comparing against digit-by-digitr = r*radix + dreference construction.Bun-side test:
test/js/bun/jsc/bigint-parse.test.ts(in the companion bun PR).