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.

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

Fold 'static readonly struct' to constants #77102

Merged
merged 9 commits into from
Oct 19, 2022

Conversation

EgorBo
Copy link
Member

@EgorBo EgorBo commented Oct 16, 2022

static readonly DateTime date = DateTime.Now;

// Return jit-time constant
DateTime Test1() => date;

// DateTime.MinValue is a static readonly field
bool Test2(DateTime dt) => dt != DateTime.MinValue;

Codegen diff:

; Assembly listing for method Program:Test1():System.DateTime:this
-      48B8C01E00DAC1010000 mov      rax, 0x1C1DA001EC0
-      488B00               mov      rax, gword ptr [rax]
-      488B4008             mov      rax, qword ptr [rax+08H]
+      48B8F0BCFF19B4AFDA88 mov      rax, 0x88DAAFB419FFBCF0 ;; represents DateTime.Now when field was inited
       C3                   ret      

-; Total bytes of code 18
+; Total bytes of code 11


; Assembly listing for method Program:Test2(System.DateTime):bool:this
-      48B80002804AF9010000 mov      rax, 0x1F94A800200
-      488B00               mov      rax, gword ptr [rax]
-      488B4008             mov      rax, qword ptr [rax+08H]
-      4833C2               xor      rax, rdx
-      48C1E002             shl      rax, 2
+      48C1E202             shl      rdx, 2
       0F95C0               setne    al
       0FB6C0               movzx    rax, al
       C3                   ret      
-; Total bytes of code 31
+; Total bytes of code 11

jit-diffs (-f --pmi --cctors):

Total bytes of base: 63490090
Total bytes of diff: 63482150
Total bytes of delta: -7940 (-0.01 % of base)
Total relative delta: -60.08
    diff is an improvement.
    relative diff is an improvement.


Total byte diff includes -103 bytes from reconciling methods
        Base had    1 unique methods,      103 unique bytes
        Diff had    0 unique methods,        0 unique bytes

Top file improvements (bytes):
       -1600 : System.Private.CoreLib.dasm (-0.03% of base)
        -755 : System.Net.Http.dasm (-0.09% of base)
        -736 : System.Data.Common.dasm (-0.04% of base)
        -423 : System.Runtime.Caching.dasm (-0.71% of base)
        -298 : System.Management.dasm (-0.07% of base)
        -295 : System.Security.Cryptography.dasm (-0.03% of base)
        -256 : System.Text.RegularExpressions.dasm (-0.04% of base)
        -249 : System.Speech.dasm (-0.05% of base)
        -220 : System.Threading.RateLimiting.dasm (-0.13% of base)
        -214 : Newtonsoft.Json.dasm (-0.02% of base)
        -205 : System.Transactions.Local.dasm (-0.09% of base)
        -191 : System.Net.Requests.dasm (-0.15% of base)
        -174 : System.Configuration.ConfigurationManager.dasm (-0.04% of base)
        -168 : Microsoft.Diagnostics.Tracing.TraceEvent.dasm (-0.00% of base)
        -158 : System.Net.Quic.dasm (-0.16% of base)
        -150 : System.Private.Xml.dasm (-0.00% of base)
        -145 : System.Drawing.Common.dasm (-0.03% of base)
        -121 : System.Net.Primitives.dasm (-0.18% of base)
        -117 : Microsoft.CodeAnalysis.dasm (-0.01% of base)
        -116 : Microsoft.VisualBasic.Core.dasm (-0.02% of base)

49 total files with Code Size differences (49 improved, 0 regressed), 225 unchanged.

Top method regressions (bytes):
          45 ( 0.33% of base) : System.Net.Http.dasm - <SendAsyncCore>d__61:MoveNext():this
          12 ( 5.41% of base) : System.Private.CoreLib.dasm - System.TimeZoneInfo:ConvertUtcToTimeZone(long,System.TimeZoneInfo,byref):System.DateTime
           4 ( 1.05% of base) : System.Private.CoreLib.dasm - System.TimeZoneInfo:ConvertToFromUtc(System.DateTime,System.TimeSpan,System.TimeSpan,bool):System.DateTime:this

