Remove all current uses of PInvokeArgIterator and the flow of native stack arg sizes through our data structures where possible #123016
Conversation
… believe that we would use it to calculate a value we never actually used. This PR changes the implementation of the runtime to remove those uses which were tied to contruction of function pointers from delegates where the delegate parameters were a structure which required marshalling. Should be a small code size win for Unix X64 platforms along with a negligible perf boost. This work is not removing the PInvokeArgIterator itself, as it appears to be necessary for the interpreter call stub generator (although it also needs to be fixed to be correct in those cases as well.)
|
Tagging subscribers to this area: @mangod9 |
There was a problem hiding this comment.
Pull request overview
This pull request removes unused PInvokeArgIterator calculations for native stack argument sizes, focusing on simplifying code for platforms where this information is not needed. The change introduces a new feature macro FEATURE_DYNAMIC_METHOD_HAS_NATIVE_STACK_ARG_SIZE defined only for TARGET_X86 and FEATURE_COMINTEROP scenarios, where native stack size tracking is still required. This should result in a small code size win and negligible performance improvement on Unix X64 platforms.
Key changes:
- Introduces
FEATURE_DYNAMIC_METHOD_HAS_NATIVE_STACK_ARG_SIZEmacro to conditionally compile native stack arg size tracking - Guards
SizeOfNativeArgStack()and related methods with the new macro - Removes empty stub methods for non-TARGET_X86 CLRToCOMCallInfo and CLRToCOMCallMethodDesc that were previously required
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/coreclr/vm/method.hpp | Adds FEATURE_DYNAMIC_METHOD_HAS_NATIVE_STACK_ARG_SIZE macro; conditionally compiles native stack arg size methods in DynamicMethodDesc and PInvokeMethodDesc; removes empty stub methods from CLRToCOMCallInfo and CLRToCOMCallMethodDesc |
| src/coreclr/vm/method.cpp | Guards SizeOfNativeArgStack() implementation with FEATURE_DYNAMIC_METHOD_HAS_NATIVE_STACK_ARG_SIZE |
| src/coreclr/vm/dllimport.cpp | Guards SetNativeStackArgSize calls and stack size calculations with appropriate macros |
| src/coreclr/vm/comdelegate.cpp | Guards InitStackArgumentSize call with TARGET_X86 |
| src/coreclr/vm/clrtocomcall.cpp | Guards InitStackArgumentSize call with TARGET_X86 |
janvorli
left a comment
janvorli
left a comment
There was a problem hiding this comment.
LGTM modulo the copilot nit on indentation.
3 participants
A quick analysis of the current use of PInvokeArgIterator leads me to believe that we would use it to calculate a value we never actually used. This PR changes the implementation of the runtime to remove those uses which were tied to construction of function pointers from delegates where the delegate parameters were a structure which required marshalling. Should be a small code size win for Unix X64 platforms along with a negligible perf boost on all non-windows platforms. We still need this thing for COM scenarios on Windows so I haven't touched it there.
This work is not removing the PInvokeArgIterator itself, as it appears to be necessary for the interpreter call stub generator (although it also needs to be fixed to be correct in those cases as well.)