Skip to content

perf(bigint): batch digits in to_string_radix for non-power-of-two radixes - #3830

Draft
bobzhang wants to merge 1 commit into
mainfrom
hongbo/perf-bigint-radix
Draft

perf(bigint): batch digits in to_string_radix for non-power-of-two radixes#3830
bobzhang wants to merge 1 commit into
mainfrom
hongbo/perf-bigint-radix

Conversation

@bobzhang

Copy link
Copy Markdown
Contributor

Closes #3827.

The generic radix path (radixes 3, 5, 6, 7, ... — anything not 10 or a power of two) extracted one digit per full BigInt division (grade_school_div per output digit — O(n²) overall). This PR generalizes the limb-by-limb algorithm the radix-10 path already uses: convert into base chunk = radix^chunk_len slots using only Int64 arithmetic (with chunk the largest power keeping (slot << RADIX_BIT_LEN) | limb inside Int64), then emit chunk_len digits per slot into a pre-sized buffer. The -self magnitude copy for negative inputs is gone too — limbs are sign-magnitude and are read directly, as radix-10 already does.

Benchmarks (native release, ~4000-bit value; committed)

radix before after
7 1.28 ms 83 µs ~15×
36 762 µs 95 µs ~8×
10 (reference, unchanged) ~105 µs ~105 µs now matched by all radixes

Tests

Known values for radixes 3/6/36 (incl. negatives), from_string round-trips for ten non-power-of-two radixes on a 700+ bit value (positive and negative), and radix-3 digit-length checks across the chunk boundaries (3^e for e ∈ {1, 18, 19, 20, 37, 38, 39} — 19 digits per chunk).

Review

Reviewed by Codex CLI (codex-cli 0.144.1): "Approved; no findings" — including an explicit overflow-bound derivation (y ≤ chunk·2³² − 2³² + 2³² − 1 < Int64.max), slot-count and pos-underflow verification, sign-magnitude equivalence for negatives, and an independent 7,650-case differential check across all 30 applicable radixes.

Signed-off-by: Codex CLI codex@openai.com

Validation

  • moon check clean, moon fmt applied, no .mbti changes
  • moon test: 6716 passed, 0 failed (bigint 157/157)

🤖 Generated with Claude Code

…dixes

Closes #3827. The generic radix path extracted one digit per full
BigInt division (grade_school_div per output digit, O(n^2) overall).
Generalize the radix-10 algorithm already used by to_string: convert
limb-by-limb into base chunk = radix^chunk_len slots using only Int64
arithmetic, with chunk chosen as the largest power keeping
(slot << RADIX_BIT_LEN) | limb inside Int64, then emit chunk_len
digits per slot into a pre-sized buffer. Also drops the -self
magnitude copy the old path made for negative inputs.

Native release benchmarks on a ~4000-bit value (committed):
- radix=7:  1.28 ms -> 83 us (~15x)
- radix=36: 762 us -> 95 us (~8x)
- both now match the optimized radix-10 path (~105 us) as expected

New tests: known values for radixes 3/6/36, from_string round-trips
for ten non-power-of-two radixes on a 700+ bit value (positive and
negative), and radix-3 digit-length checks across chunk boundaries
(3^e for e around 19 and 38).
Reviewed by Codex CLI (codex-cli 0.144.1): "Approved; no findings" —
with an explicit Int64 overflow bound derivation, slot-count and
pos-underflow checks, sign-magnitude equivalence for negatives, and an
independent 7,650-case differential check across all 30 applicable
radixes.

Signed-off-by: Codex CLI <codex@openai.com>
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
@coveralls

Copy link
Copy Markdown
Collaborator

Coverage Report for CI Build 5187

Coverage increased (+0.02%) to 91.288%

Details

  • Coverage increased (+0.02%) from the base build.
  • Patch coverage: 15 of 15 lines across 1 file are fully covered (100%).
  • No coverage regressions found.

Uncovered Changes

No uncovered changes found.

Coverage Regressions

No coverage regressions found.


Coverage Stats

Coverage Status
Relevant Lines: 17217
Covered Lines: 15717
Line Coverage: 91.29%
Coverage Strength: 191028.46 hits per line

💛 - Coveralls

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

bigint: to_string_radix is O(n²) for non-power-of-two radixes other than 10

2 participants