Skip to content

Conversation

@AustinWise
Copy link
Contributor

@AustinWise AustinWise commented Dec 6, 2025

Before this change, the addition of a new method on the JIT<->EE interface resulted in large diffs in CorInfoImpl_generated.cs when viewed on GitHub.com. For example the changes from #120411 look like this:

callbacks[154] = (delegate* unmanaged<IntPtr, IntPtr*, CORINFO_RESOLVED_TOKEN*, CORINFO_SIG_INFO*, CORINFO_GET_TAILCALL_HELPERS_FLAGS, CORINFO_TAILCALL_HELPERS*, byte>)&_getTailCallHelpers;
-callbacks[155] = (delegate* unmanaged<IntPtr, IntPtr*, CORINFO_METHOD_STRUCT_*>)&_getAsyncResumptionStub;
-callbacks[156] = (delegate* unmanaged<IntPtr, IntPtr*, CORINFO_RESOLVED_TOKEN*, byte, byte>)&_convertPInvokeCalliToCall;
-callbacks[157] = (delegate* unmanaged<IntPtr, IntPtr*, InstructionSet, byte, byte>)&_notifyInstructionSetUsage;
+callbacks[155] = (delegate* unmanaged<IntPtr, IntPtr*, nuint, bool*, nuint, CORINFO_CLASS_STRUCT_*>)&_getContinuationType;
+callbacks[156] = (delegate* unmanaged<IntPtr, IntPtr*, CORINFO_METHOD_STRUCT_*>)&_getAsyncResumptionStub;
+callbacks[157] = (delegate* unmanaged<IntPtr, IntPtr*, CORINFO_RESOLVED_TOKEN*, byte, byte>)&_convertPInvokeCalliToCall;
+callbacks[158] = (delegate* unmanaged<IntPtr, IntPtr*, InstructionSet, byte, byte>)&_notifyInstructionSetUsage;
// ...

After this change, the diffs are more readable:

 s_vtbl.GetTailCallHelpers = &_getTailCallHelpers;
+s_vtbl.GetContinuationType = &_getContinuationType;
 s_vtbl.GetAsyncResumptionStub = &_getAsyncResumptionStub;
 s_vtbl.NotifyInstructionSetUsage = &_notifyInstructionSetUsage;
// ...

Because the new pattern is pre-initializable and does not allocate native memory for each CorInfoImpl, it should be a little faster and use less a little less memory. I doubt that makes an observable difference though since CorInfoImpl instances are reused.

@github-actions github-actions bot added the needs-area-label An area label is needed to ensure this gets routed to the appropriate area owners label Dec 6, 2025
@dotnet-policy-service dotnet-policy-service bot added the community-contribution Indicates that the PR has been added by a community member label Dec 6, 2025
AustinWise added a commit to AustinWise/attachments that referenced this pull request Dec 6, 2025
@AustinWise AustinWise force-pushed the austin/ThunkGenerator branch from 45d157d to c748587 Compare December 6, 2025 20:57
@AustinWise AustinWise marked this pull request as ready for review December 6, 2025 21:48
@jkotas jkotas added area-NativeAOT-coreclr and removed needs-area-label An area label is needed to ensure this gets routed to the appropriate area owners labels Dec 6, 2025
Copy link
Member

@EgorBo EgorBo left a comment

Choose a reason for hiding this comment

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

Heh, I actually wanted to do the same once, thanks! 🙂

@jkotas
Copy link
Member

jkotas commented Dec 7, 2025

Should we rather change this to the canonical pre-initialized vtable pattern

[FixedAddressValueType]
public static readonly IInspectableVftbl Vtbl;
? It is nicely diffable as well.

@AustinWise
Copy link
Contributor Author

Should we rather change this to the canonical pre-initialized vtable pattern

[FixedAddressValueType]
public static readonly IInspectableVftbl Vtbl;

? It is nicely diffable as well.

Oh, that looks nicer. I'll mark this as draft while I rework this PR to look more like that.

@AustinWise AustinWise marked this pull request as draft December 7, 2025 03:49
@AustinWise AustinWise force-pushed the austin/ThunkGenerator branch from c748587 to d4b11ec Compare December 7, 2025 17:42
@jkotas
Copy link
Member

jkotas commented Dec 7, 2025

void** callbacks = (void**)NativeMemory.Alloc((nuint)(sizeof(void*) * numCallbacks));
callbacks[0] = (delegate* unmanaged<IntPtr, byte*, int, int>)&getIntConfigValue;
callbacks[1] = (delegate* unmanaged<IntPtr, byte*, byte*, int, int>)&getStringConfigValue;
can use the same treatment (it can be a separate PR).

@AustinWise AustinWise marked this pull request as ready for review December 7, 2025 19:18
@AustinWise
Copy link
Contributor Author

I removed the extra new lines.

@AustinWise
Copy link
Contributor Author

I applied the same pattern to JitConfigProvider. I think that addresses all feedback so far.

{
internal unsafe partial class CorInfoImpl
{
private struct ICorJitInfoVtbl
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
private struct ICorJitInfoVtbl
private struct ICorJitInfoCallbacks

One last nit: This is not actually a ICorJitInfo vtable. The actual ICorJitInfo vtable is implemented in C++ by wrapping the callbacks from this array with exception handling goo. Maybe this should be called ICorJitInfoCallbacks, UnmanagedCallbacks (matches GetUnmanagedCallbacks) to avoid confusion?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

area-NativeAOT-coreclr community-contribution Indicates that the PR has been added by a community member

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants