Reduce unsafe code in System.Decimal

[pentp]

Reduces unsafe code use in decimal where it was easy to remove.
These changes are all either performance neutral or improvements. For the remaining cases the changes are more involved and need thorough perf measurements and some effort to offset the perf losses.

[Copilot]

Pull request overview

This PR reduces the amount of unsafe code used by System.Decimal/DecCalc by replacing a few low-risk unsafe patterns with safe equivalents, while keeping behavior and performance characteristics consistent.

Changes:

• Route decimal equality (Equals, ==, !=) through a new DecCalc.Equals fast-path that avoids a full compare when possible.
• Replace unsafe unaligned writes in IFloatingPoint<decimal>.TryWriteSignificand*Endian with BinaryPrimitives.Write*Endian.
• Remove/limit unsafe usage in DecCalc helpers (e.g., pointer-based Div128By64, manual exponent extraction) and adjust internal buffer layouts to support the ref-based approach.

Reviewed changes

Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.

File	Description
src/libraries/System.Private.CoreLib/src/System/Decimal.cs	Switch equality implementations to DecCalc.Equals, replace unsafe significand writes with BinaryPrimitives, and minor logic simplifications in numeric helpers.
src/libraries/System.Private.CoreLib/src/System/Decimal.DecCalc.cs	Introduce DecCalc.Equals, remove pointer-based division helper usage, replace exponent extraction helpers with BiasedExponent, and refactor internal buffer structs to avoid unsafe casts/pointers.

[Copilot]

Pull request overview

Copilot reviewed 4 out of 4 changed files in this pull request and generated 3 comments.

[EgorBo]

Does this need some benchmark tests (feel free to use EgorBot)? Replacing unsafe code with safe too often leads to regressions so we try to validate that in advance in this sort of changes.

Overall LGTM, but am not an expert in Decimal's impl, so deferring to @tannergooding

[EgorBo]

@MihuBot

[pentp]

Does this need some benchmark tests (feel free to use EgorBot)? Replacing unsafe code with safe too often leads to regressions so we try to validate that in advance in this sort of changes.

This PR has only the easy parts that don't regress any of the codegen. For the rest I have a work in progress branch where I offset some safety checks cost by sacrificing some 32-bit perf and other tricks. #117261 would probably also help with that (automerge is blocked by some failing/flaky tests there).

[tannergooding]

@pentp I've started the review, but looks like there's a conflict that needs resolving

[Copilot]

Pull request overview

Copilot reviewed 4 out of 4 changed files in this pull request and generated no new comments.

[pentp]

I resolved the conflicts, added some comments and fixed Buf12 packing to be uint sized (not ulong).

[tannergooding]

LGTM, minus the need to add a comment explaining why the mod arithmetic is correct so future readers aren't left guessing.

[tannergooding]

@pentp just waiting on the addition of the comment explaining the mod-10 reduction and this can get merged.

[github-actions]

Workflow state for the Holistic Review Orchestrator.

{
  "version": 5,
  "last_dispatched_commit": "14b792dbffff1dcb5866797548a823293dbd52f7",
  "last_dispatched_base_ref": "main",
  "last_dispatched_base_sha": "2c21b93b70abc47370477712689190e993a3706f",
  "last_reviewed_commit": "14b792dbffff1dcb5866797548a823293dbd52f7",
  "last_reviewed_base_ref": "main",
  "last_reviewed_base_sha": "2c21b93b70abc47370477712689190e993a3706f",
  "last_recorded_worker_run_id": "29690665919",
  "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": "1fa29065c6ad49f9796cd0cb70be1e79965dd74d",
      "review_id": 4730524928
    },
    {
      "commit": "14b792dbffff1dcb5866797548a823293dbd52f7",
      "review_id": 4730923301
    }
  ]
}

[github-actions]

Holistic Review

Motivation: The PR reduces the amount of unsafe code (raw pointers, Buf* reinterpretation, Unsafe.As) in System.Decimal's core math, targeting only the cases where removal is straightforward and either performance-neutral or a small improvement. This is a worthwhile maintainability and safety win in a hot, security-sensitive numeric type in System.Private.CoreLib.

