Conversation
There was a problem hiding this comment.
Pull request overview
Refactors BigInt.sqrt() to a faster CPython-style precision-doubling implementation, adding native UInt64/UInt128 early-iteration fast paths and word-list magnitude operations to reduce allocation and wrapper overhead. Also updates benchmark task wiring and documentation references.
Changes:
- Replaces the previous
sqrt()strategy with an optimized precision-doubling implementation (_sqrt_precision_doubling_fast) including UInt64/UInt128 phases and a word-list phase. - Adjusts Pixi bench tasks to rely on a unified
benchentry point and keeps only debug-assert bench variants. - Updates docs/changelog wording and benchmark command references.
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| src/decimojo/bigint/exponential.mojo | Major sqrt() refactor: adds word-list helpers + optimized precision-doubling implementation with native fast paths. |
| pixi.toml | Simplifies/changes bench tasks to a single bench task and debug variants. |
| docs/readme_unreleased.md | Updates benchmark command reference (pixi run bench). |
| docs/changelog.md | Changelog formatting/content adjustments (incl. section headings). |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| # --- Phase 2: Word-list operations (no BigInt wrapper overhead) --- | ||
| var a_words = _uint128_to_words(a128) | ||
|
|
||
| for s in range(phase15_end, -1, -1): | ||
| var e = d | ||
| d = c >> s | ||
|
|
||
| var shift_a = d - e - 1 | ||
| var shift_n = 2 * c - e - d + 1 | ||
|
|
||
| var a_shifted = decimojo.bigint.arithmetics.left_shift(a, shift_a) | ||
| var n_shifted = decimojo.bigint.arithmetics.right_shift(x, shift_n) | ||
| var quotient = n_shifted // a | ||
| a = a_shifted + quotient | ||
| # Shift x right (skips lower words efficiently) | ||
| var n_shifted = _right_shift_magnitude_bits(x.words, shift_n) | ||
|
|
||
| # Divide n_shifted by current a (before shifting) | ||
| var div_result = decimojo.bigint.arithmetics._divmod_magnitudes( | ||
| n_shifted, a_words | ||
| ) | ||
|
|
||
| # Final adjustment: a - (1 if a*a > x else 0) | ||
| var a_sq = a * a | ||
| if a_sq > x: | ||
| a -= BigInt(1) | ||
| # Shift a left, then add quotient in-place (saves 2 allocations) | ||
| a_words = _left_shift_magnitude_bits(a_words, shift_a) | ||
| decimojo.bigint.arithmetics._add_magnitudes_inplace( | ||
| a_words, div_result[0] | ||
| ) | ||
|
|
There was a problem hiding this comment.
The refactor adds a new Phase 2 path that uses raw word-list shifting/division/addition. The existing BigInt sqrt tests appear to cover small inputs and the UInt64/UInt128 fast paths, but there’s no test that forces execution into this Phase 2 loop (large enough bit-length where e+d exceeds the UInt128 threshold). Please add at least one large-integer test (ideally cross-checked against Python math.isqrt) that guarantees Phase 2 runs, to prevent regressions in the new word-list code paths.
| # bench | ||
| bench = "pixi run package && bash benches/run_bench.sh" | ||
| bint = "clear && pixi run package && cd benches/bigint && pixi run mojo run -I ../ bench.mojo && cd ../.. && pixi run clean" | ||
|
|
||
| # bench with debug assertions enabled | ||
| bdec_debug = "clear && pixi run package && cd benches/bigdecimal && pixi run mojo run -I ../ -D ASSERT=all bench.mojo && cd ../.. && pixi run clean" | ||
| bint_debug = "clear && pixi run package && cd benches/bigint && pixi run mojo run -I ../ -D ASSERT=all bench.mojo && cd ../.. && pixi run clean" | ||
| buint = "clear && pixi run package && cd benches/biguint && pixi run mojo run -I ../ bench.mojo && cd ../.. && pixi run clean" | ||
| buint_debug = "clear && pixi run package && cd benches/biguint && pixi run mojo run -I ../ -D ASSERT=all bench.mojo && cd ../.. && pixi run clean" | ||
| dec = "clear && pixi run package && cd benches/decimal128 && pixi run mojo run -I ../ bench.mojo && cd ../.. && pixi run clean" | ||
| dec_debug = "clear && pixi run package && cd benches/decimal128 && pixi run mojo run -I ../ -D ASSERT=all bench.mojo && cd ../.. && pixi run clean" |
There was a problem hiding this comment.
This change removes the non-debug bench tasks (e.g., bdec, bint, buint, dec). There are still docs in the repo that reference pixi run bdec / pixi run bint (e.g., docs/readme_zht.md, docs/plans/bigint2_benchmark_analysis.md), so those instructions will break after this PR. Either keep these task aliases for backward compatibility or update the remaining docs to the new pixi run bench flow.
Refactors
BigInt.sqrt()to a faster CPython-style precision-doubling implementation, adding native UInt64/UInt128 early-iteration fast paths and word-list magnitude operations to reduce allocation and wrapper overhead. Also updates benchmark task wiring and documentation references.Changes:
sqrt()strategy with an optimized precision-doubling implementation (_sqrt_precision_doubling_fast) including UInt64/UInt128 phases and a word-list phase.benchentry point and keeps only debug-assert bench variants.