Skip to content

Convert JIT_Box* to C# - #115134

Merged
jkotas merged 2 commits into
dotnet:mainfrom
am11:feature/HMF-removal/JIT_Box
May 2, 2025
Merged

Convert JIT_Box* to C##115134
jkotas merged 2 commits into
dotnet:mainfrom
am11:feature/HMF-removal/JIT_Box

Conversation

@am11

@am11am11 commented Apr 28, 2025

Copy link
Copy Markdown
Member

No description provided.

@ghostghost added the area-VM-coreclr label Apr 28, 2025
@dotnet-policy-servicedotnet-policy-serviceBot added the community-contribution Indicates that the PR has been added by a community member label Apr 28, 2025
@dotnet-policy-service

Copy link
Copy Markdown
Contributor

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

@am11

am11 commented Apr 29, 2025

Copy link
Copy Markdown
MemberAuthor

@EgorBot -amd -arm

usingSystem.Reflection;usingBenchmarkDotNet.Attributes;publicclassBench{publicreadonlyintAReadonlyIntField=42;[Benchmark]publicintReflectionFieldBoxing(){FieldInfofieldInfo=typeof(Bench).GetTypeInfo().GetDeclaredField(nameof(AReadonlyIntField));BenchmyInstance=new();objectcurrent=fieldInfo.GetValue(myInstance);fieldInfo.SetValue(myInstance,int.MinValue);return(int)current+(int)fieldInfo.GetValue(myInstance);}}

@am11

am11 commented Apr 29, 2025

Copy link
Copy Markdown
MemberAuthor

@jkotas, @EgorBo, WDYT? We can get rid of this HMF and later remove the managed helpers when JIT starts inlining boxes (in Tier 0 as it does in Tier 1).

@am11
am11 marked this pull request as ready for review April 29, 2025 08:45
@am11
am11force-pushed the feature/HMF-removal/JIT_Box branch from aaadc26 to bf23c6dCompareApril 29, 2025 11:19
@EgorBo

Copy link
Copy Markdown
Member

@EgorBot -windows_intel -amd -arm

usingSystem.Runtime.CompilerServices;usingBenchmarkDotNet.Attributes;publicclassBench{[Benchmark]publicobjectBox8()=>Box(newBuffer8());[Benchmark]publicobjectBox16()=>Box(newBuffer16());[Benchmark]publicobjectBox32()=>Box(newBuffer32());[Benchmark]publicobjectBox256()=>Box(newBuffer256());[MethodImpl(MethodImplOptions.NoInlining|MethodImplOptions.NoOptimization)]staticobjectBox<T>(Tvalue)=>(object)value;}[InlineArray(1)]publicstructBuffer8{long_element0;}[InlineArray(2)]publicstructBuffer16{long_element0;}[InlineArray(4)]publicstructBuffer32{long_element0;}[InlineArray(32)]publicstructBuffer256{long_element0;}

@am11

am11 commented Apr 29, 2025

Copy link
Copy Markdown
MemberAuthor

@EgorBot -windows_intel -amd -arm

usingSystem.Runtime.CompilerServices;usingBenchmarkDotNet.Attributes;publicclassBench{[Benchmark]publicobjectBox8()=>Box(newBuffer8());[Benchmark]publicobjectBox16()=>Box(newBuffer16());[Benchmark]publicobjectBox32()=>Box(newBuffer32());[Benchmark]publicobjectBox256()=>Box(newBuffer256());[MethodImpl(MethodImplOptions.NoInlining|MethodImplOptions.NoOptimization)]staticobjectBox<T>(Tvalue)=>(object)value;}[InlineArray(1)]publicstructBuffer8{long_element0;}[InlineArray(2)]publicstructBuffer16{long_element0;}[InlineArray(4)]publicstructBuffer32{long_element0;}[InlineArray(32)]publicstructBuffer256{long_element0;}

@am11

am11 commented Apr 29, 2025

Copy link
Copy Markdown
MemberAuthor

arm32 doesn't seem to be happy with the TLV commit despite src/coreclr/vm/arm/stubs.cpp was using JIT_Box_MP_FastPortable. 🤔

@filipnavara

filipnavara commented Apr 29, 2025

Copy link
Copy Markdown
Member

arm32 doesn't seem to be happy with the TLV commit

arm32 had the special alignment code path (RequiresAlign8)

@am11
am11force-pushed the feature/HMF-removal/JIT_Box branch from 10ef59b to 841c89eCompareApril 29, 2025 17:52
Comment threadsrc/coreclr/vm/runtimehandles.cpp Outdated
Comment threadsrc/coreclr/vm/runtimehandles.cpp
@am11
am11force-pushed the feature/HMF-removal/JIT_Box branch from 0b8d8f9 to 0f4cd04CompareApril 29, 2025 18:09
@am11
am11force-pushed the feature/HMF-removal/JIT_Box branch from 0f4cd04 to 7468c56CompareApril 29, 2025 18:49
@am11