Approach: Pointer-based helpers are converted to ref/in equivalents (Div128By64, VarDecDiv losing unsafe), the Buf16 struct now exposes Low96/High96 as overlapping Buf12 fields (with Pack = sizeof(uint) added to Buf12 so the offset-4 embedding is legal) instead of duplicated big/little-endian Low64/High64 accessors, and SearchScale takes decomposed ulong/uint arguments rather than a ref Buf12. Bespoke GetExponent(float/double) helpers are replaced with the existing Single.BiasedExponent/Double.BiasedExponent members. Several INumberBase predicates and equality paths are streamlined: a new DecCalc.Equals fast-path avoids the full three-way compare, IsZero/IsNormal use direct (_hi32 | _lo64) bit tests, IsCanonical performs an explicit two-step 96-bit mod-10 instead of allocating a UInt128, and MaxMagnitude/MinMagnitude reuse a single VarDecCmp result. Signed scale comparisons are hardened to (uint) casts, and the scattered Number.ThrowOverflowException(SR.Overflow_Decimal) calls plus goto ThrowOverflow labels are unified behind a new parameterless Number.ThrowDecimalOverflowException() (also updated in UInt128.cs).

Summary: I reviewed the full base-to-head range across all four files and verified the key equivalences. BiasedExponent yields the same biased-exponent bits as the removed helpers for both float and double; the new IsCanonical two-step reduction correctly computes the 96-bit significand mod 10 ((high64 mod 10) << 32 | low32, then mod 10), and the intermediate never overflows since the folded remainder is < 10; the DecCalc.Equals fast-path is a valid specialization of VarDecCmp == 0 (zero handling, sign-mismatch via (_flags ^ _flags) < 0, then magnitude compare); IsZero/IsNormal bit tests match the prior value == 0 semantics because a decimal is zero iff its 96-bit significand is zero regardless of scale/sign; and the Buf16 overlay preserves the previous Low64/High64 offsets. The unsafe-removal and throw-helper consolidation are behavior-preserving refactors. No correctness or behavioral regressions found; changes are consistent with the stated performance-neutral-or-better intent. This looks good to merge.

Note

This review was generated by this repository's Holistic Review agentic workflow to complement the built-in Copilot review.

Generated by Holistic Review · 85.5 AIC · ⌖ 10.4 AIC · ⊞ 10K

[Copilot]

Pull request overview

Copilot reviewed 4 out of 4 changed files in this pull request and generated no new comments.

[github-actions]

Holistic Review

Motivation: The PR reduces the amount of unsafe code (raw pointers, Buf* reinterpretation, Unsafe.As) in System.Decimal's core math, targeting only the cases where removal is straightforward and either performance-neutral or a small improvement. This is a worthwhile maintainability and safety win in a hot, security-sensitive numeric type in System.Private.CoreLib.

Approach: Pointer-based helpers become ref/in equivalents, the Buf16 struct exposes overlapping Low96/High96 Buf12 fields (with Pack = sizeof(uint) on Buf12) instead of duplicated big/little-endian accessors, SearchScale takes decomposed ulong/uint arguments, and bespoke GetExponent helpers are replaced by Single/Double.BiasedExponent. Several INumberBase predicates and equality paths are streamlined, including an explicit two-step 96-bit mod-10 reduction in IsCanonical, and the scattered decimal overflow throws are unified behind Number.ThrowDecimalOverflowException(). The latest commit only refines the explanatory comment above the IsCanonical reduction step; it makes no code change.

Summary: I refreshed the cumulative assessment over the full base-to-head range and reviewed the single incremental commit. The new comment ("magnitude is high64 * 2^32 + low32"; reduce high64 first, then fold in low32) accurately describes the existing code: tmp = (hi32 << 32) | (lo64 >> 32) reconstructs the top 64 bits of the 96-bit significand, and ((high64 % 10) << 32 | low32) % 10 correctly equals (high64 * 2^32 + low32) % 10 because (x * k) % 10 == ((x % 10) * k) % 10. The wording "depends only on a % 10" is a slightly loose but acceptable paraphrase of that reduction. This is a documentation-only clarification with no behavioral effect. The overall verdict is unchanged: the unsafe-removal, predicate streamlining, and throw-helper consolidation remain behavior-preserving, and the PR looks good to merge.

Assessment History:

• review 4730524928 (commit 1fa29065): verdict LGTM. Current verdict LGTM. Unchanged — the only new commit (14b792db) clarifies the IsCanonical comment without altering code, so motivation, approach, and risk are all the same.

Note

This review was generated by this repository's Holistic Review agentic workflow to complement the built-in Copilot review.

Generated by Holistic Review · 48.9 AIC · ⌖ 10.6 AIC · ⊞ 10K