perf(bigint): batch digits in to_string_radix for non-power-of-two radixes - #3830
Merged
Conversation
Collaborator
Coverage Report for CI Build 6545Coverage remained the same at 89.19%Details
Uncovered ChangesNo uncovered changes found. Coverage RegressionsNo coverage regressions found. Coverage Stats
💛 - Coveralls |
…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>
bobzhang
force-pushed
the
hongbo/perf-bigint-radix
branch
from
September 4, 2026 02:37
295a18c to
b7e0ab4
Compare
bobzhang
marked this pull request as ready for review
September 4, 2026 02:48
Contributor
There was a problem hiding this comment.
🟢 Approval recommended
The new chunked conversion mirrors the proven radix-10 approach, preserves existing radix validation via to_string, and is backed by focused new tests covering correctness and boundary cases.
Pull request overview
Improves BigInt.to_string(radix=…) performance for non-power-of-two radixes (excluding radix 10) by switching from per-digit full-BigInt division to a chunked, limb-by-limb conversion strategy using Int64 arithmetic, aligning the generic radix path with the existing radix-10 approach.
Changes:
- Replaced the generic non-power-of-two radix conversion with a chunked (
chunk = radix^chunk_len) limb-by-limb algorithm and pre-sized output buffer emission. - Added correctness tests covering known values, round-trips across multiple non-power-of-two radixes, and boundary checks for radix-3 chunk sizing.
- Added benchmark tests for radix 7/36 (and radix 10 as reference) on a ~4000-bit value.
File summaries
| File | Description |
|---|---|
| bigint/bigint_default.mbt | Reworks to_string_radix generic path to emit multiple digits per chunk using Int64 division/mod and a pre-sized char buffer. |
| bigint/bigint_test.mbt | Adds targeted tests for non-power-of-two radix formatting correctness, including negatives and chunk-boundary cases. |
| bigint/to_string_radix_bench_test.mbt | Introduces benchmark coverage for the optimized radixes to track performance regressions/improvements. |
Review details
- Files reviewed: 3/3 changed files
- Comments generated: 0
- Review effort level: Lite
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
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.
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_divper output digit — O(n²) overall). This PR generalizes the limb-by-limb algorithm the radix-10 path already uses: convert into basechunk = radix^chunk_lenslots using onlyInt64arithmetic (withchunkthe largest power keeping(slot << RADIX_BIT_LEN) | limbinsideInt64), then emitchunk_lendigits per slot into a pre-sized buffer. The-selfmagnitude 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)
Tests
Known values for radixes 3/6/36 (incl. negatives),
from_stringround-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 andpos-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 checkclean,moon fmtapplied, no.mbtichangesmoon test: 6716 passed, 0 failed (bigint 157/157)🤖 Generated with Claude Code