Top method improvements (bytes):
        -109 (-12.22% of base) : Newtonsoft.Json.dasm - Newtonsoft.Json.Converters.UnixDateTimeConverter:WriteJson(Newtonsoft.Json.JsonWriter,System.Object,Newtonsoft.Json.JsonSerializer):this
        -103 (-100.00% of base) : Microsoft.CodeAnalysis.dasm - Microsoft.CodeAnalysis.SmallDictionary`2[System.__Canon,int]:RightComplex(AvlNode[System.__Canon,int]):AvlNode[System.__Canon,int] (1 base, 0 diff methods)
         -94 (-5.68% of base) : System.Net.Http.dasm - System.Net.Http.HttpConnectionPool:HandleAltSvc(System.Collections.Generic.IEnumerable`1[System.String],System.Nullable`1[System.TimeSpan]):this
         -91 (-26.84% of base) : System.Net.Http.dasm - System.Net.Http.HttpConnectionSettings:.ctor():this
         -80 (-13.05% of base) : System.Private.CoreLib.dasm - System.TimeZoneInfo:GetUtcOffsetFromUtc(System.DateTime,System.TimeZoneInfo,byref,byref):System.TimeSpan
         -75 (-8.81% of base) : Microsoft.Diagnostics.Tracing.TraceEvent.dasm - Microsoft.Diagnostics.Tracing.BPerfEventSource:.ctor(System.String,Microsoft.Diagnostics.Tracing.TraceEventDispatcherOptions,ubyte[],ubyte[],bool,Microsoft.Diagnostics.Tracing.BPerfEventSourceDecompress):this
         -67 (-1.26% of base) : System.Private.CoreLib.dasm - System.DateTimeParse:ParseByFormat(byref,byref,byref,System.Globalization.DateTimeFormatInfo,byref):bool
         -64 (-4.96% of base) : System.Private.CoreLib.dasm - AdjustmentRule:ValidateAdjustmentRule(System.DateTime,System.DateTime,System.TimeSpan,TransitionTime,TransitionTime,bool)
         -62 (-26.61% of base) : System.Net.Http.dasm - System.Net.Http.HttpConnectionPoolManager:SetCleaningTimer(System.TimeSpan):this
         -56 (-1.61% of base) : Microsoft.VisualBasic.Core.dasm - Microsoft.VisualBasic.CompilerServices.Operators:CompareObject2(System.Object,System.Object,bool):int
         -53 (-2.78% of base) : System.Private.DataContractSerialization.dasm - System.Xml.XmlConverter:TryParseDateTime(ubyte[],int,int,byref):bool
         -47 (-11.08% of base) : System.Runtime.Caching.dasm - System.Runtime.Caching.MemoryCacheEntry:.ctor(System.String,System.Object,System.DateTimeOffset,System.TimeSpan,int,System.Collections.ObjectModel.Collection`1[System.Runtime.Caching.ChangeMonitor],System.Runtime.Caching.CacheEntryRemovedCallback,System.Runtime.Caching.MemoryCache):this
         -45 (-21.63% of base) : Microsoft.Diagnostics.Tracing.TraceEvent.dasm - Microsoft.Diagnostics.Tracing.TraceEventSource:QPCTimeToDateTimeUTC(long):System.DateTime:this
         -45 (-11.60% of base) : System.Private.CoreLib.dasm - System.DateTimeParse:DateTimeOffsetTimeZonePostProcessing(byref,int):bool
         -45 (-11.57% of base) : System.Private.CoreLib.dasm - System.TimeZone:CalculateUtcOffset(System.DateTime,System.Globalization.DaylightTime):System.TimeSpan
         -44 (-21.67% of base) : System.Runtime.Caching.dasm - System.Runtime.Caching.UsageBucket:AddUsageEntryToFreeList(System.Runtime.Caching.UsageEntryRef):this
         -43 (-2.42% of base) : System.Net.Requests.dasm - System.Net.HttpWebRequest:AddCacheControlHeaders(System.Net.Http.HttpRequestMessage):this
         -43 (-3.08% of base) : System.Runtime.Caching.dasm - System.Runtime.Caching.ExpiresBucket:FlushExpiredItems(System.DateTime,bool):int:this
         -42 (-50.60% of base) : System.Data.Common.dasm - System.Data.SqlTypes.SqlBoolean:op_BitwiseAnd(System.Data.SqlTypes.SqlBoolean,System.Data.SqlTypes.SqlBoolean):System.Data.SqlTypes.SqlBoolean
         -42 (-50.60% of base) : System.Data.Common.dasm - System.Data.SqlTypes.SqlBoolean:op_BitwiseOr(System.Data.SqlTypes.SqlBoolean,System.Data.SqlTypes.SqlBoolean):System.Data.SqlTypes.SqlBoolean

