Improving the 64-bit number formatting to better match the 32-bit algorithm #68795
Conversation
|
Tagging subscribers to this area: @dotnet/area-system-numerics Issue DetailsFound that the implementation was less efficient than expected when working on
|
|
Do you plan to add a few int128 formatting/parsing benchmarks in the perf repo? |
After Int128/UInt128 is added to the repo, yes. This is just some intermediate cleanup that will make adding those types simpler. I'll get benchmark results for Int64/UInt64 after CI is passing. |
|
A few small wins (running the Int64/UInt64 formatting benchmarks from dotnet/performance) BeforeBenchmarkDotNet=v0.13.1.1786-nightly, OS=Windows 11 (10.0.22000.652/21H2) PowerPlanMode=00000000-0000-0000-0000-000000000000 IterationTime=250.0000 ms MaxIterationCount=20
AfterBenchmarkDotNet=v0.13.1.1786-nightly, OS=Windows 11 (10.0.22000.652/21H2) PowerPlanMode=00000000-0000-0000-0000-000000000000 Toolchain=CoreRun IterationTime=250.0000 ms
|
| } | ||
|
|
||
| [MethodImpl(MethodImplOptions.AggressiveInlining)] // called from only one location | ||
| [MethodImpl(MethodImplOptions.AggressiveInlining)] |
There was a problem hiding this comment.
Was the comment stale, or this PR is causing it to be called in more places?
There was a problem hiding this comment.
Yes. We now handle both byte and char based versions so its at least 2 places
| return true; | ||
| } | ||
|
|
||
| [MethodImpl(MethodImplOptions.AggressiveInlining)] |
There was a problem hiding this comment.
We're ok with whatever additional code size this leads to? (General question for the AggressiveInlinings added in the PR)
There was a problem hiding this comment.
This was previously manually inlined in a few cases already.
Otherwise, this is a decently hot loop that executes under 10 iterations for 32-bit and 20 iterations for 64-bit, so its worth inlining.
| ulong value = (ulong)(-input); | ||
|
|
||
| int bufferLength = Math.Max(digits, FormattingHelpers.CountDigits((ulong)(-input))) + sNegative.Length; | ||
| int bufferLength = Math.Max(digits, FormattingHelpers.CountDigits((ulong)(-value))) + sNegative.Length; |
There was a problem hiding this comment.
Any benefit to doing value = -value once here rather than negating it both here and down below?
There was a problem hiding this comment.
For Int128/UInt128 there will be. For Int64/UInt64 I'd expect the JIT to do the relevant CSE here.
I'm fine with manually extracting it for consistency if we want.
| do | ||
| { | ||
| ulong remainder; | ||
| (value, remainder) = Math.DivRem(value, 10); |
There was a problem hiding this comment.
We use Math.DivRem here and elsewhere but separate % and / operations above in Int64DivMod1E9, and in both cases the divisor is a constant. Is there a reason for the difference?
There was a problem hiding this comment.
DivRem is more efficient. Int64DivMod1E9 is a bit of a special case where we are effectively doing both 32-bit = 64-bit % 32-bit and 64-bit = 64-bit / 32-bit.
The former can be optimized on some platforms, but I didn't do any investigation to confirm if the JIT was doing anything special here or not.
|
|
||
| internal static unsafe string UInt64ToDecStr(ulong value) | ||
| { | ||
| // Intrinsified in mono interpreter |
There was a problem hiding this comment.
What does this comment refer to?
There was a problem hiding this comment.
I believe its referring to CountDigits. It's a pre-existing comment that I preserved
|
Improvements seen on arm64: dotnet/perf-autofiling-issues#5277 and dotnet/perf-autofiling-issues#5282 Also dotnet/perf-autofiling-issues#5278 shows both better and worse results (bimodal with new highs and lows) |
ghost
locked as resolved and limited conversation to collaborators
5 participants
Found that the implementation was less efficient than expected when working on
Int128andUInt128. This updates the 64-bit paths to be closer to what the 32-bit paths do, but with keeping the "decomposed" operation on 32-bit platforms.