Movatterモバイル変換


[0]ホーム

URL:


Skip to content

Navigation Menu

Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly

Sign up
Appearance settings

Avoid conv.i opcodes in hot paths in CoreLib#51190

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.

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

Already on GitHub?Sign in to your account

Merged

Conversation

@GrabYourPitchforks
Copy link
Member

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 ofSystem.Private.CoreLib.dll and look for calls toconv.i. If the method fit on my screen or was markedaggressiveinlining, 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 instring andArray that 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 toUnsafe.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 (likeUnsafe.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[]:thisTop 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.StringTop 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[]:this372 total methods with Code Size differences (365 improved, 7 regressed), 28698 unchanged.

@tannergooding
Copy link
Member

Is this worth a small perf analyzer?

@GrabYourPitchforks
Copy link
MemberAuthor

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,sbyte -> ushort,int -> ulong, etc. These conversions are well-defined and behave the same in C and C# as far as I can tell.

@tannergooding
Copy link
Member

I imagine it would be for well known cases, likeUnsafe.Add where the API takesIntPtr, but anint is being passed.
There are a likely a few other well-known cases for this as well, such asnint being passed into array indexers, etc.

@tannergooding
Copy link
Member

There are definitely valid use cases for passing in negativeint, but I'd expect those are more a minority and so the false positives would hopefully be minimal.

@GrabYourPitchforks
Copy link
MemberAuthor

Still weighing whether to add internalUnsafe.Add(..., [n]uint) methods. It makes the call sites ever so slightly cleaner, but it's a fairly big hammer.

@danmoseley
Copy link
Member

The jit can't be improved instead?

@stephentoub
Copy link
Member

stephentoub commentedApr 18, 2021
edited
Loading

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:

inti=-1;

and then do:

nuintn=(nuint)i;

thenn == ulong.MaxValue, because the -1 (0xFFFFFFFF) is sign extended (0xFFFFFFFFFFFFFFFF). But if you instead do:

nuintn=(nuint)(uint)i;

then you end up withn == uint.MaxValue, because the -1 (0xFFFFFFFF) is zero extended (0x00000000FFFFFFFF).

@GrabYourPitchforksGrabYourPitchforks merged commitde591a8 intodotnet:mainApr 20, 2021
@GrabYourPitchforksGrabYourPitchforks deleted the audit_convi branchApril 20, 2021 22:10
@ghostghost locked asresolvedand limited conversation to collaboratorsMay 20, 2021
Sign up for freeto subscribe to this conversation on GitHub. Already have an account?Sign in.

Reviewers

@stephentoubstephentoubstephentoub approved these changes

Assignees

No one assigned

Projects

None yet

Milestone

6.0.0

Development

Successfully merging this pull request may close these issues.

4 participants

@GrabYourPitchforks@tannergooding@danmoseley@stephentoub

[8]ページ先頭

©2009-2025 Movatter.jp