Extract the generic integer formatting helper APIs to live on a generic type instead #121077
Conversation
github-actions
bot
added
the
needs-area-label
There was a problem hiding this comment.
Pull Request Overview
This PR refactors integer formatting helper APIs to consolidate them in a new generic NumberFormat<TChar> type. The goal is to reduce the number of types loaded at startup by minimizing type resolutions, though this may introduce slightly more instantiation overhead for unused methods.
Key changes:
- Created new
NumberFormat<TChar>generic class containing integer formatting methods - Moved formatting methods from
Numberclass toNumberFormat<TChar> - Updated all integer types and DateTime/TimeSpan formatting to use the new generic helper
- Changed visibility of several
Numberclass methods frominternal/privatetopublicto support the new architecture
Reviewed Changes
Copilot reviewed 18 out of 18 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
| NumberFormat_1.cs | New generic formatting helper class with methods for integer formatting operations |
| Number.Formatting.cs | Removed integer formatting methods moved to NumberFormat<TChar>, updated visibility of remaining helpers |
| Int16.cs, Int32.cs, Int64.cs, Int128.cs | Updated to call NumberFormat<TChar>.TryFormat* methods instead of Number.TryFormat* |
| UInt16.cs, UInt32.cs, UInt64.cs, UInt128.cs | Updated to call NumberFormat<TChar>.TryFormat* methods instead of Number.TryFormat* |
| Byte.cs, SByte.cs, Decimal.cs | Updated to call NumberFormat<TChar>.TryFormat* methods instead of Number.TryFormat* |
| DateTimeFormat.cs | Updated to call NumberFormat<TChar>.WriteTwoDigits/WriteFourDigits/WriteDigits methods |
| TimeSpanFormat.cs | Updated to call NumberFormat<TChar>.WriteTwoDigits/WriteDigits methods |
| Utf8Formatter.Integer.cs | Updated to call NumberFormat<byte> methods for UTF-8 formatting |
| Number.Formatting.Common.cs | Added conditional compilation to use NumberFormat<TChar> methods when in System.Private.CoreLib |
| System.Private.CoreLib.Shared.projitems | Added NumberFormat_1.cs to the project |
| internal static partial class Number | ||
| { | ||
| internal const int DecimalPrecision = 29; // Decimal.DecCalc also uses this value | ||
| private const int DecimalPrecision = 29; // Decimal.DecCalc also uses this value |
There was a problem hiding this comment.
The DecimalPrecision constant is changed from internal to private, but it's referenced in the comment as being used by Decimal.DecCalc. If DecimalPrecision is indeed shared with Decimal.DecCalc, changing it to private may break that usage. Please verify that Decimal.DecCalc doesn't need access to this constant, or consider keeping it internal.
| private const int DecimalPrecision = 29; // Decimal.DecCalc also uses this value | |
| internal const int DecimalPrecision = 29; // Decimal.DecCalc also uses this value |
| } | ||
|
|
||
| internal static string UInt32ToDecStrForKnownSmallNumber(uint value) | ||
| private static string UInt32ToDecStrForKnownSmallNumber(uint value) |
There was a problem hiding this comment.
The visibility of UInt32ToDecStrForKnownSmallNumber was changed from internal to private. If this method was previously used by other parts of the codebase outside this class, this change could break those dependencies.
| private static string UInt32ToDecStrForKnownSmallNumber(uint value) | |
| internal static string UInt32ToDecStrForKnownSmallNumber(uint value) |
| } | ||
|
|
||
| internal static unsafe string UInt64ToDecStr(ulong value, int digits) | ||
| private static unsafe string UInt64ToDecStr(ulong value, int digits) |
There was a problem hiding this comment.
The visibility of UInt64ToDecStr was changed from internal to private. If this method was previously used by other parts of the codebase outside this class, this change could break those dependencies.
| private static unsafe string UInt64ToDecStr(ulong value, int digits) | |
| internal static unsafe string UInt64ToDecStr(ulong value, int digits) |
| } | ||
|
|
||
| internal static unsafe string UInt128ToDecStr(UInt128 value, int digits) | ||
| private static unsafe string UInt128ToDecStr(UInt128 value, int digits) |
There was a problem hiding this comment.
The visibility of UInt128ToDecStr was changed from internal to private. If this method was previously used by other parts of the codebase outside this class, this change could break those dependencies.
| private static unsafe string UInt128ToDecStr(UInt128 value, int digits) | |
| internal static unsafe string UInt128ToDecStr(UInt128 value, int digits) |
|
With the fix I got checked in this probably doesn't have a big effect on types loaded if things are r2r compiled. It likely does have an impact on count of types loaded if hitting. There is another fix to be made in the accessibility checking code to deal with that. |
|
Would you suggest this isn't needed/necessary anymore then and can likely be closed out? |
2 participants
This was raised by @davidwrighton as something that might help with the number of types being loaded (#120407) on startup paths due to requiring less resolutions (at the tradeoff of slightly more instantiation overhead for unused methods)
This is just a simple first pass to see if it does benefit. More cleanup can be done if the approach shows to be effective and we desire more consolidation. This namely targets the integer APIs. We could isolate it specifically to groups that call eachother (i.e. break apart Int32/Int64/Int128 into their own classes), but in many cases the larger sizes invoke the smaller sizes for some work, so it's probably not that meaningful.
CC. @jkotas, @EgorBo