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

Allocate boxed static structs on Frozen Object Heap #77737

Merged
merged 41 commits into from
Nov 13, 2022

Conversation

EgorBo
Copy link
Member

@EgorBo EgorBo commented Nov 1, 2022

Contributes to #76151

All static fields of value types (non-primitive) are usually boxed and stored in the normal heap. This PR moves them to the Frozen Object Heap, it allows us to avoid an indirect load when we access them (we no longer need a pinned handle to the "movable" object).

Example:

static DateTime Date { get; set; }

Codegen for the getter:

; Assembly listing for method Proga:get_Date():System.DateTime
; Tier-1 compilation
-      48B8C01E00BDFC010000 mov      rax, 0x1FCBD001EC0      ; box for Proga:<Date>k__BackingField
-      488B00               mov      rax, gword ptr [rax]
-      488B4008             mov      rax, qword ptr [rax+08H]
+      48B8C01EC0BC2B010000 mov      rax, 0x12BBCC01EC0      ; data for Proga:<Date>k__BackingField
+      488B00               mov      rax, qword ptr [rax]
       C3                   ret      
-; Total bytes of code 18
+; Total bytes of code 14

Diffs for Libraries (jit-utils -f -pmi):

Summary of Code Size diffs:
(Lower is better)

Total bytes of base: 59050516
Total bytes of diff: 59014832
Total bytes of delta: -35684 (-0.06 % of base)
Total relative delta: -214.62
    diff is an improvement.
    relative diff is an improvement.


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

Top file regressions (bytes):
         339 : System.Data.Common.dasm (0.02% of base)
         106 : System.Management.dasm (0.03% of base)
          92 : System.Diagnostics.EventLog.dasm (0.07% of base)
          21 : System.Threading.RateLimiting.dasm (0.01% of base)
           4 : Newtonsoft.Json.dasm (0.00% of base)

Top file improvements (bytes):
      -30685 : Microsoft.Diagnostics.Tracing.TraceEvent.dasm (-0.84% of base)
       -2287 : FSharp.Core.dasm (-0.06% of base)
        -863 : System.Private.Xml.dasm (-0.02% of base)
        -503 : System.Reflection.DispatchProxy.dasm (-2.10% of base)
        -396 : System.Data.OleDb.dasm (-0.12% of base)
        -303 : Microsoft.CodeAnalysis.dasm (-0.02% of base)
        -282 : System.Formats.Asn1.dasm (-0.29% of base)
        -172 : Microsoft.Extensions.DependencyInjection.dasm (-0.27% of base)
        -115 : System.Private.CoreLib.dasm (-0.01% of base)
        -100 : Microsoft.CodeAnalysis.VisualBasic.dasm (-0.00% of base)
         -56 : System.ComponentModel.TypeConverter.dasm (-0.02% of base)
         -44 : System.Runtime.Caching.dasm (-0.07% of base)
         -41 : System.Runtime.Numerics.dasm (-0.03% of base)
         -38 : System.Transactions.Local.dasm (-0.02% of base)
         -36 : OSExtensions.dasm (-0.35% of base)
         -32 : System.Drawing.Common.dasm (-0.01% of base)
         -28 : System.Private.DataContractSerialization.dasm (-0.00% of base)
         -28 : Microsoft.Diagnostics.FastSerialization.dasm (-0.01% of base)
         -26 : System.DirectoryServices.AccountManagement.dasm (-0.01% of base)
         -24 : System.Net.Http.dasm (-0.00% of base)
         -24 : System.Text.RegularExpressions.dasm (-0.00% of base)
         -22 : Microsoft.Extensions.FileProviders.Physical.dasm (-0.13% of base)
         -20 : System.Diagnostics.PerformanceCounter.dasm (-0.02% of base)
         -20 : System.Linq.Expressions.dasm (-0.00% of base)
         -17 : System.Net.Http.WinHttpHandler.dasm (-0.01% of base)
         -15 : System.Net.HttpListener.dasm (-0.01% of base)
         -14 : System.Reflection.Metadata.dasm (-0.00% of base)
         -12 : System.Console.dasm (-0.02% of base)
          -8 : Microsoft.CodeAnalysis.CSharp.dasm (-0.00% of base)
          -8 : System.Text.Json.dasm (-0.00% of base)

