Remove the Number static constructor - #132550
Conversation
Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
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 |
|
This was raised by @MichalStrehovsky in MichalStrehovsky/rt-sz#247 as caused by #131019 Basically the issue was some new static readonly field introduced. So this PR fully removes all the static fields, ensuring we either use RVA statics or a helper lookup so that it can be fully avoided and there is now no |
There was a problem hiding this comment.
Pull request overview
Refactors System.Number implementation details to eliminate the type’s static constructor by moving previously eagerly-initialized caches/tables into RVA-backed spans and nested holder types, computing the remaining small constants on demand. This primarily targets improved trimming (unused data not rooted by .cctor) while preserving existing formatting/math behavior.
Changes:
- Replaces managed string-array formatting patterns with switch-selected
ReadOnlySpan<byte>format templates in the shared formatting helper. - Moves small number and Mono “TwoDigits” caches behind nested holder types to avoid
Number-level eager initialization. - Converts multiple Decimal IEEE754 coefficient/constant tables (trig/log/pow/exp/hyper) from
static readonlyarrays to span-backed representations plus small on-demand constant getters.
Show a summary per file
| File | Description |
|---|---|
| src/libraries/System.Private.CoreLib/src/System/Number.Formatting.cs | Moves small-number and Mono TwoDigits caches into nested holders to avoid Number static initialization. |
| src/libraries/System.Private.CoreLib/src/System/Number.DecimalIeee754.Transcendental.cs | Updates call sites to use on-demand pi / inverse-trig constants instead of static arrays. |
| src/libraries/System.Private.CoreLib/src/System/Number.DecimalIeee754.DiyFp128Trig.cs | Switches trig coefficient tables to span-backed coefficient data. |
| src/libraries/System.Private.CoreLib/src/System/Number.DecimalIeee754.DiyFp128Sqrt.cs | Replaces the sqrt coefficient struct table with split span-backed AB/C coefficient data. |
| src/libraries/System.Private.CoreLib/src/System/Number.DecimalIeee754.DiyFp128Pow.cs | Converts pow coefficient tables to span-backed coefficients. |
| src/libraries/System.Private.CoreLib/src/System/Number.DecimalIeee754.DiyFp128PiTrig.cs | Replaces static pi-fraction constants with an on-demand getter and makes small constants computed properties. |
| src/libraries/System.Private.CoreLib/src/System/Number.DecimalIeee754.DiyFp128Log.cs | Converts log coefficient table to span-backed coefficients. |
| src/libraries/System.Private.CoreLib/src/System/Number.DecimalIeee754.DiyFp128InvTrig.cs | Replaces inverse-trig constants array with an on-demand getter; converts coefficient tables to span-backed coefficients. |
| src/libraries/System.Private.CoreLib/src/System/Number.DecimalIeee754.DiyFp128Hyper.cs | Converts sinh/cosh coefficient tables to span-backed coefficients. |
| src/libraries/System.Private.CoreLib/src/System/Number.DecimalIeee754.DiyFp128Exp.cs | Adds a shared span-casting helper for fixed coefficients and converts exp/exp10 tables to span-backed coefficients. |
| src/libraries/Common/src/System/Number.Formatting.Common.cs | Replaces currency/number/percent format string arrays with switch-selected ReadOnlySpan<byte> templates. |
Review details
- Files reviewed: 11/11 changed files
- Comments generated: 0
- Review effort level: Lite
Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
There was a problem hiding this comment.
Review details
Suppressed comments (1)
Previously missed (1) — in code that hasn't changed since the last review.
src/libraries/System.Private.CoreLib/src/System/Number.DecimalIeee754.DiyFp128Exp.cs:39
DiyFp128FixedCoefficientsrelies onMemoryMarshal.Cast<ulong, DiyFp128FixedCoefficient>which assumesDiyFp128FixedCoefficientstays exactly 2 ulongs (16 bytes). Adding an explicit size assertion makes this helper robust against future edits to the struct that would otherwise silently reinterpret data incorrectly.
private static ReadOnlySpan<DiyFp128FixedCoefficient> DiyFp128FixedCoefficients(ReadOnlySpan<ulong> coefficients)
{
Debug.Assert((coefficients.Length & 1) == 0);
return MemoryMarshal.Cast<ulong, DiyFp128FixedCoefficient>(coefficients);
}
- Files reviewed: 11/11 changed files
- Comments generated: 1
- Review effort level: Lite
| "(#)", "-#", "- #", "#-", "# -", | ||
| ]; | ||
|
|
||
| private static ReadOnlySpan<byte> GetCurrencyFormat(bool isNegative, int index) |
There was a problem hiding this comment.
not sure if perf matters for this, but I am sure this is unlikely to inline for NAOT without PGO (and others).
There was a problem hiding this comment.
It should be fine if it doesn't inline, the index is never constant and so its a "bounds check" either way.
Looks like this fully fixes the regression: MichalStrehovsky/rt-sz#249
|
Moves
Number's primitive Decimal32/64/128 coefficient data to RVA-backed spans and computes the remaining small structured constants on demand. Managed caches that still require initialization are isolated in nested holder types, soNumberno longer has a static constructor and unused data can be trimmed.Formatting patterns now use switch-selected RVA byte spans rather than managed string arrays. Direct byte handling was within benchmark noise versus the prior implementation: currency
-1.9%, number-1.2%, and percent+0.2%, with unchanged allocations.Validated CoreCLR, NativeAOT, and Mono builds and all 77,366
System.Runtime.Tests.Note
This pull request was created with GitHub Copilot.