Skip to content

Make the output of ThunkGenerator more diffable#122266

Merged
jkotas merged 9 commits intodotnet:mainfrom
AustinWise:austin/ThunkGenerator
Dec 9, 2025
Merged

Make the output of ThunkGenerator more diffable#122266
jkotas merged 9 commits intodotnet:mainfrom
AustinWise:austin/ThunkGenerator

Conversation

@AustinWise
Copy link
Contributor

@AustinWise AustinWise commented Dec 6, 2025 •
edited
Loading

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
Add for dotnet/runtime#122266
1543963
@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
EgorBo
EgorBo approved these changes Dec 7, 2025
View reviewed changes
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! 🙂

EgorBo
EgorBo reviewed Dec 7, 2025
View reviewed changes
@jkotas
Copy link
Member

jkotas commented Dec 7, 2025

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

runtime/src/tests/nativeaot/SmokeTests/Preinitialization/Preinitialization.cs

Lines 1761 to 1762 in 86dfeb9

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

@AustinWise
Copy link
Contributor Author

AustinWise commented Dec 7, 2025

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

runtime/src/tests/nativeaot/SmokeTests/Preinitialization/Preinitialization.cs

Lines 1761 to 1762 in 86dfeb9

[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

runtime/src/coreclr/tools/Common/JitInterface/JitConfigProvider.cs

Lines 168 to 171 in 74cf618

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).

jkotas
jkotas approved these changes Dec 7, 2025
View reviewed changes
@AustinWise AustinWise marked this pull request as ready for review December 7, 2025 19:18
@AustinWise
Copy link
Contributor Author

AustinWise commented Dec 7, 2025

I removed the extra new lines.

@AustinWise
Copy link
Contributor Author

AustinWise commented Dec 7, 2025

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

MichalPetryka
MichalPetryka reviewed Dec 7, 2025
View reviewed changes
jkotas
jkotas reviewed Dec 8, 2025
View reviewed changes
jkotas
jkotas reviewed Dec 8, 2025
View reviewed changes
jkotas
jkotas reviewed Dec 8, 2025
View reviewed changes
jkotas
jkotas reviewed Dec 8, 2025
View reviewed changes
jkotas
jkotas approved these changes Dec 9, 2025
View reviewed changes
Copy link
Member

@jkotas jkotas left a comment

Choose a reason for hiding this comment

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

Thank you!

@jkotas
Copy link
Member

jkotas commented Dec 9, 2025

/ba-g DeadLetter

@jkotas jkotas merged commit 007bd9d into dotnet:main Dec 9, 2025
93 of 96 checks passed
This was referenced Dec 9, 2025
Open
Open
Open
@dotnet-maestro dotnet-maestro bot mentioned this pull request Dec 9, 2025
Closed
@AustinWise AustinWise deleted the austin/ThunkGenerator branch December 10, 2025 03:15
@maestro-local-dev maestro-local-dev bot mentioned this pull request Dec 10, 2025
Closed
@dotnet-maestro dotnet-maestro bot mentioned this pull request Dec 13, 2025
Merged
@github-actions github-actions bot locked and limited conversation to collaborators Jan 9, 2026
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Reviewers

@EgorBo EgorBo EgorBo approved these changes

@jkotas jkotas jkotas approved these changes

@MichalStrehovsky MichalStrehovsky Awaiting requested review from MichalStrehovsky MichalStrehovsky is a code owner

+1 more reviewer

@MichalPetryka MichalPetryka MichalPetryka left review comments

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

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

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

@AustinWise @jkotas @EgorBo @MichalPetryka