Repository navigation
Implement IBinaryInteger<T>.Write*Endian explicitly on the numeric types to avoid boxing - #134152
caraioniurie47 wants to merge 1 commit into
Conversation
The default interface implementations of the six WriteBigEndian and WriteLittleEndian overloads box a value-type receiver unless the JIT removes the box, which it does not do at every call site. The fourteen IBinaryInteger<T> implementations in the libraries now implement them explicitly and forward to a helper that makes a constrained call to TryWriteBigEndian or TryWriteLittleEndian and throws the same exceptions as the default implementations. Fix dotnet#77969 Co-Authored-By: Claude Opus 5 <noreply@anthropic.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 is a significant amount of code for a niche edge case API. You can even trivially work around by doing I'm not convinced it is worth the churn and additional implementation bloat nor that the box is meaningfully enough to warrant it. |
Thanks for looking. One data point on the workaround: in the PR description's benchmark, |
|
@tannergooding, following up on the question above with a re-check on a newer build. On a using System.Numerics;
using System.Runtime.CompilerServices;
BigInteger value = BigInteger.Parse("123456789012345678901234567890");
byte[] buffer = new byte[64];
// Let tiered compilation promote Write to Tier1 first.
for (int round = 0; round < 50; round++)
{
for (int i = 0; i < 100_000; i++)
{
Write(value, buffer.AsSpan());
}
Thread.Sleep(20);
}
long before = GC.GetAllocatedBytesForCurrentThread();
Write(value, buffer.AsSpan());
Console.WriteLine(GC.GetAllocatedBytesForCurrentThread() - before);
[MethodImpl(MethodImplOptions.NoInlining)]
static int Write<T>(T value, Span<byte> destination) where T : IBinaryInteger<T>
=> value.WriteBigEndian(destination);In the Tier1 code the JIT inlines the default A Note AI-generated, written at my direction and reviewed by me before posting. The snippet targets net10.0 and was run as shown on Windows x64, on 10.0.12 and on the daily builds 11.0.0-rtm.26480.113 and 12.0.0-alpha.1.26480.102 (installed with |
|
I wasn't saying the box was going to go away. Rather, I was questioning whether the box is actually meaningful at all here given the work being done, the likely rarity of it all, etc. It's a lot of additional API churn and surface for something that is unlikely to be problematic in practice. |
|
Understood, thanks for explaining. Closing this; since explicit implementations were tried and judged not worth the churn, #77969 could be closed as well. |
IBinaryInteger<T>.WriteBigEndianandWriteLittleEndian(three overloads each) are default interface methods that callTryWriteBigEndian/TryWriteLittleEndian. No implementation provides them, so calling one on a value type through a generic constraint runs the default implementation, which boxes the value unless the JIT removes the box. The repro from the issue, with two lines added to print the bytes allocated by its loop, allocates onmainas it does on .NET 10:main(Release build)Change. As suggested in #77969 (comment), the fourteen implementations of
IBinaryInteger<T>now implement the six methods explicitly:Byte,SByte,Char,Int16,UInt16,Int32,UInt32,Int64,UInt64,Int128,UInt128,IntPtr,UIntPtrandBigInteger. Each forwards to a new internalBinaryIntegerWriter, whose two generic methods contain the default implementation's body: call the constrainedTryWritemethod and throwThrowHelper.ThrowArgumentException_DestinationTooShort()if it returnsfalse. System.Runtime.Numerics compiles the same file, as it already doesBigIntegerCalculator.Shared.cs, and gains the matching throw helper and resource string, copied from CoreLib. The ReleaseSystem.Private.CoreLib.dllgrows by 16,384 bytes andSystem.Runtime.Numerics.dllby 1,024 bytes; no real-world application was measured.The ref assemblies list the new explicit implementations next to each type's
TryWrite*implementations. ApiCompat does not require them: a build with a ref member that the implementation lacked also passed. They are there to keep the ref in step with the implementation, and can be dropped if that is preferred.Behaviour. Unchanged. The bytes written, the return value and the exceptions (
ArgumentExceptionwithParamNamedestinationfor a null array, a destination too short and an empty span;ArgumentOutOfRangeExceptionfor astartIndexpast the end) were compared for all fourteen types on .NET 10.0.12 and on this branch, with the same results. Thefalsepath throws the sameArgumentExceptionas the default implementation —Destination is too short.withParamNamedestination, from CoreLib's throw helper and, in System.Runtime.Numerics, from the copy of it and of the resource string added here — and in all fourteen typesTryWrite*Endianreturnsfalseonly where the destination is smaller than the value's byte count, so that message describes every failure they report.Tests. No existing test called these six methods. Each type's
*Tests.GenericMath.csgains three tests next to theTryWrite*tests, all throughBinaryIntegerHelper<T>:WriteBigEndianTestandWriteLittleEndianTestcheck the bytes, the return value and the exceptions, andWriteEndianDoesNotAllocateTestcalls the six methods twice and asserts that the second round allocates nothing. WithWriteLittleEndianinBinaryIntegerWritermade to return one byte too many, the fourteenWriteLittleEndianTestfailed and the fourteenWriteBigEndianTestpassed. With the six explicit implementations removed fromInt32andBigInteger, their allocation tests failed (144 and 192 bytes) and the other twelve passed. System.Runtime.Tests and System.Runtime.Numerics.Tests pass.Left out.
IFloatingPoint<T>has twelve default interface methods of the same shape (WriteExponent*andWriteSignificand*), implemented by nine types. I can send them as a follow-up PR.Performance
On
mainthe JIT already removes the box at many call sites once the code is optimized, so the difference depends on the call site. BenchmarkDotNet 0.15.2,[MemoryDiagnoser], three launches,mainand this branch as two Release test hosts that differ only inSystem.Private.CoreLib.dllandSystem.Runtime.Numerics.dll. Each call goes through a generic helper, either inlinable or[MethodImpl(MethodImplOptions.NoInlining)]. Mean ± standard deviation; wheremain's spread is wide its median is given as well, and * marks the rows for which BenchmarkDotNet warned thatmain's distribution is multimodal.mainlong.WriteBigEndian(byte[])BigInteger.WriteBigEndian(Span<byte>)long.WriteBigEndian(byte[])Int128.WriteLittleEndian(Span<byte>)int.WriteLittleEndian(Span<byte>)int.WriteLittleEndian(Span<byte>)Int128.WriteLittleEndian(Span<byte>)BigInteger.WriteBigEndian(Span<byte>)int.TryWriteLittleEndian(Span<byte>), controlOnly the first two rows allocate on
main. For the lastBigIntegerrow, which is slower on this branch,DOTNET_JitDisasmof the benchmark method, in a separate one-launch short run, gives the same Tier1 code on both hosts (185 lines, identical apart from addresses and labels). WithDOTNET_TieredCompilation=0,mainalso allocates for the single-argumentbyte[]overloads and for everyBigIntegeroverload; this branch allocates in none of them.Resolves #77969
Note
AI-generated, written at my direction and reviewed by me before posting. Everything ran on Windows 11 x64 (build 26200), on local builds of this branch and of
mainatbad7509cb82, the commit it is based on: the tests and the behaviour comparison on Debug libraries with a Checked runtime, the issue's repro, the benchmark, the disassembly and theDOTNET_TieredCompilation=0runs on Release builds; the issue's repro and the behaviour comparison also on the installed .NET 10.0.12. System.Runtime.Tests and System.Runtime.Numerics.Tests also passed on an x86 Checked build and on x64 Mono, with its JIT and withMONO_ENV_OPTIONS=--interpreter; nothing ran on NativeAOT, Mono AOT, WASM, Linux or macOS.