Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Adjust the usage of ReverseEndianness in BinaryPrimitives #69063

Merged
merged 1 commit into from
May 11, 2022
Merged

Adjust the usage of ReverseEndianness in BinaryPrimitives #69063

merged 1 commit into from
May 11, 2022

Conversation

aromaa
Copy link
Contributor

@aromaa aromaa commented May 9, 2022

Follow up on #66965

Adjusts the usage of ReverseEndianness in BinaryPrimitives to allow the JIT to recognize the pattern and optimize them to movbe

@dotnet-issue-labeler
Copy link

I couldn't figure out the best area label to add to this PR. If you have write-permissions please help me learn by adding exactly one area label.

@ghost ghost added the community-contribution Indicates that the PR has been added by a community member label May 9, 2022
@aromaa
Copy link
Contributor Author

aromaa commented May 9, 2022 •
edited
Loading

Diffs


Summary of Perf Score diffs:
(Lower is better)

Total PerfScoreUnits of base: 2.4243817735148437E+31
Total PerfScoreUnits of diff: 2.4243817735148437E+31
Total PerfScoreUnits of delta: 102,08 (0.00 % of base)
Total relative delta: 0.88
    diff is a regression.
    relative diff is a regression.
Detail diffs 


Top file regressions (PerfScoreUnits):
       50,74 : System.Private.CoreLib.dasm (0,00 % of base)
       13,85 : System.Security.Cryptography.dasm (0,01 % of base)
       11,79 : Microsoft.CodeAnalysis.dasm (0,00 % of base)
       10,65 : System.IO.Hashing.dasm (0,24 % of base)
       10,05 : System.Private.Xml.dasm (0,00 % of base)
        8,18 : System.Net.Http.dasm (0,00 % of base)
        3,35 : System.Net.Primitives.dasm (0,01 % of base)
        2,90 : System.Speech.dasm (0,00 % of base)
        2,85 : System.Net.Quic.dasm (0,01 % of base)
        2,70 : System.Net.NetworkInformation.dasm (0,01 % of base)
        2,70 : System.Net.Ping.dasm (0,03 % of base)
        1,05 : System.Net.Security.dasm (0,00 % of base)
        1,02 : System.Formats.Cbor.dasm (0,01 % of base)
        0,55 : System.Transactions.Local.dasm (0,00 % of base)
        0,50 : System.Private.Uri.dasm (0,00 % of base)

Top file improvements (PerfScoreUnits):
      -17,25 : System.IO.Compression.dasm (-0,02 % of base)
       -1,85 : System.Drawing.Common.dasm (-0,00 % of base)
       -1,70 : System.Net.Sockets.dasm (-0,00 % of base)

18 total files with Perf Score differences (3 improved, 15 regressed), 253 unchanged.