am11 commented May 1, 2025

Copy link
Copy Markdown
MemberAuthor

NativeAOT's AllocFast mechanism is much efficient, maybe coreclr can reuse it via asm sharing being introduced in #114982?

Alternatively, JIT can optimize/inline InternalAlloc{NoChecks}. It will not only speed up the existing helpers, but also allow us to complete all remaining HMFs; basically all the remaining ones are due to Alloc GC API call Allocate{Object,String,Array} end up calling GCHeapUtilities::GetGCHeap()->Alloc() and therefore require HELPER_METHOD_FRAME. If we move them to C# today as-is, they will show mixed regressions, just likt JIT_Box which also requrie HMF due to AllocateOjbect call in the fallback path.

@huoyaoyuan

Copy link
Copy Markdown
Member

NativeAOT's AllocFast mechanism is much efficient, maybe coreclr can reuse it via asm sharing being introduced in #114982?

I'm unsure if there is behavioral difference: #105949
But sharing allocation stubs between coreclr and nativeaot should be very appealing.

Comment threadsrc/coreclr/System.Private.CoreLib/src/System/MulticastDelegate.CoreCLR.cs Outdated
Comment threadsrc/coreclr/System.Private.CoreLib/src/System/RuntimeHandles.cs Outdated
Comment threadsrc/coreclr/vm/amd64/JitHelpers_Slow.asm Outdated
Comment threadsrc/coreclr/vm/amd64/JitHelpers_Slow.asm
Comment threadsrc/coreclr/vm/amd64/JitHelpers_Slow.asm Outdated
Comment threadsrc/coreclr/vm/object.h Outdated
Comment threadsrc/coreclr/System.Private.CoreLib/src/System/RuntimeHandles.cs Outdated
Comment threadsrc/coreclr/System.Private.CoreLib/src/System/RuntimeHandles.cs Outdated
Comment threadsrc/coreclr/vm/runtimehandles.cpp
@am11
am11force-pushed the feature/HMF-removal/JIT_Box branch from 100647b to 9dbafa3CompareMay 2, 2025 15:08
Comment threadsrc/coreclr/vm/runtimehandles.cpp
@am11
am11force-pushed the feature/HMF-removal/JIT_Box branch from 9d00363 to 113056fCompareMay 2, 2025 18:51
Comment threadsrc/coreclr/vm/gcheaputilities.h Outdated

@jkotasjkotas left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Thank you!

@jkotas
jkotas merged commit 9e490ef into dotnet:mainMay 2, 2025
@am11
am11 deleted the feature/HMF-removal/JIT_Box branch May 2, 2025 21:19
@am11

am11 commented May 2, 2025

Copy link
Copy Markdown
MemberAuthor

Thanks @jkotas, @EgorBo and @filipnavara for the help!

Do you think we can bring the rest of these to the same plan:

SetJitHelperFunction(CORINFO_HELP_NEWSFAST, JIT_TrialAllocSFastSP);
SetJitHelperFunction(CORINFO_HELP_NEWSFAST_ALIGN8, JIT_TrialAllocSFastSP);
SetJitHelperFunction(CORINFO_HELP_NEWARR_1_VC, JIT_NewArr1VC_UP);
SetJitHelperFunction(CORINFO_HELP_NEWARR_1_OBJ, JIT_NewArr1OBJ_UP);
or would they (some of them?) require different approach?

@filipnavara

Copy link
Copy Markdown
Member

or would they (some of them?) require different approach?

I'd think that they are good candidate for implementing in assembly and sharing with NativeAOT (as per #115134 (comment)). For some of them it looks like a quite straightforward mapping but perhaps I was just lucky to look at the ones that match up closely...

@am11

am11 commented May 2, 2025

Copy link
Copy Markdown
MemberAuthor

Sounds good. Then we can follow @davidwrighton's footsteps once #114982 is completed. :)

@filipnavara

Copy link
Copy Markdown
Member

Then we can follow @davidwrighton's footsteps once #114982 is completed. :)

JFYI I have a prototype on top of #114982 that moves AllocFast.S/asm to the shared code base and replaces most of the helpers. Still needs a bit of love but wanted to let you know to avoid working on the same thing...

@am11

am11 commented May 5, 2025

Copy link
Copy Markdown
MemberAuthor

Thanks! Can't wait to see class HelperMethodFrame cleanup all the way up to LazyMachState::unwindLazyState, which has one of 13 calls to PAL_VirtualUnwind -> unw_step. 😁

Sign up for freeto subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

area-VM-coreclrcommunity-contributionIndicates that the PR has been added by a community member

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants

@am11@EgorBo@filipnavara@huoyaoyuan@jkotas