-
Notifications
You must be signed in to change notification settings - Fork 5.3k
Remove MethodDescCallSite in appdomain.cpp
#123967
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
base: main
Are you sure you want to change the base?
Remove MethodDescCallSite in appdomain.cpp
#123967
Conversation
Convert 7 VM-to-managed calls in appdomain.cpp from MethodDescCallSite to the more efficient UnmanagedCallersOnlyCaller pattern: - OnAssemblyLoad - OnTypeResolve - OnResourceResolve - OnAssemblyResolve - Resolve - ResolveSatelliteAssembly - ResolveUsingResolvingEvent Changes: - Add UCO wrapper methods in AssemblyLoadContext.cs with object* parameters - Add metasig signatures for the new UCO methods - Update corelib.h method definitions to use new signatures - Update appdomain.cpp to use UnmanagedCallersOnlyCaller - Remove obsolete methods from AssemblyLoadContext.CoreCLR.cs - Update TypeNameResolver.CoreCLR.cs to use OnTypeResolveCore Contributes to dotnet#123864
|
Tagging subscribers to this area: @agocke |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull request overview
This pull request converts assembly loading callbacks in appdomain.cpp from the older MethodDescCallSite / CallDescrWorker infrastructure to the more efficient UnmanagedCallersOnly pattern, contributing to the broader effort tracked in issue #123864. This change improves VM-to-managed call performance by using the reverse P/Invoke infrastructure.
Changes:
- Added seven
[UnmanagedCallersOnly]wrapper methods in AssemblyLoadContext.cs for assembly loading callbacks - Updated native code to use
UnmanagedCallersOnlyCallerinstead ofMethodDescCallSitefor invoking managed methods - Updated method signatures in corelib.h and added new metadata signatures in metasig.h to support the new calling convention
Reviewed changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| src/libraries/System.Private.CoreLib/src/System/Runtime/Loader/AssemblyLoadContext.cs | Added seven UnmanagedCallersOnly wrapper methods (OnAssemblyLoad, OnTypeResolve, OnResourceResolve, OnAssemblyResolve, Resolve, ResolveSatelliteAssembly, ResolveUsingEvent) with exception handling via out-parameters |
| src/coreclr/vm/metasig.h | Added three new UCO method signatures for assembly loading callbacks with pointer parameters and exception handling |
| src/coreclr/vm/corelib.h | Updated seven method definitions to reference the new UCO signatures and corrected method name (ResolveUsingResolvingEvent → ResolveUsingEvent) |
| src/coreclr/vm/appdomain.cpp | Converted seven call sites from MethodDescCallSite to UnmanagedCallersOnlyCaller, updated argument passing to use pointers, and simplified code by removing ARG_SLOT array construction |
| src/coreclr/System.Private.CoreLib/src/System/Runtime/Loader/AssemblyLoadContext.CoreCLR.cs | Removed two wrapper methods (ResolveSatelliteAssembly, ResolveUsingResolvingEvent) now replaced by UnmanagedCallersOnly wrappers in the main file |
src/libraries/System.Private.CoreLib/src/System/Runtime/Loader/AssemblyLoadContext.cs
Outdated
Show resolved
Hide resolved
|
I assume this will need the wasm callhelpers update too. |
src/libraries/System.Private.CoreLib/src/System/Runtime/Loader/AssemblyLoadContext.cs
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull request overview
Copilot reviewed 6 out of 6 changed files in this pull request and generated 3 comments.
src/libraries/System.Private.CoreLib/src/System/Runtime/Loader/AssemblyLoadContext.cs
Outdated
Show resolved
Hide resolved
src/libraries/System.Private.CoreLib/src/System/Runtime/Loader/AssemblyLoadContext.cs
Outdated
Show resolved
Hide resolved
| { | ||
| try | ||
| { | ||
| *ppResult = OnTypeResolve(*pAssembly, new string(typeName)); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
new string(sbyte*) uses CP_ACP on Windows. I do not think you want that here. This should use Utf8StringMarshaller.ConvertToManaged or something like that.
Also, I would use byte* for the typeName. byte* is typically used for UTF8 throughout the BCL.
src/libraries/System.Private.CoreLib/src/System/Runtime/Loader/AssemblyLoadContext.cs
Outdated
Show resolved
Hide resolved
|
I've got a type load level issue here. Looking for any advice from @jkotas or @davidwrighton. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull request overview
Copilot reviewed 6 out of 6 changed files in this pull request and generated no new comments.
Contributes to #123864