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 Array.Empty<> on a frozen segment (NonGC heap) #85559

Merged
merged 23 commits into from
May 3, 2023

Conversation

EgorBo
Copy link
Member

@EgorBo EgorBo commented Apr 29, 2023

Contributes to #76151 (implements @jkotas's suggestion to put empty arrays on FOH). Initial infrastructure to emit "frozen" allocators instead of normal ones for newobj, newarr in static constructors. Basically, we want static readonly <gcref> fields to be allocated on frozen segments since they're immortal. It gives us:

  1. Less immortal objects in normal heaps
  2. JIT can bake direct object addresses in codegen
  3. JIT can perform more optimizations on them e.g. read their fields in case of immutable objects, fold comparison, etcs.

Codegen improvement example:

string[] Test()
{
    return Array.Empty<string>();
}
; Assembly listing for method MyClass:Test():string[]:this
; Tier-1 compilation
       mov      rax, 0x25D00309C68      ; 'System.String[]'
       ret      
; Total bytes of code 11

Also, works for user-defined arrays:

static byte[] Array { get; } = new byte[1024];

public byte[] Test()
{
    return Array;
}
; Assembly listing for method MyClass:Test():ubyte[]:this
       mov      rax, 0x29480209C80      ; 'System.Byte[]'
       ret      
; Total bytes of code 11

@EgorBo
Copy link
Member Author

EgorBo commented Apr 29, 2023

Although, I assume I'd better land a workaound to detect EmptyArray's cctor shape to emit a frozen allocator in JIT. Will see if I can do it easily

@EgorBo
Copy link
Member Author

EgorBo commented Apr 30, 2023

@jkotas @MichalStrehovsky I'm a bit lost here - can you please point me where R2R helper can be mapped to a VM function? I basically want CORINFO_HELP_READYTORUN_NEWARR_1_FROZEN to call my jithelpers.cpp's JIT_NewArr1Frozen

@EgorBo
Copy link
Member Author

EgorBo commented Apr 30, 2023

Ah, I think I've figured that out, just not sure if I bumped R2R version correctly (maybe a minor version change is enough?)

@jkotas
Copy link
Member

jkotas commented Apr 30, 2023

Right, this does not need MINIMUM_READYTORUN_MAJOR_VERSION bump. It adds new helpers; it does not break anything existing.

@EgorBo EgorBo marked this pull request as ready for review April 30, 2023 16:07
@EgorBo EgorBo mentioned this pull request Apr 30, 2023
14 tasks
@EgorBo
Copy link
Member Author

EgorBo commented Apr 30, 2023

/azp run runtime-coreclr outerloop

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@EgorBo
Copy link
Member Author

EgorBo commented May 1, 2023

@jkotas when you have time can you please review VM/CG2 side?

I've added an allocator for non-array too but that is not used currently, I want to work separately on a phase to do a proper analysis and emit them less conservatively for many types of objects and e.g. initialized arrays. Currently, it only works for Array.Empty<> and similar user patterns, actually even

static readonly int[] myArr = new int[10]; // without initialization

works. I've also verified that the allocators work in both R2R and only-JIT modes

@jkotas
Copy link
Member

jkotas commented May 1, 2023

#85559

LGTM otherwise

@EgorBo
Copy link
Member Author

EgorBo commented May 1, 2023

@jakobbotsch @AndyAyersMS @dotnet/jit-contrib Please, sign off the JIT side - it should be simple at this point, recognizes a fixed IL pattern. I'll remove that once I come up with a smarter phase to insert allocators (already have a prototype)

Also, I've removed some AreTypesEquivalent left overs in JIT

@EgorBo EgorBo requested review from jakobbotsch and AndyAyersMS May 1, 2023 21:47

Verified

This commit was created on GitHub.com and signed with GitHub’s verified signature. The key has expired.
Co-authored-by: Jakob Botsch Nielsen <Jakob.botsch.nielsen@gmail.com>
Copy link
Member

@jakobbotsch jakobbotsch left a comment

Choose a reason for hiding this comment

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

JIT changes LGTM assuming we can generalize it more in the future.

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.

I realize it's tempting to aggregate a bunch of jit interface changes to minimize some of the churn, but I'd really like to see PRs like this split into two -- one for things that should be no diff and another for things that that cause diffs.

@EgorBo
Copy link
Member Author

EgorBo commented May 3, 2023

I realize it's tempting to aggregate a bunch of jit interface changes to minimize some of the churn, but I'd really like to see PRs like this split into two -- one for things that should be no diff and another for things that that cause diffs.

This is exactly what I do in this PR - I add new helpers and plan to actively use them in a separate PR. In this PR I only added Array.Empty but that didn't cause a lot of diffs (although, caused new inlining events because Foo(Array<int>.Empty,...) was recognized as a constant arg at the call-site (due to frozen object handle).

@EgorBo EgorBo merged commit 0be256e into dotnet:main May 3, 2023
@EgorBo EgorBo deleted the alloc-frozen-array branch May 3, 2023 09:41
@EgorBo
Copy link
Member Author

EgorBo commented May 3, 2023

Failure is known - #85637, #85659

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants