Remove redundant pinning over static fields/RVA #119674

EgorBo · 2025-09-12T23:10:23Z

I noticed that sometimes developers make assumptions that it's fine to use Unsafe.AsPointer over RVA fields since they're not movable, which is fairly unreliable thing to do (e.g. Unloadable ALCs). So let's just optimize the pinning overhead in JIT when we know it's safe to do so:

// Case 0: RVA fields
void Test0()
{
    ReadOnlySpan<int> rva = [1,2,3,4,5];
    fixed (int* p = rva)
        Consume(p);
}

// Case 1: Primitive static fields (unmanaged memory)
static int _primitiveFld;
void Test1()
{
    fixed (int* p = &_primitiveFld)
        Consume(p);
}


// Case 2: Struct static fields (boxed and stored on the NonGC heap)
static MyStruct _structFld;
void Test2()
{
    fixed (int* p = &_structFld.Y)
        Consume(p);
}

Codegen diff: https://www.diffchecker.com/LgIALnAn/

dotnet-policy-service · 2025-09-12T23:11:17Z

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

EgorBo · 2025-09-13T15:30:41Z

@MihuBot

jkotas · 2025-09-14T00:41:21Z

src/coreclr/tools/Common/JitInterface/CorInfoImpl.cs

+        private bool canOmitPinning(CORINFO_FIELD_STRUCT_* fldHnd)
+        {
+            FieldDesc field = HandleToObject(fldHnd);
+            if (!field.IsStatic || field.IsThreadStatic || field.HasGCStaticBase || field.OwningType.IsCanonicalSubtype(CanonicalFormKind.Any))


HasGCStaticBase condition should not be needed for NativeAOT.

See dotnet/runtimelab#870

I see, thanks. Removed

HasGCStaticBase condition is needed for R2R to be functionally correct. Are we seeing any crashes in GCStress+R2R with this change? If not, we may consider adding some targeted tests to validate that this optimization is valid.

It is an interesting question how conservative the contract should be for R2R. I guess we can start with the condition you had here originally. It will be one more reason why R2R is incompatible and ignored for collectible ALCs.

EgorBo · 2025-09-30T22:27:36Z

@jkotas could you please review the VM side? I guess I have a few conservative checks like ThreadStatics, but I was mostly focusing on RVA fields and structs (boxed statics)
@dotnet/jit-contrib PTAL jit side

Copilot

Pull Request Overview

This PR optimizes pinning operations in the JIT compiler by introducing a new canOmitPinning API that allows the JIT to skip pinning overhead when working with static fields that are guaranteed to be stable in memory. The optimization applies to RVA fields, primitive static fields in unmanaged memory, and struct static fields stored on the NonGC heap.

Key changes:

Introduces canOmitPinning JIT interface method to determine when pinning can be safely omitted
Enhances IsNotGcDef to consider field sequences and call the new API for static fields
Updates SuperPMI infrastructure to support the new interface method

Reviewed Changes

Copilot reviewed 20 out of 20 changed files in this pull request and generated no comments.

Show a summary per file

File	Description
src/coreclr/vm/jitinterface.cpp	Implements `canOmitPinning` with logic to check if static fields are safe to use without pinning
src/coreclr/jit/gentree.cpp	Refactors `IsNotGcDef` to accept compiler parameter and check static field sequences
src/coreclr/jit/gentree.h	Changes `IsNotGcDef` signature to require compiler parameter
src/coreclr/jit/lclvars.cpp	Updates call to `IsNotGcDef` with compiler parameter
src/coreclr/inc/corinfo.h	Adds `canOmitPinning` method declaration to JIT interface
src/coreclr/inc/jiteeversionguid.h	Updates JIT-EE version identifier for interface change
src/coreclr/tools/Common/JitInterface/CorInfoImpl.cs	Implements simplified AOT version of `canOmitPinning`
src/coreclr/tools/Common/JitInterface/CorInfoImpl_generated.cs	Auto-generated code for new interface method
Various SuperPMI files	Adds infrastructure support for recording and replaying `canOmitPinning` calls

EgorBo · 2025-09-30T22:29:23Z

src/coreclr/jit/gentree.cpp

+    }
+
+    return false;
+}


This function was moved from gentree.h and canOmitPinning is added.
Also I added gtPeelOffsets check

jkotas · 2025-10-01T04:10:11Z

src/coreclr/vm/jitinterface.cpp

+        TypeHandle fieldOwnerType(field->GetEnclosingMethodTable());
+        if (!fieldOwnerType.GetLoaderAllocator()->CanUnload() && !fieldOwnerType.IsCanonicalSubtype())
+        {
+            if (field->IsRVA())


Suggested change

if (field->IsRVA())

if (field->IsRVA() || !field->IsByValue())

This can mirror the logic from CEEInfo::getFieldInfo

jkotas · 2025-10-01T04:33:03Z

src/coreclr/vm/jitinterface.cpp

+            else
+            {
+                GCX_COOP();
+                void* pFieldAddr = field->GetCurrentStaticAddress();


Suggested change

void* pFieldAddr = field->GetCurrentStaticAddress();

void* pAddr = field->GetStaticAddressHandle((void*)field->GetBase());

Object* frozenObj = VolatileLoad((Object**)pAddr);

// ContainsGCPointers here is unnecessary but it's cheaper than IsInFrozenSegment

// for structs containing gc handles

if (!frozenObj->GetMethodTable()->ContainsGCPointers() &&

GCHeapUtilities::GetGCHeap()->IsInFrozenSegment(frozenObj))

{

result = true;

}

It is questionable to cast an interior pointer to Object* and pass it to the GC. It may happen to work currently but it does not sound like a good idea.

Instead, we should mirror the logic from getFieldInfo that does not have this problem,

jkotas · 2025-10-01T04:48:46Z

src/coreclr/tools/Common/JitInterface/CorInfoImpl.cs

+        private bool canOmitPinning(CORINFO_FIELD_STRUCT_* fldHnd)
+        {
+            FieldDesc field = HandleToObject(fldHnd);
+            if (!field.IsStatic || field.IsThreadStatic || field.HasGCStaticBase || field.OwningType.IsCanonicalSubtype(CanonicalFormKind.Any))


HasGCStaticBase condition is needed for R2R to be functionally correct. Are we seeing any crashes in GCStress+R2R with this change? If not, we may consider adding some targeted tests to validate that this optimization is valid.

It is an interesting question how conservative the contract should be for R2R. I guess we can start with the condition you had here originally. It will be one more reason why R2R is incompatible and ignored for collectible ALCs.

Remove redundant pinning over static fields/RVA

1555ada

github-actions bot added the area-CodeGen-coreclr CLR JIT compiler in src/coreclr/src/jit and related components such as SuperPMI label Sep 12, 2025

dotnet-policy-service bot assigned EgorBo Sep 12, 2025

less conservative

59e75d3

EgorBo added the reduce-unsafe label Sep 12, 2025

fix build

cc4a8f5

build-analysis bot mentioned this pull request Sep 13, 2025

Intermittent build failure in AfterSourceBuild: "Could not write state file" #76488

Open

MihuBot mentioned this pull request Sep 13, 2025

[JitDiff X64] [EgorBo] Remove redundant pinning over static fields/RVA MihuBot/runtime-utils#1484

Open

EgorBo added 2 commits September 13, 2025 17:02

Implement R2R/NAOT

1297bc2

formatting

a175448

MihuBot mentioned this pull request Sep 13, 2025

[JitDiff X64] [EgorBo] Remove redundant pinning over static fields/RVA MihuBot/runtime-utils#1485

Open

fix diffs

1630f0c

build-analysis bot mentioned this pull request Sep 13, 2025

[android] Android.Device_Emulator.JIT.Test failing on emulators with CoreCLR #112633

Open

jkotas reviewed Sep 14, 2025

View reviewed changes

EgorBo added 2 commits October 1, 2025 00:26

Update CorInfoImpl.cs

8c9e82d

Merge branch 'main' into avoid-pinning-rva

3caad0b

EgorBo marked this pull request as ready for review September 30, 2025 22:27

EgorBo requested a review from MichalStrehovsky as a code owner September 30, 2025 22:27

Copilot AI review requested due to automatic review settings September 30, 2025 22:27

Copilot AI reviewed Sep 30, 2025

View reviewed changes

EgorBo requested a review from jkotas September 30, 2025 22:27

EgorBo commented Sep 30, 2025

View reviewed changes

jkotas reviewed Oct 1, 2025

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Remove redundant pinning over static fields/RVA #119674

Remove redundant pinning over static fields/RVA #119674

Uh oh!

EgorBo commented Sep 12, 2025 •

edited

Loading

Uh oh!

dotnet-policy-service bot commented Sep 12, 2025

Uh oh!

EgorBo commented Sep 13, 2025

Uh oh!

jkotas Sep 14, 2025

Uh oh!

EgorBo Sep 30, 2025

Uh oh!

jkotas Oct 1, 2025

Uh oh!

EgorBo commented Sep 30, 2025 •

edited

Loading

Uh oh!

Copilot AI left a comment

Uh oh!

EgorBo Sep 30, 2025

Uh oh!

jkotas Oct 1, 2025

Uh oh!

jkotas Oct 1, 2025

Uh oh!

jkotas Oct 1, 2025

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

	if (field->IsRVA())
	if (field->IsRVA() \|\| !field->IsByValue())

-                void* pFieldAddr = field->GetCurrentStaticAddress();
+                void* pAddr = field->GetStaticAddressHandle((void*)field->GetBase());
+                Object* frozenObj = VolatileLoad((Object**)pAddr);
+                // ContainsGCPointers here is unnecessary but it's cheaper than IsInFrozenSegment
+                // for structs containing gc handles
+                if (!frozenObj->GetMethodTable()->ContainsGCPointers() &&
+                    GCHeapUtilities::GetGCHeap()->IsInFrozenSegment(frozenObj))
+                {
+                    result = true;
+                }

Remove redundant pinning over static fields/RVA #119674

Are you sure you want to change the base?

Remove redundant pinning over static fields/RVA #119674

Uh oh!

Conversation

EgorBo commented Sep 12, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

dotnet-policy-service bot commented Sep 12, 2025

Uh oh!

EgorBo commented Sep 13, 2025

Uh oh!

jkotas Sep 14, 2025

Choose a reason for hiding this comment

Uh oh!

EgorBo Sep 30, 2025

Choose a reason for hiding this comment

Uh oh!

jkotas Oct 1, 2025

Choose a reason for hiding this comment

Uh oh!

EgorBo commented Sep 30, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull Request Overview

Reviewed Changes

Uh oh!

EgorBo Sep 30, 2025

Choose a reason for hiding this comment

Uh oh!

jkotas Oct 1, 2025

Choose a reason for hiding this comment

Uh oh!

jkotas Oct 1, 2025

Choose a reason for hiding this comment

Uh oh!

jkotas Oct 1, 2025

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

EgorBo commented Sep 12, 2025 •

edited

Loading

EgorBo commented Sep 30, 2025 •

edited

Loading