Top method regressions (percentages):
          12 ( 5.41% of base) : System.Private.CoreLib.dasm - System.TimeZoneInfo:ConvertUtcToTimeZone(long,System.TimeZoneInfo,byref):System.DateTime
           4 ( 1.05% of base) : System.Private.CoreLib.dasm - System.TimeZoneInfo:ConvertToFromUtc(System.DateTime,System.TimeSpan,System.TimeSpan,bool):System.DateTime:this
          45 ( 0.33% of base) : System.Net.Http.dasm - <SendAsyncCore>d__61:MoveNext():this

Top method improvements (percentages):
        -103 (-100.00% of base) : Microsoft.CodeAnalysis.dasm - Microsoft.CodeAnalysis.SmallDictionary`2[System.__Canon,int]:RightComplex(AvlNode[System.__Canon,int]):AvlNode[System.__Canon,int] (1 base, 0 diff methods)
         -15 (-83.33% of base) : System.Private.CoreLib.dasm - System.Globalization.Calendar:get_MinSupportedDateTime():System.DateTime:this
         -15 (-83.33% of base) : System.Private.CoreLib.dasm - System.Globalization.GregorianCalendar:get_MinSupportedDateTime():System.DateTime:this
         -15 (-83.33% of base) : System.Private.CoreLib.dasm - System.Globalization.JulianCalendar:get_MinSupportedDateTime():System.DateTime:this
         -15 (-83.33% of base) : System.Private.CoreLib.dasm - System.Globalization.KoreanCalendar:get_MinSupportedDateTime():System.DateTime:this
         -15 (-83.33% of base) : System.Private.CoreLib.dasm - System.Globalization.ThaiBuddhistCalendar:get_MinSupportedDateTime():System.DateTime:this
         -14 (-82.35% of base) : Microsoft.CodeAnalysis.CSharp.dasm - AnonymousTypePublicSymbol:GetAttributeUsageInfo():Microsoft.CodeAnalysis.AttributeUsageInfo:this
         -14 (-82.35% of base) : Microsoft.CodeAnalysis.CSharp.dasm - AnonymousTypeTemplateSymbol:GetAttributeUsageInfo():Microsoft.CodeAnalysis.AttributeUsageInfo:this
         -21 (-65.62% of base) : System.Security.Cryptography.dasm - System.Security.Cryptography.X509Certificates.X509Certificate:.ctor():this
         -11 (-64.71% of base) : System.Configuration.ConfigurationManager.dasm - System.Configuration.MgmtConfigurationRecord:get_ClassFlags():System.Configuration.SimpleBitVector32:this
         -11 (-64.71% of base) : System.Configuration.ConfigurationManager.dasm - System.Configuration.RuntimeConfigurationRecord:get_ClassFlags():System.Configuration.SimpleBitVector32:this
         -26 (-61.90% of base) : System.Drawing.Common.dasm - System.Drawing.Printing.TriState:op_Implicit(bool):System.Drawing.Printing.TriState
         -42 (-60.00% of base) : System.Data.Common.dasm - System.Data.SqlTypes.SqlBoolean:op_LogicalNot(System.Data.SqlTypes.SqlBoolean):System.Data.SqlTypes.SqlBoolean
         -13 (-59.09% of base) : System.Text.RegularExpressions.dasm - System.Text.RegularExpressions.Regex:.ctor():this
         -17 (-58.62% of base) : System.Drawing.Common.dasm - System.Drawing.Printing.TriState:get_IsDefault():bool:this
         -17 (-58.62% of base) : System.Drawing.Common.dasm - System.Drawing.Printing.TriState:get_IsNotDefault():bool:this
         -15 (-57.69% of base) : System.Threading.RateLimiting.dasm - System.Threading.RateLimiting.FixedWindowRateLimiterOptions:.ctor():this
         -15 (-57.69% of base) : System.Threading.RateLimiting.dasm - System.Threading.RateLimiting.SlidingWindowRateLimiterOptions:.ctor():this
         -15 (-57.69% of base) : System.Threading.RateLimiting.dasm - System.Threading.RateLimiting.TokenBucketRateLimiterOptions:.ctor():this
         -16 (-55.17% of base) : System.Drawing.Common.dasm - System.Drawing.Printing.TriState:get_IsFalse():bool:this

434 total methods with Code Size differences (431 improved, 3 regressed), 391584 unchanged.

@dotnet-issue-labeler dotnet-issue-labeler bot added the area-CodeGen-coreclr CLR JIT compiler in src/coreclr/src/jit and related components such as SuperPMI label Oct 16, 2022
@ghost ghost assigned EgorBo Oct 16, 2022
@ghost
Copy link

ghost commented Oct 16, 2022

Tagging subscribers to this area: @JulieLeeMSFT, @jakobbotsch
See info in area-owners.md if you want to be subscribed.

Issue Details
static readonly DateTime date = DateTime.Now;

DateTime Test() => date;

Codegen diff:

; Assembly listing for method Program:Test():System.DateTime:this
-      48B8C01E00DAC1010000 mov      rax, 0x1C1DA001EC0
-      488B00               mov      rax, gword ptr [rax]
-      488B4008             mov      rax, qword ptr [rax+08H]
+      48B8F0BCFF19B4AFDA88 mov      rax, 0x88DAAFB419FFBCF0
       C3                   ret      

-; Total bytes of code 18
+; Total bytes of code 11
Author: EgorBo
Assignees: -
Labels:

area-CodeGen-coreclr

Milestone: -

@EgorBo EgorBo force-pushed the static-readonly-struct branch from 41d2ffa to 432cf04 Compare October 16, 2022 19:33
@omariom
Copy link
Contributor

omariom commented Oct 16, 2022

What kind of structs are supported? Single-fields only?

@EgorBo
Copy link
Member Author

EgorBo commented Oct 16, 2022

What kind of structs are supported? Single-fields only?

With the current impl up to 4 fields of primitive types

@En3Tho
Copy link
Contributor

En3Tho commented Oct 17, 2022

@EgorBo does this mean that Guid is out of the equation? It's layout is quite complex but I think comparisons with Guid.Empty are quite common

@EgorBo
Copy link
Member Author

EgorBo commented Oct 17, 2022

@EgorBo does this mean that Guide is out of the equation? It's layout is quite complex but I think comparisons with Guide.Empty are quite common

Yes, I plan to play with guid separately

@EgorBo EgorBo marked this pull request as ready for review October 17, 2022 10:05
@EgorBo
Copy link
Member Author

EgorBo commented Oct 17, 2022

@dotnet/jit-contrib PTAL, I limited it to single-field structs for now to avoid unexpected regressions due to promotions and since mostly TimeSpan/DateTime/TimeOnly/DateOnly/DateTimeOffset benefit from this which are single-field structs.

I plan to play with it in on VN-level and with larger structs such as Guid in future PRs.

@EgorBo
Copy link
Member Author

EgorBo commented Oct 17, 2022

This works for NativeAOT as well, although, DateTime.MinValue is not handled because MinValue is not initialized to default - https://github.com/dotnet/runtime/blob/main/src/libraries/System.Private.CoreLib/src/System/DateTime.cs#L131 so PreinitializationManager::IsPreinitialized returns false. cc @MichalStrehovsky

@MichalStrehovsky
Copy link
Member

This works for NativeAOT as well, although, DateTime.MinValue is not handled because MinValue is not initialized to default - https://github.com/dotnet/runtime/blob/main/src/libraries/System.Private.CoreLib/src/System/DateTime.cs#L131 so PreinitializationManager::IsPreinitialized returns false. cc @MichalStrehovsky

Not close to computers these days but pretty sure DateTime is not preinitialized because of leap seconds support stuff that does pinvokes and can't be interpreted at compile time. We would have to factor that out to a spearate cctor or something.

@EgorBo
Copy link
Member Author

EgorBo commented Oct 17, 2022

@AndyAyersMS @jakobbotsch PTAL

@EgorBo
Copy link
Member Author

EgorBo commented Oct 17, 2022

/azp run runtime-coreclr jitstress, runtime-coreclr outerloop, Fuzzlyn

@azure-pipelines
Copy link

Azure Pipelines successfully started running 3 pipeline(s).

src/coreclr/jit/importer.cpp Outdated Show resolved Hide resolved
src/coreclr/jit/importer.cpp Outdated Show resolved Hide resolved
src/coreclr/jit/importer.cpp Outdated Show resolved Hide resolved
@jkotas
Copy link
Member

jkotas commented Oct 18, 2022

pretty sure DateTime is not preinitialized because of leap seconds support stuff that does pinvokes and can't be interpreted at compile time. We would have to factor that out to a spearate cctor or something.

#77163

Copy link
Member

@AndyAyersMS AndyAyersMS left a comment

Choose a reason for hiding this comment

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

Thanks for reworking all this into impImportStaticReadOnlyField.

@EgorBo EgorBo merged commit 888ad2b into dotnet:main Oct 19, 2022
@EgorBo EgorBo deleted the static-readonly-struct branch October 21, 2022 16:04
@ghost ghost locked as resolved and limited conversation to collaborators Nov 20, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
area-CodeGen-coreclr CLR JIT compiler in src/coreclr/src/jit and related components such as SuperPMI
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants