Switch attribute equality to the valuetype equality plan #84095
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Attribute.Equals/GetHashCode is currently implemented using reflection. The implementation iterates over all instance fields and calls Equals/GetHashCode as needed. This has problems because the code is not actually trim safe (requires special casing in the compiler to work around an invalid suppression in CoreLib) and has extra problems for reflection blocking (we need to make sure the fields are never subject to blocking). In this PR I'm switching Attribute to a similar plan that .NET Native had. In .NET Native we injected a pair of virtual methods to read and access fields on System.Attribute descendants. We can actually get away with one method - I'm reusing all the infrastructure we have for ValueType.Equals/GetHashCode since the problem is the same. Saves 5.8% on a Hello World. For BasicMinimalApi, this is a wash (almost to the byte). It will become an improvement once RyuJIT starts generating better code for the field offset computations we're doing in the injected method. Contributes to dotnet#83069.
|
Tagging subscribers to this area: @agocke, @MichalStrehovsky, @jkotas Issue Details
This has problems because the code is not actually trim safe (requires special casing in the compiler to work around an invalid suppression in CoreLib) and has extra problems for reflection blocking (we need to make sure the fields are never subject to blocking). In this PR I'm switching Attribute to a similar plan that .NET Native had. In .NET Native we injected a pair of virtual methods to read and access fields on System.Attribute descendants. We can actually get away with one method - I'm reusing all the infrastructure we have for Saves 5.8% on a Hello World. For BasicMinimalApi, this is a wash (almost to the byte). It will become an improvement once RyuJIT starts generating better code for the field offset computations we're doing in the injected method. Contributes to #83069. Cc @dotnet/ilc-contrib
|
7 tasks
|
/azp run runtime-extra-platforms |
|
Azure Pipelines successfully started running 1 pipeline(s). |
jkotas
reviewed
Mar 30, 2023
src/coreclr/nativeaot/System.Private.CoreLib/src/System/Attribute.NativeAot.cs
Outdated
Show resolved
Hide resolved
jkotas
reviewed
Mar 30, 2023
src/coreclr/tools/Common/TypeSystem/IL/Stubs/ValueTypeGetFieldHelperMethodOverride.cs
Outdated
Show resolved
Hide resolved
jkotas
reviewed
Mar 30, 2023
src/coreclr/nativeaot/System.Private.CoreLib/src/System.Private.CoreLib.csproj
Outdated
Show resolved
Hide resolved
jakobbotsch
added a commit
that referenced
this pull request
Apr 4, 2023
Both during local morph and during VN. Fix #40021 Saves 3 KB on BasicMinimalApi after #84095 (there's 1111 __GetFieldHelper functions). About 0.04%. There's still a null check kept for each offset computation, which we cannot really get rid of, but NAOT could maybe emit the IL such that there is a dominating null check so that only one is emitted. Example: Base: ``` .managed:0000000140347CC0 loc_140347CC0: ; CODE XREF: S_P_CoreLib_System_Collections_Generic_KeyValuePair_2_System_Net_Security_System_Net_Security_SslSessionsCache_SslCredKey__System___Canon_____GetFieldHelper+23↑j .managed:0000000140347CC0 ; DATA XREF: .rdata:__readonlydata_S_P_CoreLib_System_Collections_Generic_KeyValuePair_2_System_Net_Security_System_Net_Security_SslSessionsCache_SslCredKey__System___Canon_____GetFieldHelper↓o .managed:0000000140347CC0 lea rax, ??_7Boxed_System_Net_Security_System_Net_Security_SslSessionsCache_SslCredKey@@6b@ ; jumptable 0000000140347CB3 case 0 .managed:0000000140347CC7 mov [r9], rax .managed:0000000140347CCA cmp [rcx], cl .managed:0000000140347CCC lea rax, [rcx+8] .managed:0000000140347CD0 sub rax, rcx .managed:0000000140347CD3 add rsp, 8 .managed:0000000140347CD7 retn ``` Diff: ``` .managed:0000000140347AA0 loc_140347AA0: ; CODE XREF: S_P_CoreLib_System_Collections_Generic_KeyValuePair_2_System_Net_Security_System_Net_Security_SslSessionsCache_SslCredKey__System___Canon_____GetFieldHelper+23↑j .managed:0000000140347AA0 ; DATA XREF: .rdata:__readonlydata_S_P_CoreLib_System_Collections_Generic_KeyValuePair_2_System_Net_Security_System_Net_Security_SslSessionsCache_SslCredKey__System___Canon_____GetFieldHelper↓o .managed:0000000140347AA0 lea rax, ??_7Boxed_System_Net_Security_System_Net_Security_SslSessionsCache_SslCredKey@@6b@ ; jumptable 0000000140347A93 case 0 .managed:0000000140347AA7 mov [r9], rax .managed:0000000140347AAA cmp [rcx], cl .managed:0000000140347AAC mov eax, 8 .managed:0000000140347AB1 add rsp, 8 .managed:0000000140347AB5 retn ``` Local morph changes handle the pattern for local structs -- VN changes handle the pattern for classes (and more complicated struct cases, like storing them in locals, which there are a few examples of in #40021).
ghost
locked as resolved and limited conversation to collaborators
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Attribute.Equals/GetHashCodeis currently implemented using reflection. The implementation iterates over all instance fields and callsEquals/GetHashCodeas needed.This has problems because the code is not actually trim safe (requires special casing in the compiler to work around an invalid suppression in CoreLib) and has extra problems for reflection blocking (we need to make sure the fields are never subject to blocking).
In this PR I'm switching Attribute to a similar plan that .NET Native had. In .NET Native we injected a pair of virtual methods to read and access fields on System.Attribute descendants. We can actually get away with one method - I'm reusing all the infrastructure we have for
ValueType.Equals/GetHashCodesince the problem is the same.Saves 5.8% on a Hello World. For BasicMinimalApi, this is a wash (almost to the byte). It will become an improvement once RyuJIT starts generating better code for the field offset computations we're doing in the injected method.
Contributes to #83069.
Cc @dotnet/ilc-contrib