Speed up Decimal32/64/128 arithmetic, conversions, and equality - #130957
Conversation
Add fast paths across the shared IEEE 754 decimal implementation: divide extracts multiple digits per iteration, integer/double conversions gain direct-encode and single-rounding paths, and equality gets a dedicated routine instead of routing through the full ordering compare. The shared float mantissa->bits core is factored out so ToDouble reuses the Clinger and Eisel-Lemire fast paths. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
|
Azure Pipelines: Successfully started running 3 pipeline(s). 12 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 |
There was a problem hiding this comment.
Pull request overview
Optimizes the shared IEEE 754 decimal implementation used by Decimal32 / Decimal64 / Decimal128 by adding several fast paths in arithmetic and conversions, and factoring out a reusable mantissa→floating-bits helper to avoid string-based roundtrips in hot paths.
Changes:
- Add adjusted-exponent helpers for decimal formats and use them to enable a direct-encode fast path for common
Addresults. - Speed up
Divideby extracting multiple decimal digits per long-division iteration using a10^kchunk scale chosen from remainder headroom. - Avoid decimal→string→float reparsing by reusing a shared
TryFloatingPointBitsFromMantissa(Clinger + Eisel-Lemire), and add float→decimal rounding fast path via Dragon4 significant-digit cutoff.
Show a summary per file
| File | Description |
|---|---|
| src/libraries/System.Private.CoreLib/src/System/Numerics/Decimal32.cs | Implements adjusted-exponent interface properties for Decimal32. |
| src/libraries/System.Private.CoreLib/src/System/Numerics/Decimal64.cs | Implements adjusted-exponent interface properties for Decimal64. |
| src/libraries/System.Private.CoreLib/src/System/Numerics/Decimal128.cs | Implements adjusted-exponent interface properties for Decimal128. |
| src/libraries/System.Private.CoreLib/src/System/Number.NumberToFloatingPointBits.cs | Factors out TryFloatingPointBitsFromMantissa and reuses it from the digit-based conversion pipeline. |
| src/libraries/System.Private.CoreLib/src/System/Number.DecimalIeee754.cs | Adds fast paths for add/divide/int/float conversions and introduces a dedicated equality routine. |
Copilot's findings
- Files reviewed: 5/5 changed files
- Comments generated: 1
Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
jeffhandley
left a comment
There was a problem hiding this comment.
Note
This review was generated by the holistic code review workflow being iterated on as part of #130339. It was produced by GitHub Copilot (Claude Opus 4.8) and independently cross-checked with two additional models (gpt-5.3-codex, gemini-3.1-pro-preview). Please treat its findings as assistive input for human review.
Holistic Review
Motivation: Justified. This is a pure performance pass over the shared Number.DecimalIeee754.cs implementation, backed by a before→after benchmark table showing large wins (e.g. FromInt32 74→1.8 ns, ToDouble 137→19 ns for D128, Divide 418→109 ns for D128). No new public API surface (no ref/ changes); the MaxAdjustedExponent/MinAdjustedExponent additions are explicit interface overrides of an internal interface using the identical default formula.
Approach: Sound. Every change is a fast path guarded so the general (already-validated) path remains the fallback, and each is intended to be strictly behavior-preserving. The refactor also usefully de-duplicates the Clinger/Eisel-Lemire core (TryFloatingPointBitsFromMantissa) so the decimal→float path reuses the float parser's rounding rather than rendering to ASCII and re-parsing.
Summary: ✅ LGTM. I independently verified each fast path is behavior-equivalent, and two additional models reached the same conclusion (one ran its own 50k-case differential on the divide loop). No correctness or overflow issues were found. Remaining items are advisory (💡). Because correctness assurance rests on the existing suites, a maintainer should confirm the Decimal32/64/128 and Double/Single/Half CI legs are green before merge.
Detailed Findings
✅ Performance evidence — provided and credible
A before→after microbench table is included for all affected operations across D32/D64/D128, plus differential validation (7.9M FromDouble vs BigInteger exact expansions with 0 mismatches; 9M divide loop-identity cases; 4482 xunit tests incl. the Intel BID reference vectors). This satisfies the "performance changes need measured evidence" bar.
✅ Behavior-preserving correctness — verified across all fast paths (corroborated by 2 models)
- Equality rewrite: Matches the old
Compare(...) == 0for identical bits, NaN (always unequal), non-canonical/signed infinities, all zero cohorts, and finite cohort equality. ThediffExponent >= Precisionshort-circuit correctly returnsfalseand protectsPower10from an out-of-table argument; theDivRem(right.Significand, Power10(diffExponent))divisibility check is the correct cohort-equality test. - Add fast paths:
effectiveDifference == 0is exactly multiply-by-one. The "no rounding" direct-encode is safe becausestickyrequiresdroppedDigits > 0⇒exponentDifference > Precision + 2⇒hiscaled by10^(Precision+2), forcing ≥Precision+2result digits (or a non-zero high limb) — soCountDigits(magnitudeLow) <= Precisionprovably excludes everystickycase. Signed-zero and exponent-clamp behavior matches the wide-path tail. - Divide multi-digit chunking:
k = (lz*3)/10 = ⌊0.3·lz⌋ < lz/log2(10), so10^k < 2^lzandremainder·10^kcannot overflow the limb (thek>remaining/k<1clamps stay safe sinceremainder < MaxSignificandandMaxSignificand·10fits every format). Processingkdigits per step is algebraically identical tokone-digit steps. - WideMultiply low-half / WideDigitCount high==0 / DropDigits high==0 fast paths are exact equivalents; the
DropDigitsDivRemPow10(low, dropCount)andPower10(dropCount-1)arguments stay in range (dropCount < Precision), matching existingDivRemPow10usage. - FromInt32 direct-encode (
magnitude <= MaxSignificand, exponent 0) matches the digit-pipeline result for exactly-representable integers. - ToDouble fast path feeds
TryFloatingPointBitsFromMantissawithscale = CountDigits + exponent(=number.Scale) and the sameMin/MaxDecimalExponentshortcuts as the string path, falling back viagoto Slowon the uncommon Eisel-Lemire miss. - FromDouble single-round:
Dragon4(cutoff=Precision, isSignificantDigits=true)is correctly rounded, and since the coefficient then has ≤Precisiondigits the encoder does no second rounding. The(number.Scale - number.DigitsCount) >= MinAdjustedExponentguard correctly diverts only the decimal-subnormal range (reachable only for Decimal32 from binary floats) to the exact full-expansion path, avoiding double rounding, and handles the cutoff carry (9…9 → 10…0) via Dragon4's adjusted scale. TryFloatingPointBitsFromMantissaextraction:(exponent < 0) ? divide : multiplyis exactly equivalent to the old(fractionalDigitsPresent != 0) ? …, sincefractionalDigitsPresent > 0 ⟺ Scale < DigitsCount ⟺ exponent < 0.
✅ NumberBuffer reuse across two Dragon4 calls — safe
ConvertFloatToDecimalIeee754 now calls Dragon4 twice on the same buffer in the subnormal branch. Number.Dragon4.Dragon4<TNumber> writes fresh digits from index 0 and resets Scale, DigitsCount, and the '\0' terminator on every call; IsNegative is re-set by the caller and HasNonZeroTail legitimately stays false. Stale digits beyond the new DigitsCount are never read (the pipeline bounds on DigitsCount/terminator).
💡 Tests — no committed coverage for the new branches
The PR adds no tests, relying on the existing Intel BID reference vectors plus one-off differential harnesses that aren't committed. That's reasonable for a behavior-preserving refactor, but the divide loop-identity and FromDouble-vs-BigInteger differentials are exactly the kind of high-value checks worth committing (even as outerloop/ConditionalTheory), and it's worth confirming the existing suites actually exercise the new fast-path branches (e.g. the Add no-rounding path, the Equals cohort path, the decimal-subnormal FromDouble fallback).
💡 Scope & the disclosed Decimal128 equality regression
The change is broader than the branch name suggests (equality, add, divide, and three conversions), though cohesive and per-operation committed; the author offers to gate individual pieces. The one self-disclosed regression — Decimal128 equality 19→22 ns, attributed to UInt128 codegen-layout drift and dwarfed by same-type wins — is worth a maintainer's glance but looks like an acceptable trade given the net improvement.
|
Workflow state for the Holistic Review Orchestrator. {
"version": 5,
"last_dispatched_commit": "6b52930437f7bf1fadb0b85837633439fabe4393",
"last_dispatched_base_ref": "main",
"last_dispatched_base_sha": "7f416ac6c742e34d2b11ebb6c09b9d2397717185",
"last_reviewed_commit": "6b52930437f7bf1fadb0b85837633439fabe4393",
"last_reviewed_base_ref": "main",
"last_reviewed_base_sha": "7f416ac6c742e34d2b11ebb6c09b9d2397717185",
"last_recorded_worker_run_id": "29685660942",
"review_attempt_commit": "",
"review_attempt_base_ref": "",
"review_attempt_count": 0,
"max_review_attempts": 5,
"review_history_format": "holistic-review-disclosure-v1",
"review_history": [
{
"commit": "6b52930437f7bf1fadb0b85837633439fabe4393",
"review_id": 4730707311
}
]
} |
There was a problem hiding this comment.
Holistic Review
Motivation: Justified. The shared IEEE-754 decimal implementation was uniformly slow on hot paths (FromInt32, FromDouble, ToDouble, Divide, Add, equality), and the PR backs each optimization with concrete before/after microbenchmarks plus differential validation against exact BigInteger expansions and the Intel BID reference vectors.
Approach: Sound and consistent with the existing generic-math structure. Fast paths are additive guards in front of the existing correct-but-slow code (single-limb shortcuts in WideMultiply/WideDigitCount/DropDigits, direct encoding in FromInt32, exponent-equal shortcut in Add, multi-digit chunking in Divide), and the ToDouble path reuses the float parser's Clinger/Eisel-Lemire core by extracting TryFloatingPointBitsFromMantissa rather than duplicating it. MaxAdjustedExponent/MinAdjustedExponent are added as interface members with correct per-format overrides.
Summary: FromDouble, overflow bounds in the Divide chunk loop, scale equivalence in the ToDouble bound checks) that warrant a maintainer's confirmation, and this worker cannot build or run the D32/64/128 and float-parse suites that the author reports passing. A human should confirm those test runs and the FromDouble subnormal-decimal boundary.
Detailed Findings
✅ ToDouble fast path — scale and range checks match the parse path
In ConvertDecimalIeee754ToFloat, scale = CountDigits(decoded.Significand) + exponent reproduces NumberBuffer.Scale (integer-digit count of significand × 10^exponent), and the < MinDecimalExponent → zero / > MaxDecimalExponent → infinity boundaries match NumberToFloat exactly, so the underflow/overflow behavior is preserved. The goto Slow only skips into code whose locals (mantissa/exponent/scale) are scoped inside the fast-path block, so no uninitialized-use hazard.
✅ Divide multi-digit chunking — overflow and table bounds hold
k = (lz * 3) / 10 under-approximates log10(2^lz), so remainder * 10^k < 2^(bits-lz) * 2^lz = 2^bits cannot overflow the limb; k is clamped to [1, Precision-1] and to the digits still owed, keeping every Power10(k) in-table. Because remainder < divisor <= MaxSignificand, there is always enough leading-zero headroom for k >= 1. Quotient/remainder identity is preserved versus the original per-digit loop.
✅ Equality cohort check — divisibility logic is correct
The dedicated path handles bit-equal (incl. NaN), infinities, zeros (all-zero cohort equivalence), and sign before the exponent-alignment step. After swapping so left holds the larger exponent, DivRem(right.Significand, Power10(diff)) with remainder == 0 && quotient == left.Significand correctly tests cohort equality, and the diff >= Precision early-out is safe because right.Significand < 10^Precision <= 10^diff <= left.Significand * 10^diff.
✅ Add equal-exponent shortcut and single-limb helper guards
The effectiveDifference == 0 branch (scale factor = 1) and the new no-rounding tail (magnitudeHigh == 0, exponent in adjusted range, CountDigits <= Precision) both reduce to the same encoding the wide path would produce; the comment correctly argues a non-empty sticky tail cannot co-occur with those bounds. The WideMultiply/WideDigitCount/DropDigits single-limb (high == 0) fast paths are exact equivalents of the wider-integer computations.
💡 FromDouble single-rounding argument depends on Dragon4 cutoff correctness
The fast path (Dragon4 with cutoffNumber: Precision, then MaterializePreferredZeros) relies on Dragon4's significant-digit cutoff being correctly round-half-even so the shared pipeline performs no second rounding, with subnormal-decimal inputs (Scale - DigitsCount < MinAdjustedExponent) correctly diverted to the exact full-expansion path to avoid double rounding. This reasoning looks right and matches the author's 7.9M-case differential, but it is the highest-risk change; a maintainer should confirm the boundary condition and that the reported suite runs green.
Note
This review was generated by this repository's Holistic Review agentic workflow to complement the built-in Copilot review.
Generated by Holistic Review · 175 AIC · ⌖ 10.8 AIC · ⊞ 10K
Fast paths across the shared IEEE 754 decimal implementation (
Number.DecimalIeee754.cs) used byDecimal32/Decimal64/Decimal128:kdigits per iteration (bounded by the remainder's leading-zero count) instead of one.TryFloatingPointBitsFromMantissa.Precisionsignificant digits (already correctly rounded), then the encoder drops only re-materialized preferred zeros -- no double rounding. Subnormal-decimal inputs fall back to the exact full expansion.Before -> after, ns/op (local min-of-trials microbench, same harness; lower is better):
Negate,Abs,Parse,TryParse,ToString, andTryFormatare unchanged.The one regression is
Decimal128equality (~19 -> ~22 ns). The equality routine itself is faster than before; the delta is codegen-layout drift on theUInt128path from the surrounding arithmetic changes, and is dwarfed by the same-type wins (e.g. Divide -309 ns, ToDouble -118 ns). Happy to gate any individual piece if preferred.Validation:
Decimal32/64/128xunit suites pass (4482 tests, including the Intel BID reference vectors).Double/Single/Halfparse suites pass (the extracted mantissa helper is shared with float parsing).BigIntegerdecimal expansions -- 0 value mismatches.No JIT changes.
Note
This PR description and the underlying changes were assisted by GitHub Copilot.