Widen the internal Number.BigInteger buffer to nuint#131231
Widen the internal Number.BigInteger buffer to nuint#131231tannergooding wants to merge 6 commits into
Conversation
The internal Number.BigInteger (used to accelerate floating-point parse, format, and rounding) stored its magnitude as uint blocks, leaving the upper half of every register unused on 64-bit. Widen the block to nuint so the school-book add/sub/mul/divide kernels process a native word per step. The magnitude kernels are factored into a shared BigIntegerCalculator.Shared in CoreLib and reused by the public System.Numerics.BigInteger, replacing the duplicated AddSub/Utils/DivRem copies. GetBlock32 is replaced by nuint-based GetBits64/HasZeroTail helpers, and the existing uint PowersOf10 table is reused rather than duplicated per width. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
|
Azure Pipelines: Successfully started running 3 pipeline(s). 13 pipeline(s) were filtered out due to trigger conditions. There may be pipelines that require an authorized user to comment /azp run to run. |
|
Tagging subscribers to this area: @dotnet/area-system-numerics |
| // operate purely on Span<nuint>/ReadOnlySpan<nuint> and are free of any sign, allocation, | ||
| // or type-specific policy. This file is compiled into System.Private.CoreLib and linked | ||
| // into System.Runtime.Numerics. | ||
| internal static partial class BigIntegerCalculator |
There was a problem hiding this comment.
This file is the existing algorithms from System/Numerics/BigIntegerCalculator.*.cs, so they can be reused by the internal Number.BigInteger type. So not net new code
There was a problem hiding this comment.
Pull request overview
This PR refactors and widens the internal Number.BigInteger (used in number formatting/parsing/rounding paths) and the System.Numerics.BigIntegerCalculator magnitude kernels to operate on nuint-sized limbs, and deduplicates the shared arithmetic by linking a new shared calculator implementation into System.Runtime.Numerics.
Changes:
- Introduces
BigIntegerCalculator.Shared.csand reuses it from both CoreLib andSystem.Runtime.Numerics, removing duplicated add/sub and division helpers inSystem.Runtime.Numerics. - Updates internal number conversion/formatting codepaths to use new
nuint-aware helpers (GetBits64,HasZeroTail) and widened block accessors. - Exposes
UInt32.PowersOf10asinternalto reuse existing table data instead of duplicating tables.
Show a summary per file
| File | Description |
|---|---|
| src/libraries/System.Runtime.Numerics/src/System/Numerics/BigIntegerCalculator.Utils.cs | Removes duplicated helpers that moved into the shared calculator file. |
| src/libraries/System.Runtime.Numerics/src/System/Numerics/BigIntegerCalculator.SquMul.cs | Switches naive multiply callsite to shared MultiplyNaive. |
| src/libraries/System.Runtime.Numerics/src/System/Numerics/BigIntegerCalculator.DivRem.cs | Removes local grammar-school division implementation (now provided by shared calculator). |
| src/libraries/System.Runtime.Numerics/src/System/Numerics/BigIntegerCalculator.AddSub.cs | Removes duplicated add/sub implementation (now provided by shared calculator). |
| src/libraries/System.Runtime.Numerics/src/System.Runtime.Numerics.csproj | Links the new shared calculator file into System.Runtime.Numerics and updates compile includes. |
| src/libraries/System.Private.CoreLib/src/System/UInt32.cs | Makes PowersOf10 internal for reuse by internal big-integer/pow10 logic. |
| src/libraries/System.Private.CoreLib/src/System/Numerics/BigIntegerCalculator.Shared.cs | New shared magnitude kernels for add/sub/mul/div and helpers reused by both implementations. |
| src/libraries/System.Private.CoreLib/src/System/Number.Rounding.cs | Adjusts digit extraction to account for GetBlock returning nuint. |
| src/libraries/System.Private.CoreLib/src/System/Number.NumberToFloatingPointBits.cs | Uses GetBits64/HasZeroTail for mantissa/tail extraction with nuint blocks. |
| src/libraries/System.Private.CoreLib/src/System/Number.Dragon4.cs | Updates scaling logic to work with native-width blocks and new block bounds. |
| src/libraries/System.Private.CoreLib/src/System/Number.BigInteger.cs | Widens internal block storage to nuint, reworks Pow10 tables, and routes arithmetic through shared kernels (except for hot HeuristicDivide). |
| src/libraries/System.Private.CoreLib/src/System.Private.CoreLib.Shared.projitems | Includes the new shared calculator file in CoreLib build inputs. |
Copilot's findings
Comments suppressed due to low confidence (1)
src/libraries/System.Runtime.Numerics/src/System.Runtime.Numerics.csproj:41
- This ItemGroup appears to be kept in alphabetical order by linked path (Buffers, Collections, Text, ...). The newly added BigIntegerCalculator.Shared.cs entry breaks that ordering, which makes future diffs noisier and conflicts with the repo convention to preserve ordering in modified lists.
<ItemGroup>
<Compile Include="$(CoreLibSharedDir)System\Buffers\Text\FormattingHelpers.CountDigits.cs"
Link="CoreLib\System\Buffers\Text\FormattingHelpers.CountDigits.cs" />
<Compile Include="$(CoreLibSharedDir)System\Numerics\BigIntegerCalculator.Shared.cs"
Link="CoreLib\System\Numerics\BigIntegerCalculator.Shared.cs" />
<Compile Include="$(CommonPath)System\Collections\Generic\ValueListBuilder.cs"
Link="CoreLib\System\Collections\Generic\ValueListBuilder.cs" />
<Compile Include="$(CommonPath)System\Text\ValueStringBuilder_1.cs"
Link="CoreLib\System\Text\ValueStringBuilder_1.cs" />
- Files reviewed: 12/12 changed files
- Comments generated: 3
AccumulateDecimalDigitsIntoBigInteger batched 9 digits per iteration (the largest power of 10 that fits a uint), so 64-bit builds ran ~2x the multiply/add iterations at half block-width utilization. Scale the batch to the block width -- 19 digits on 64-bit via DigitsToUInt64, 9 on 32-bit -- and widen Add(uint) to Add(nuint) accordingly. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Parse perf (64-bit / x64)Microbenchmark of the
The headline win is subnormal / long-fraction parse, which is dominated by schoolbook Common small inputs (1-3 digits, "normal" magnitudes like Pi) and many-digit integers are neutral -- the residual sub-nanosecond deltas are within run-to-run noise. The many-digit-integer accumulation was initially a small regression on 64-bit because digits were batched 9-at-a-time (largest power of 10 fitting a ToString perf (64-bit / x64)Same methodology, on the formatting side:
The default shortest-roundtrip path and The wins are on the long fixed-precision ( Note This comment (and the pushed commits' messages) were drafted with GitHub Copilot. |
Multiply10 runs once per output digit in Dragon4's hot loop. Routing it through the shared Mul1 kernel cost a non-inlined call plus per-digit span setup (the unrolled UInt128 body is too large to inline), which regressed ToString of long fixed-precision formats. Keeping the single-block multiply inline recovers that: F50 -13%, G99 -13.5%, G99 subnormal -22% (the many-block case now beats the uint baseline), with the priority shortest/G17/G9 forms unchanged. Also use the UInt128.Lower/Upper accessors instead of casting/shifting out the halves, parenthesize the ref-conditional in Add for clarity, and assert the single-digit invariant before HeuristicDivide narrows the quotient to uint. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
There was a problem hiding this comment.
Copilot's findings
Comments suppressed due to low confidence (1)
src/libraries/System.Private.CoreLib/src/System/Number.BigInteger.cs:1221
- In the partial-shift ShiftLeft path, the overflow/space check is also based on
lengthonly, but the code writes up towriteIndex(which includesblocksToShiftand the extra block). IfwriteIndex + 1 > MaxBlockCount, this can index past the inline buffer. The check should validate the computed new length (writeIndex + 1).
// Set the length to hold the shifted blocks
_length = writeIndex + 1;
- Files reviewed: 12/12 changed files
- Comments generated: 1
The length-cutoff loop extracted one decimal digit per iteration via a full HeuristicDivide + Multiply10, which is O(scale-length) per digit and left the long F<n>/G<n> formats slower after the nuint widening. Pull a native block of digits at once (19 on 64-bit, 9 on 32-bit) via one MultiplyPow10 + DivRem, only when scale spans more than one block so small values keep the O(1) scalar path. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
|
Azure Pipelines: Successfully started running 3 pipeline(s). 13 pipeline(s) were filtered out due to trigger conditions. There may be pipelines that require an authorized user to comment /azp run to run. |
Multiply already documents empty right with a smaller bits buffer as legal; the naive loop is a no-op in that case. Match that contract so the Toom3 pInf * empty qInf path doesn't trip the Debug assert. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
| } | ||
|
|
||
| int resultLength = largeLength; | ||
| Debug.Assert(unchecked((uint)largeLength) < MaxBlockCount); |
There was a problem hiding this comment.
This looks overly restrictive and may trigger for valid additions. For example if I add
To handle overflow I think you probably want something like:
AddSafe(ReadOnlySpan<nuint> left, nuint right, Span<nuint> bits, out nint carry)
and then check whether carry > 0 and if so, unchecked((uint)largeLength) < MaxBlockCount.
Or just remove the check and assert.
There was a problem hiding this comment.
@PranavSenthilnathan, the Number.BigInteger type isn't a true big integer type for arbitrary addition. It and its callers are explicitly bounded based on the constraints of double and so the assertion here is rather serving as a guard that we haven't messed up our logic. -- With has various existing comments/docs throughout, including around the numbers used to compute MaxBlockCount and why we have 1074 for the longest mantissa and 2552 for longest digit sequence.
This path is only reachable from Number.Dragon4 within the shortest roundtrippable path, so the addition is always bounded by scale after a HeuristicDivide has occurred and that gives us roughly 3x headroom for any potential caller here.
We would rather need to expand a few things if we wanted this to become a more general BigInteger type or to support larger floating-point types.
There was a problem hiding this comment.
We notably also have the explicit SetZero guard just after this assert to avoid any potential for real bugs or buffer overruns. So this assert is purely a development guard for if people try to expand things in the future
What
The internal
Number.BigInteger(used to accelerate floating-point parse, format via Dragon4, and rounding) stored its magnitude asuintblocks, leaving the upper half of every register unused on 64-bit. This widens the block tonuintso the school-book add/sub/mul/divide kernels process a native word per step on 64-bit, while remaining correct (and unchanged in width) on 32-bit.Details
nuint-backed blocks.BigInteger's block buffer,BitsPerBlock/BlockShift, and all magnitude operations now operate onnuint. Per-block widening arithmetic usesUInt128on 64-bit andulongon 32-bit, selected by anint.Size/TARGET_64BITconstant that folds at JIT time.BigIntegerCalculator.Shared.csin CoreLib and reused by the publicSystem.Numerics.BigInteger, replacing the duplicatedAddSub/Utils/DivRemcopies (net ~1300 lines removed).GetBlock32->GetBits64/HasZeroTail. The 32-bit-granular block accessors in the parse path are replaced by annuint-aware funnel-shiftGetBits64(bitIndex)(single read on 64-bit when aligned) and a vectorizedHasZeroTail(bitCount)tail-zero scan (ContainsAnyExcept).#ifdef-ing a per-width power-of-ten table, the existingUInt32.PowersOf10uintRVA table is exposedinternaland reused.HeuristicDividestays specialized. Dragon4's per-digit divide keeps its fully-inline multiply-subtract loop behind thenint.Size == 8constant fold -- routing it through the sharedSubMul1regressedToString("G50")~40%+ (measured), since it runs once per output decimal digit.Validation
System.Runtime.Testsfloat suite (Double/Single/Half, incl. exhaustive ibm-fpgen parse patterns): 4608 pass.System.Runtime.Extensions.TestsMath/MathF: 3583 pass.System.Runtime.Numerics.Tests(publicBigInteger, exercises the shared kernels): 8422 pass.Fresh, focused benchmark numbers (shortest-roundtrip
ToString(),G9/G17, common small values + Pi) are in progress and will be posted below.Note
This PR description was drafted by GitHub Copilot on behalf of the author.