41 total files with Code Size differences (36 improved, 5 regressed), 233 unchanged

4966 total methods with Code Size differences (4920 improved, 46 regressed), 356408 unchanged

TODO: Avoid allocating the pinned handle for the boxed structs.

@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 Nov 1, 2022
@ghost ghost assigned EgorBo Nov 1, 2022
@ghost
Copy link

ghost commented Nov 1, 2022

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

Issue Details

Contributes to #76151

All static fields of value types (non-primitive) are usually boxed and stored in the normal heap. This PR moves them to the Frozen Object Heap, it allows us to avoid an indirect load when we access them (we no longer need a pinned handle to the "movable" object).

Example:

static DateTime Date { get; set; }

Codegen for the getter:

; Assembly listing for method Proga:get_Date():System.DateTime
; Tier-1 compilation
-      48B8C01E00BDFC010000 mov      rax, 0x1FCBD001EC0      ; box for Proga:<Date>k__BackingField
+      48B8E81C94C107020000 mov      rax, 0x207C1941CE8      ; 'System.DateTime'
-      488B00               mov      rax, gword ptr [rax]
       488B4008             mov      rax, qword ptr [rax+08H]
       C3                   ret      
-; Total bytes of code 18
+; Total bytes of code 15

Diffs for Libraries (jit-utils -f -pmi):

Total bytes of delta: -24086 (-0.04 % of base)
Total relative delta: -162.40
    diff is an improvement.
    relative diff is an improvement.


Total byte diff includes -206 bytes from reconciling methods
        Base had    2 unique methods,      206 unique bytes
        Diff had    0 unique methods,        0 unique bytes

Top file regressions (bytes):
          49 : System.Threading.RateLimiting.dasm (0.03% of base)
          20 : Newtonsoft.Json.dasm (0.00% of base)

Top file improvements (bytes):
      -21844 : Microsoft.Diagnostics.Tracing.TraceEvent.dasm (-0.60% of base)
        -510 : System.Private.Xml.dasm (-0.01% of base)
        -393 : System.Reflection.DispatchProxy.dasm (-1.64% of base)
        -244 : System.Data.Common.dasm (-0.01% of base)
        -233 : Microsoft.CodeAnalysis.dasm (-0.01% of base)
        -198 : System.Data.OleDb.dasm (-0.06% of base)
        -141 : System.Formats.Asn1.dasm (-0.15% of base)
        -117 : Microsoft.Extensions.DependencyInjection.dasm (-0.19% of base)
         -83 : System.Private.CoreLib.dasm (-0.00% of base)
         -42 : System.ComponentModel.TypeConverter.dasm (-0.01% of base)
         -32 : System.Console.dasm (-0.06% of base)
         -27 : Microsoft.CodeAnalysis.VisualBasic.dasm (-0.00% of base)
         -27 : OSExtensions.dasm (-0.26% of base)
         -21 : Microsoft.Diagnostics.FastSerialization.dasm (-0.01% of base)
         -18 : System.Runtime.Numerics.dasm (-0.01% of base)
         -18 : System.Speech.dasm (-0.00% of base)
         -18 : System.DirectoryServices.AccountManagement.dasm (-0.00% of base)
         -18 : System.Diagnostics.PerformanceCounter.dasm (-0.02% of base)
         -15 : Microsoft.Extensions.FileProviders.Physical.dasm (-0.09% of base)
         -15 : System.Text.RegularExpressions.dasm (-0.00% of base)

4887 total methods with Code Size differences (4870 improved, 17 regressed), 387406 unchanged

TODO: Avoid allocating the pinned handle for the boxed structs.

Author: EgorBo
Assignees: -
Labels:

area-CodeGen-coreclr

Milestone: -

