Avoid conv.i opcodes in hot paths in CoreLib #51190
Conversation
|
Is this worth a small perf analyzer? |
Maybe, but I'm really not sure what would be a good candidate for that analyzer. The best I can think of is any sign-extending conversion from a signed narrow type to an unsigned wide type. That is, |
|
I imagine it would be for well known cases, like |
|
There are definitely valid use cases for passing in negative |
|
Still weighing whether to add internal |
src/coreclr/System.Private.CoreLib/src/System/Buffer.CoreCLR.cs
Outdated
Show resolved
Hide resolved
|
The jit can't be improved instead? |
There are different semantics here, and the JIT needs to respect the semantics the dev / C# compiler wrote, or else it would potentially introduce a bug. Levi is only changing places where it won't be a bug. Just as an example, if you have: int i = -1;and then do: nuint n = (nuint)i;then nuint n = (nuint)(uint)i;then you end up with |
ghost
locked as resolved and limited conversation to collaborators
We have several places in CoreLib where we sign-extend a non-negative 32-bit value to the native word size. Changing these to use zero-extensions is somewhat more efficient from a codegen perspective.
The methodology here was very simplistic: ildasm all of System.Private.CoreLib.dll and look for calls to
conv.i. If the method fit on my screen or was marked aggressiveinlining, I assume it's potentially on a hot path, so it's a candidate for fixing in this PR. I intentionally excluded methods that were large or complex or where trying to avoid the sign extension would seem not worth the tradeoff. For example, with this PR there are still lots of places instringandArraythat perform sign-extension, but those extensions are performed within a larger unit of work and I didn't believe addressing them would provide any measurable benefit.I also checked calls to
Unsafe.Add(..., int), rewriting them to useUnsafe.Add(..., nint)(via zero-extension) following approximately the same criteria as above. Though TBH these would be a little cleaner if we were to add anUnsafe.Add(..., uint)overload, but that seems out of scope for this.Andy offline helped me run a diff tool against the binaries. Most of the improvements are expected. Some (like
Unsafe.Add) seem like false positives, since I'm not changing that code. The regressions I believe might be due to more leaf methods now being eligible candidates for inlining, but that's just a guess.Top file improvements (bytes): -630 : System.Private.CoreLib.dasm (-0.02% of base) 1 total files with Code Size differences (1 improved, 0 regressed), 0 unchanged. Top method regressions (bytes): 57 (142.50% of base) : System.Private.CoreLib.dasm - Internal.Runtime.CompilerServices.Unsafe:Add(byref,long):byref (8 base, 17 diff methods) 44 ( 2.56% of base) : System.Private.CoreLib.dasm - System.Buffers.Text.Utf8Parser:TryParseNumber(System.ReadOnlySpan`1[Byte],byref,byref,int,byref):bool 8 ( 3.43% of base) : System.Private.CoreLib.dasm - System.String:Concat(System.ReadOnlySpan`1[Char],System.ReadOnlySpan`1[Char]):System.String 4 ( 0.41% of base) : System.Private.CoreLib.dasm - System.Enum:InternalFlagsFormat(EnumInfo,long):System.String 4 ( 0.84% of base) : System.Private.CoreLib.dasm - System.String:Concat(System.ReadOnlySpan`1[Char],System.ReadOnlySpan`1[Char],System.ReadOnlySpan`1[Char],System.ReadOnlySpan`1[Char]):System.String 3 ( 0.86% of base) : System.Private.CoreLib.dasm - System.String:Concat(System.ReadOnlySpan`1[Char],System.ReadOnlySpan`1[Char],System.ReadOnlySpan`1[Char]):System.String 2 ( 1.44% of base) : System.Private.CoreLib.dasm - System.Exception:<ToString>g__Write|65_0(System.String,byref) Top method improvements (bytes): -19 (-5.46% of base) : System.Private.CoreLib.dasm - Internal.Runtime.CompilerServices.Unsafe:Add(byref,int):byref (39 base, 37 diff methods) -16 (-1.66% of base) : System.Private.CoreLib.dasm - System.Globalization.CompareInfo:Compare(System.String,int,int,System.String,int,int,int):int:this -14 (-2.14% of base) : System.Private.CoreLib.dasm - System.String:MakeSeparatorList(System.ReadOnlySpan`1[Char],byref):this -11 (-2.47% of base) : System.Private.CoreLib.dasm - System.Text.Encoding:GetBytes(long,int,long,int):int:this -11 (-2.46% of base) : System.Private.CoreLib.dasm - System.Text.Encoding:GetChars(long,int,long,int):int:this -7 (-2.80% of base) : System.Private.CoreLib.dasm - System.Text.Encoding:GetByteCount(long,int):int:this -7 (-2.83% of base) : System.Private.CoreLib.dasm - System.Text.Encoding:GetCharCount(long,int):int:this -7 (-0.58% of base) : System.Private.CoreLib.dasm - System.Collections.Generic.HashSet`1[__Canon][System.__Canon]:SymmetricExceptWithEnumerable(System.Collections.Generic.IEnumerable`1[__Canon]):this -6 (-4.80% of base) : System.Private.CoreLib.dasm - System.String:InitializeProbabilisticMap(long,System.ReadOnlySpan`1[Char]) -6 (-1.27% of base) : System.Private.CoreLib.dasm - System.Globalization.CompareInfo:IsSuffix(System.ReadOnlySpan`1[Char],System.ReadOnlySpan`1[Char],int):bool:this -5 (-4.07% of base) : System.Private.CoreLib.dasm - System.ReadOnlySpan`1[__Canon][System.__Canon]:ToArray():System.__Canon[]:this -5 (-4.07% of base) : System.Private.CoreLib.dasm - System.Span`1[__Canon][System.__Canon]:ToArray():System.__Canon[]:this -5 (-0.19% of base) : System.Private.CoreLib.dasm - System.Buffers.Text.Utf8Formatter:TryFormat(System.TimeSpan,System.Span`1[Byte],byref,System.Buffers.StandardFormat):bool -5 (-0.54% of base) : System.Private.CoreLib.dasm - System.Text.Encoding:GetBytesWithFallback(long,int,long,int,int,int,System.Text.EncoderNLS):int:this -5 (-0.75% of base) : System.Private.CoreLib.dasm - System.Text.Unicode.Utf8:ToUtf16(System.ReadOnlySpan`1[Byte],System.Span`1[Char],byref,byref,bool,bool):int -5 (-6.10% of base) : System.Private.CoreLib.dasm - System.ReadOnlySpan`1[TimeSpan][System.TimeSpan]:ToArray():System.TimeSpan[]:this -5 (-6.10% of base) : System.Private.CoreLib.dasm - System.ReadOnlySpan`1[GCGenerationInfo][System.GCGenerationInfo]:ToArray():System.GCGenerationInfo[]:this -5 (-6.17% of base) : System.Private.CoreLib.dasm - System.Span`1[Int16][System.Int16]:ToArray():System.Int16[]:this -5 (-6.10% of base) : System.Private.CoreLib.dasm - System.Span`1[Int64][System.Int64]:ToArray():System.Int64[]:this -5 (-6.10% of base) : System.Private.CoreLib.dasm - System.ReadOnlySpan`1[Single][System.Single]:ToArray():System.Single[]:this Top method regressions (percentages): 57 (142.50% of base) : System.Private.CoreLib.dasm - Internal.Runtime.CompilerServices.Unsafe:Add(byref,long):byref (8 base, 17 diff methods) 8 ( 3.43% of base) : System.Private.CoreLib.dasm - System.String:Concat(System.ReadOnlySpan`1[Char],System.ReadOnlySpan`1[Char]):System.String 44 ( 2.56% of base) : System.Private.CoreLib.dasm - System.Buffers.Text.Utf8Parser:TryParseNumber(System.ReadOnlySpan`1[Byte],byref,byref,int,byref):bool 2 ( 1.44% of base) : System.Private.CoreLib.dasm - System.Exception:<ToString>g__Write|65_0(System.String,byref) 3 ( 0.86% of base) : System.Private.CoreLib.dasm - System.String:Concat(System.ReadOnlySpan`1[Char],System.ReadOnlySpan`1[Char],System.ReadOnlySpan`1[Char]):System.String 4 ( 0.84% of base) : System.Private.CoreLib.dasm - System.String:Concat(System.ReadOnlySpan`1[Char],System.ReadOnlySpan`1[Char],System.ReadOnlySpan`1[Char],System.ReadOnlySpan`1[Char]):System.String 4 ( 0.41% of base) : System.Private.CoreLib.dasm - System.Enum:InternalFlagsFormat(EnumInfo,long):System.String Top method improvements (percentages): -3 (-11.11% of base) : System.Private.CoreLib.dasm - System.String:IsCharBitSet(long,ubyte):bool -3 (-10.00% of base) : System.Private.CoreLib.dasm - System.String:SetCharBit(long,ubyte) -3 (-9.68% of base) : System.Private.CoreLib.dasm - System.Span`1[KeyValuePair`2][System.Collections.Generic.KeyValuePair`2[System.Int32,System.__Canon]]:Clear():this -5 (-6.41% of base) : System.Private.CoreLib.dasm - System.ReadOnlySpan`1[SByte][System.SByte]:ToArray():System.SByte[]:this -5 (-6.41% of base) : System.Private.CoreLib.dasm - System.Span`1[Byte][System.Byte]:ToArray():System.Byte[]:this -5 (-6.41% of base) : System.Private.CoreLib.dasm - System.Span`1[Boolean][System.Boolean]:ToArray():System.Boolean[]:this -5 (-6.41% of base) : System.Private.CoreLib.dasm - System.ReadOnlySpan`1[Boolean][System.Boolean]:ToArray():System.Boolean[]:this -5 (-6.41% of base) : System.Private.CoreLib.dasm - System.ReadOnlySpan`1[Byte][System.Byte]:ToArray():System.Byte[]:this -5 (-6.41% of base) : System.Private.CoreLib.dasm - System.Span`1[SByte][System.SByte]:ToArray():System.SByte[]:this -5 (-6.17% of base) : System.Private.CoreLib.dasm - System.Span`1[Int16][System.Int16]:ToArray():System.Int16[]:this -5 (-6.17% of base) : System.Private.CoreLib.dasm - System.Span`1[Char][System.Char]:ToArray():System.Char[]:this -5 (-6.17% of base) : System.Private.CoreLib.dasm - System.ReadOnlySpan`1[Char][System.Char]:ToArray():System.Char[]:this -5 (-6.17% of base) : System.Private.CoreLib.dasm - System.ReadOnlySpan`1[UInt16][System.UInt16]:ToArray():System.UInt16[]:this -5 (-6.17% of base) : System.Private.CoreLib.dasm - System.ReadOnlySpan`1[Int16][System.Int16]:ToArray():System.Int16[]:this -5 (-6.17% of base) : System.Private.CoreLib.dasm - System.Span`1[UInt16][System.UInt16]:ToArray():System.UInt16[]:this -5 (-6.10% of base) : System.Private.CoreLib.dasm - System.ReadOnlySpan`1[TimeSpan][System.TimeSpan]:ToArray():System.TimeSpan[]:this -5 (-6.10% of base) : System.Private.CoreLib.dasm - System.ReadOnlySpan`1[GCGenerationInfo][System.GCGenerationInfo]:ToArray():System.GCGenerationInfo[]:this -5 (-6.10% of base) : System.Private.CoreLib.dasm - System.Span`1[Int64][System.Int64]:ToArray():System.Int64[]:this -5 (-6.10% of base) : System.Private.CoreLib.dasm - System.ReadOnlySpan`1[Single][System.Single]:ToArray():System.Single[]:this -5 (-6.10% of base) : System.Private.CoreLib.dasm - System.ReadOnlySpan`1[Int32][System.Int32]:ToArray():System.Int32[]:this 372 total methods with Code Size differences (365 improved, 7 regressed), 28698 unchanged.