Top method regressions (PerfScoreUnits):
       47,35 (11,96 % of base) : System.Private.CoreLib.dasm - ManifestBasedResourceGroveler:CreateResourceSet(Stream,Assembly):ResourceSet:this
       10,60 (1,20 % of base) : System.Private.Xml.dasm - XmlSqlBinaryReader:ValueAs(int,Type,IXmlNamespaceResolver):Object:this
        9,70 (0,61 % of base) : Microsoft.CodeAnalysis.dasm - Win32ResourceConversions:AppendIconToResourceStream(Stream,Stream)
        7,80 (5,83 % of base) : System.IO.Hashing.dasm - State:ProcessStripe(ReadOnlySpan`1):this (2 methods)
        5,85 (1,15 % of base) : System.Security.Cryptography.dasm - CapiHelper:ToDSAParameters(ref,bool,ref):DSAParameters
        5,65 (1,74 % of base) : System.Security.Cryptography.dasm - CapiHelper:ToRSAParameters(ref,bool):RSAParameters
        3,80 (1,73 % of base) : System.Net.Http.dasm - Http2Connection:ProcessSettingsFrame(FrameHeader,bool):this
        2,90 (0,96 % of base) : System.Speech.dasm - PhoneMapData:.ctor(Stream):this
        2,70 (1,72 % of base) : System.Net.NetworkInformation.dasm - SocketAddress:GetHashCode():int:this
        2,70 (1,72 % of base) : System.Net.Ping.dasm - SocketAddress:GetHashCode():int:this
        2,70 (1,72 % of base) : System.Net.Primitives.dasm - SocketAddress:GetHashCode():int:this
        2,70 (1,72 % of base) : System.Net.Quic.dasm - SocketAddress:GetHashCode():int:this
        2,70 (1,72 % of base) : System.Net.Sockets.dasm - SocketAddress:GetHashCode():int:this
        2,09 (0,18 % of base) : Microsoft.CodeAnalysis.dasm - COFFResourceReader:ReadWin32ResourcesFromCOFF(Stream):ResourceSection
        1,90 (1,20 % of base) : System.Private.Xml.dasm - Ucs4Decoder1234:GetFullChars(ref,int,int,ref,int):int:this
        1,05 (5,00 % of base) : System.IO.Hashing.dasm - State:ApplyRound(long,ReadOnlySpan`1):long
        0,90 (1,42 % of base) : System.Security.Cryptography.dasm - DES:IsSemiWeakKey(ref):bool
        0,90 (0,46 % of base) : System.Net.Http.dasm - Http2Connection:ProcessPingFrame(FrameHeader):this
        0,90 (6,02 % of base) : System.IO.Hashing.dasm - Crc64:GetCurrentHashCore(Span`1):this
        0,90 (6,64 % of base) : System.Private.CoreLib.dasm - BinaryPrimitives:ReadInt64BigEndian(ReadOnlySpan`1):long