src/coreclr/inc/corinfo.h Outdated Show resolved Hide resolved
@EgorBo EgorBo force-pushed the boxed-statics-foh branch from 86a8833 to 77a42b7 Compare November 1, 2022 19:13
@EgorBo EgorBo mentioned this pull request Nov 2, 2022
14 tasks
@EgorBo
Copy link
Member Author

EgorBo commented Nov 2, 2022

@jkotas do we want to avoid allocations of handles here and use that & 1 logic?

Looks like even large apps don't use statics that a lot to care about? the bingsnr ends up emitting 16kb of statics on FOH with this PR only. So it's only about a slightly better codegen.

@jkotas
Copy link
Member

jkotas commented Nov 2, 2022

What is the handle you have in mind? I do not think there is any handle to save, without completely redoing how statics are allocated.

@EgorBo
Copy link
Member Author

EgorBo commented Nov 2, 2022

What is the handle you have in mind? I do not think there is any handle to save, without completely redoing how statics are allocated.

Alright, so then it's ok as is?

@EgorBo EgorBo marked this pull request as ready for review November 2, 2022 17:51
@MichalStrehovsky
Copy link
Member

@MichalStrehovsky do you plan to resurrect your PR to improve statics in NativeAOT (as far as I understand even statics of primitive types currently require the helper call)? if not I can take a look 🙂 as we've got rid of getFieldAddress api completely

I was only looking at it because nobody else was and it bothers me :). You'll likely be able to come up with a much better fix and much faster than me. Thank you!

@EgorBo
Copy link
Member Author

EgorBo commented Nov 10, 2022

Had to fix an outerloop test that expected statics to change address after GC.Collect()

@EgorBo
Copy link
Member Author

EgorBo commented Nov 10, 2022

/azp run runtime-coreclr pgo, runtime-coreclr libraries-pgo

@azure-pipelines
Copy link

Azure Pipelines successfully started running 2 pipeline(s).

@EgorBo
Copy link
Member Author

EgorBo commented Nov 13, 2022

note: I'm investigating a flaky NullReferenceException happening in System.Text.RegularExpression tests

      System.Text.RegularExpressions.Tests.AttRegexTests.Test(engine: SourceGenerated, options: None, pattern: "abc", input: "abc", expected: "(0,3)", skipNonBacktracking: False) [FAIL]
        System.TypeInitializationException : The type initializer for 'System.Text.RegularExpressions.Generated.<RegexGenerator_g>F1__Get0_0' threw an exception.
        ---- System.TypeInitializationException : The type initializer for 'System.Text.RegularExpressions.Generated.<RegexGenerator_g>F1__Utilities' threw an exception.
        -------- System.NullReferenceException : Object reference not set to an instance of an object.
        Stack Trace:
          System.Text.RegularExpressions.Generator\System.Text.RegularExpressions.Generator.RegexGenerator\RegexGenerator.g.cs(16,0): at C.Get0()
             at System.RuntimeMethodHandle.InvokeMethod(Object target, Void** arguments, Signature sig, Boolean isConstructor)

@EgorBo
Copy link
Member Author

EgorBo commented Nov 13, 2022

Oops, bad rebase. I think I've found the reason, it was an incorrect fix to handle EqualityComparer.Default field (to remove it under GTF_CALL_M_HELPER_SPECIAL_DCE flag), fixed via 3db4522 it used to be able to drop any IR tree with IND(anything).
the rest of changes are untouched

@EgorBo
Copy link
Member Author

EgorBo commented Nov 13, 2022

llvmfullaot pipeline fails because of #78290

@EgorBo EgorBo merged commit cabb8b0 into dotnet:main Nov 13, 2022
@EgorBo EgorBo deleted the boxed-statics-foh branch November 13, 2022 20:48
EgorBo added a commit to EgorBo/runtime-1 that referenced this pull request Nov 14, 2022
jkotas pushed a commit that referenced this pull request Nov 14, 2022
@EgorBo EgorBo mentioned this pull request Dec 2, 2022
@ghost ghost locked as resolved and limited conversation to collaborators Dec 14, 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.

5 participants