Top method improvements (PerfScoreUnits):
      -10,10 (-2,34 % of base) : System.IO.Compression.dasm - ZipGenericExtraField:ParseExtraField(Stream):List`1
       -7,15 (-0,95 % of base) : System.IO.Compression.dasm - Zip64ExtraField:GetJustZip64Block(Stream,bool,bool,bool,bool):Zip64ExtraField
       -4,55 (-0,80 % of base) : System.Net.Sockets.dasm - Socket:.ctor(SafeSocketHandle,bool):this
       -2,30 (-0,47 % of base) : System.Private.Xml.dasm - XmlSqlBinaryReader:ValueAsObject(int,bool):Object:this
       -1,85 (-1,68 % of base) : System.Drawing.Common.dasm - ImageConverter:GetBitmapStream(ReadOnlySpan`1):Stream
       -1,80 (-11,54 % of base) : System.Private.CoreLib.dasm - BinaryPrimitives:ReadInt16BigEndian(ReadOnlySpan`1):short
       -0,85 (-6,07 % of base) : System.Private.CoreLib.dasm - BinaryPrimitives:WriteInt16BigEndian(Span`1,short)
       -0,73 (-5,60 % of base) : System.Private.CoreLib.dasm - BinaryPrimitives:TryWriteInt16BigEndian(Span`1,short):bool
       -0,65 (-0,33 % of base) : System.Private.Xml.dasm - XmlSqlBinaryReader:ValueAsDateTimeString():String:this
       -0,55 (-3,99 % of base) : System.Private.CoreLib.dasm - BinaryPrimitives:WriteUInt16BigEndian(Span`1,ushort)
       -0,43 (-3,35 % of base) : System.Private.CoreLib.dasm - BinaryPrimitives:TryWriteUInt16BigEndian(Span`1,ushort):bool
       -0,15 (-0,37 % of base) : System.Net.Primitives.dasm - SocketAddress:GetIPEndPoint():IPEndPoint:this
       -0,15 (-0,37 % of base) : System.Net.Quic.dasm - SocketAddress:GetIPEndPoint():IPEndPoint:this
       -0,15 (-0,37 % of base) : System.Net.Sockets.dasm - SocketAddress:GetIPEndPoint():IPEndPoint:this
       -0,15 (-0,32 % of base) : System.Net.Security.dasm - TlsFrameHelper:TryGetSniFromServerNameList(ReadOnlySpan`1,byref):bool
       -0,05 (-0,21 % of base) : System.Net.Security.dasm - TlsFrameHelper:SkipOpaqueType2(ReadOnlySpan`1):ReadOnlySpan`1

Top method regressions (percentages):
       47,35 (11,96 % of base) : System.Private.CoreLib.dasm - ManifestBasedResourceGroveler:CreateResourceSet(Stream,Assembly):ResourceSet:this
        0,90 (6,64 % of base) : System.Private.CoreLib.dasm - BinaryPrimitives:ReadInt64BigEndian(ReadOnlySpan`1):long
        0,90 (6,64 % of base) : System.Private.CoreLib.dasm - BinaryPrimitives:ReadUInt64BigEndian(ReadOnlySpan`1):long
        0,90 (6,02 % of base) : System.IO.Hashing.dasm - Crc64:GetCurrentHashCore(Span`1):this
        7,80 (5,83 % of base) : System.IO.Hashing.dasm - State:ProcessStripe(ReadOnlySpan`1):this (2 methods)
        0,90 (5,36 % of base) : System.IO.Hashing.dasm - Crc64:GetHashAndResetCore(Span`1):this
        1,05 (5,00 % of base) : System.IO.Hashing.dasm - State:ApplyRound(long,ReadOnlySpan`1):long
        0,55 (4,70 % of base) : System.Private.CoreLib.dasm - BinaryPrimitives:TryWriteInt64BigEndian(Span`1,long):bool
        0,55 (4,70 % of base) : System.Private.CoreLib.dasm - BinaryPrimitives:TryWriteUInt64BigEndian(Span`1,long):bool
        0,55 (4,38 % of base) : System.Private.CoreLib.dasm - BinaryPrimitives:WriteInt64BigEndian(Span`1,long)
        0,55 (4,38 % of base) : System.Private.CoreLib.dasm - BinaryPrimitives:WriteUInt64BigEndian(Span`1,long)
        0,45 (4,09 % of base) : System.Private.CoreLib.dasm - BinaryPrimitives:TryWriteInt32BigEndian(Span`1,int):bool
        0,45 (4,09 % of base) : System.Private.CoreLib.dasm - BinaryPrimitives:TryWriteUInt32BigEndian(Span`1,int):bool
        0,50 (3,89 % of base) : System.Private.CoreLib.dasm - BinaryPrimitives:ReadInt32BigEndian(ReadOnlySpan`1):int
        0,50 (3,89 % of base) : System.Private.CoreLib.dasm - BinaryPrimitives:ReadUInt32BigEndian(ReadOnlySpan`1):int
        0,45 (3,80 % of base) : System.Private.CoreLib.dasm - BinaryPrimitives:WriteInt32BigEndian(Span`1,int)
        0,45 (3,80 % of base) : System.Private.CoreLib.dasm - BinaryPrimitives:WriteUInt32BigEndian(Span`1,int)
        0,83 (2,44 % of base) : System.Net.Http.dasm - VariableLengthIntegerHelper:TryWrite(Span`1,long,byref):bool
        0,30 (2,19 % of base) : System.Private.CoreLib.dasm - BinaryPrimitives:ReadUInt16BigEndian(ReadOnlySpan`1):ushort
        0,50 (1,95 % of base) : System.Private.Xml.dasm - Compiler:GetPersistentHashCode(String):int

Top method improvements (percentages):
       -1,80 (-11,54 % of base) : System.Private.CoreLib.dasm - BinaryPrimitives:ReadInt16BigEndian(ReadOnlySpan`1):short
       -0,85 (-6,07 % of base) : System.Private.CoreLib.dasm - BinaryPrimitives:WriteInt16BigEndian(Span`1,short)
       -0,73 (-5,60 % of base) : System.Private.CoreLib.dasm - BinaryPrimitives:TryWriteInt16BigEndian(Span`1,short):bool
       -0,55 (-3,99 % of base) : System.Private.CoreLib.dasm - BinaryPrimitives:WriteUInt16BigEndian(Span`1,ushort)
       -0,43 (-3,35 % of base) : System.Private.CoreLib.dasm - BinaryPrimitives:TryWriteUInt16BigEndian(Span`1,ushort):bool
      -10,10 (-2,34 % of base) : System.IO.Compression.dasm - ZipGenericExtraField:ParseExtraField(Stream):List`1
       -1,85 (-1,68 % of base) : System.Drawing.Common.dasm - ImageConverter:GetBitmapStream(ReadOnlySpan`1):Stream
       -7,15 (-0,95 % of base) : System.IO.Compression.dasm - Zip64ExtraField:GetJustZip64Block(Stream,bool,bool,bool,bool):Zip64ExtraField
       -4,55 (-0,80 % of base) : System.Net.Sockets.dasm - Socket:.ctor(SafeSocketHandle,bool):this
       -2,30 (-0,47 % of base) : System.Private.Xml.dasm - XmlSqlBinaryReader:ValueAsObject(int,bool):Object:this
       -0,15 (-0,37 % of base) : System.Net.Primitives.dasm - SocketAddress:GetIPEndPoint():IPEndPoint:this
       -0,15 (-0,37 % of base) : System.Net.Quic.dasm - SocketAddress:GetIPEndPoint():IPEndPoint:this
       -0,15 (-0,37 % of base) : System.Net.Sockets.dasm - SocketAddress:GetIPEndPoint():IPEndPoint:this
       -0,65 (-0,33 % of base) : System.Private.Xml.dasm - XmlSqlBinaryReader:ValueAsDateTimeString():String:this
       -0,15 (-0,32 % of base) : System.Net.Security.dasm - TlsFrameHelper:TryGetSniFromServerNameList(ReadOnlySpan`1,byref):bool
       -0,05 (-0,21 % of base) : System.Net.Security.dasm - TlsFrameHelper:SkipOpaqueType2(ReadOnlySpan`1):ReadOnlySpan`1

73 total methods with Perf Score differences (16 improved, 57 regressed), 266510 unchanged.

Regressions are due to different register allocation and decisions to inline more methods. Some methods show up as regression due to #66965 (comment)

@ghost
Copy link

ghost commented May 9, 2022

Tagging subscribers to this area: @dotnet/area-system-buffers
See info in area-owners.md if you want to be subscribed.

Issue Details

Follow up on #66965

Adjusts the usage of ReverseEndianness in BinaryPrimitives to allow the JIT to recognize the pattern and optimize them to movbe

Author: aromaa
Assignees: -
Labels:

area-System.Buffers, optimization, community-contribution
Milestone: -

tannergooding
tannergooding reviewed May 9, 2022
View reviewed changes
src/libraries/System.Private.CoreLib/src/System/Buffers/Binary/ReaderBigEndian.cs
}
return result;
return BitConverter.IsLittleEndian ?
ReverseEndianness(MemoryMarshal.Read<short>(source)) :
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The JIT is reordering the IR so that the ReverseEndianness occurs directly over the Unsafe.ReadUnaligned and emitting just movbe reg, [addr]? The throw helpers/checks and inlining boundaries aren't getting in the way here?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, thats the case. We end up generating:

mov      rax, bword ptr [rcx]
mov      ecx, dword ptr [rcx+8]
cmp      ecx, 4
jl       SHORT G_M53413_IG04
movbe    eax, dword ptr [rax]

@jkotas jkotas merged commit 9b3eabc into dotnet:main May 11, 2022
@aromaa aromaa deleted the fix/binary-primitives branch May 11, 2022 17:14
@jkotas jkotas mentioned this pull request May 20, 2022
Merged
@ghost ghost locked as resolved and limited conversation to collaborators Jun 10, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@GrabYourPitchforks GrabYourPitchforks GrabYourPitchforks approved these changes

@tannergooding tannergooding tannergooding approved these changes

Assignees
No one assigned
Labels
area-System.Buffers community-contribution Indicates that the PR has been added by a community member optimization
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@aromaa @GrabYourPitchforks @tannergooding @jkotas