Use shared managed thunks for reflection invocation - #133662
jkoritzinsky wants to merge 18 commits into
Conversation
|
Azure Pipelines: Successfully started running 3 pipeline(s). 13 pipeline(s) were filtered out due to trigger conditions. There may be pipelines that require an authorized user to comment /azp run to run. |
|
Tagging subscribers to this area: @steveisok, @dotnet/area-system-reflection |
c3f6c6f to
12bb7ac
Compare
|
Azure Pipelines: Successfully started running 3 pipeline(s). 13 pipeline(s) were filtered out due to trigger conditions. There may be pipelines that require an authorized user to comment /azp run to run. |
There was a problem hiding this comment.
Copilot review overview
🟡 Changes recommended
Three unresolved findings affect constructor initialization, unmanaged-only/P/Invoke handling, and existing-instance constructor IL.
Get a fresh assessment by requesting another Copilot review.
Review tier: Lite
Findings: 2
Open (2)
What changed in this PR
This PR routes selected reflection invocation paths through shared managed thunks while retaining emitted fallbacks and specialization.
Changes:
- Adds bounded shared method and constructor thunk families with 10,000-call promotion.
- Updates virtual dispatch, constructor handling, calli integration, and CoreCLR/ReadyToRun support.
- Expands reflection and boundary tests; no public API is added.
| File | Reviewed change |
|---|---|
src/libraries/System.Runtime/tests/System.Reflection.Tests/MethodInvokerTests.cs |
Tests method thunk promotion, dispatch, and boundaries. |
src/libraries/System.Runtime/tests/System.Reflection.Tests/ConstructorInvokerTests.cs |
Tests constructor promotion and existing-instance scenarios. |
src/libraries/System.Runtime/tests/System.Reflection.Tests/ConstructorInfoTests.cs |
Tests constructor shapes, semantics, and fallback behavior. |
src/libraries/System.Private.CoreLib/src/System/Reflection/MethodInvokerCommon.cs |
Updates common invocation strategy and caching. |
src/libraries/System.Private.CoreLib/src/System/Reflection/MethodInvoker.cs |
Routes managed method invocation through shared thunks. |
src/libraries/System.Private.CoreLib/src/System/Reflection/MethodBaseInvoker.Constructor.cs |
Integrates constructor invocation state and strategy selection. |
src/libraries/System.Private.CoreLib/src/System/Reflection/InvokerEmitUtil.cs |
Updates emitted fallback generation; existing-instance constructor IL has an inconsistent stack path (critical). |
src/coreclr/vm/runtimehandles.h |
Declares receiver-specific target resolution support. |
src/coreclr/vm/runtimehandles.cpp |
Implements receiver-specific virtual target resolution. |
src/coreclr/vm/reflectioninvocation.cpp |
Updates native reflection invocation integration. |
src/coreclr/vm/qcallentrypoints.cpp |
Registers the new QCall entrypoint. |
src/coreclr/vm/corelib.h |
Registers the CoreLib helper type. |
src/coreclr/vm/callingconvention.h |
Updates calling-convention support. |
src/coreclr/vm/appdomain.cpp |
Updates reflection helper registration. |
src/coreclr/tools/Common/CallingConvention/ArgIterator.cs |
Updates calling-convention argument iteration support. |
src/coreclr/tools/aot/ILCompiler.ReadyToRun/IL/Stubs/InstanceCalliHelperIntrinsics.cs |
Rewrites explicit-this calli signatures. |
src/coreclr/tools/aot/ILCompiler.ReadyToRun.Tests/InstanceCalliHelperTests.cs |
Tests ReadyToRun calli rewriting. |
src/coreclr/System.Private.CoreLib/System.Private.CoreLib.csproj |
Includes CoreCLR thunk sources. |
src/coreclr/System.Private.CoreLib/src/System/RuntimeHandles.cs |
Declares managed virtual-target resolution support. |
src/coreclr/System.Private.CoreLib/src/System/Reflection/MethodInvoker.CoreCLR.cs |
Routes CoreCLR method invocation through shared thunks. |
src/coreclr/System.Private.CoreLib/src/System/Reflection/MethodBaseInvoker.CoreCLR.cs |
Integrates CoreCLR invocation state. |
src/coreclr/System.Private.CoreLib/src/System/Reflection/IntrinsicInvokeHelper.cs |
Classifies and dispatches shared thunks; constructor initialization and unmanaged-only/P/Invoke handling require changes (moderate, critical). |
src/coreclr/System.Private.CoreLib/src/System/Reflection/InstanceCalliHelper.cs |
Adds explicit-this calli helpers. |
src/coreclr/System.Private.CoreLib/src/System/Reflection/ConstructorInvoker.CoreCLR.cs |
Routes CoreCLR constructor invocation through shared thunks. |
src/coreclr/nativeaot/System.Private.CoreLib/src/System/Reflection/MethodInvoker.cs |
Maintains the NativeAOT method invoker implementation. |
There was a problem hiding this comment.
Copilot review overview
🟡 Changes recommended
Shared calli paths can bypass required type initialization, and forced-emitted mode currently selects shared thunks.
Get a fresh assessment by requesting another Copilot review.
Review tier: Lite
Findings: 3
Open (3)
Resolved since last review (1)
b9b2c27 to
95ec713
Compare
Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
There was a problem hiding this comment.
Copilot review overview
🟡 Changes recommended
Unresolved correctness and fallback issues affect generic constructors, existing-instance array/string constructors, and thunk promotion behavior.
Get a fresh assessment by requesting another Copilot review.
Review effort: Lite
Findings: 2
Open (2)
Avoid emitting direct existing-instance constructor calls for string and array constructors. Keep those special constructor forms on the allocation-style emitted path so reflection invoke does not call VM special constructors as ordinary instance entrypoints. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Keep string and array existing-instance constructor invocation on the no-op constructor result path while avoiding direct emitted calls to their special VM constructor entrypoints. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
There was a problem hiding this comment.
Copilot review overview
🟡 Changes recommended
Critical virtual function-pointer preparation and moderate dynamic-code fallback issues remain unresolved.
Get a fresh assessment by requesting another Copilot review.
Review effort: Lite
Findings: 1
Open (2)
Resolved since last review (2)
- RuntimeMethodHandle_GetVirtualFunctionPointer now calls PrepareForUseAsAFunctionPointer() after EnsureActive() before resolving the virtual target, matching RuntimeMethodHandle_GetFunctionPointer's contract that value-type parameters be loaded before a raw entrypoint is handed out for a managed calli. - Fix "constuctor" -> "constructor" typo in a comment. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
There was a problem hiding this comment.
Copilot review overview
🟡 Changes recommended
A critical compiler-less CoreCLR fallback issue and moderate constructor initialization coverage gap remain.
Get a fresh assessment by requesting another Copilot review.
Review effort: Lite
Findings: 1
Open (1)
Resolved since last review (2)
|
Test failures related to the PR |
|
@EgorBot --filter "System.Reflection.Invoke.*" |
| method.ContainsGenericParameters || | ||
| (method.CallingConvention & CallingConventions.VarArgs) != 0 || | ||
| (method.Attributes & MethodAttributes.PinvokeImpl) != 0 || | ||
| method.IsDefined(typeof(UnmanagedCallersOnlyAttribute), inherit: false)) |
There was a problem hiding this comment.
What is not going to work when we delete (method.Attributes & MethodAttributes.PinvokeImpl) != 0 and method.IsDefined(typeof(UnmanagedCallersOnlyAttribute), inherit: false checks here?
I do not think the failure modes are any different between the emit path and the precompiled path if we delete these checks. There may be some asserts in checked build - but that's by design for invalid unsafe interop code.
There was a problem hiding this comment.
I would expect PInvoke to just work; and UnmanagedCallersOnlyAttribute to crash with "Invalid Program: attempted to call a UnmanagedCallersOnly method from managed code."
| !IsReferenceType(declaringType) || | ||
| declaringType.IsAbstract || | ||
| declaringType.IsArray || | ||
| declaringType.ContainsGenericParameters || |
There was a problem hiding this comment.
This should not be needed - we have checked method.ContainsGenericParameters above.
… caching invoker AssertPromoted(MethodBase) and AssertNotPromoted(MethodBase, int) called GetCachedInvoker(method) directly as part of evaluating the argument expression, before the ShouldAssertSharedSelection guard (which requires PlatformDetection.IsCoreCLR) could short-circuit — unlike the sibling AssertShared(MethodBase)/AssertFallback(MethodBase) overloads, which check the flag first. On Mono (all failing System.Reflection.Tests / InvokeEmit.Tests / InvokeInterpreted.Tests WASM CI legs run on Mono), RuntimeMethodInfo has no CoreCLR-only "m_invoker" cached-invoker field, so GetCachedInvoker's Assert.NotNull failed immediately regardless of platform. No product code changes; this only affects test-helper gating. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
…ng-type checks TryGetShape's P/Invoke and [UnmanagedCallersOnly] exclusions were unnecessary: the shared-calli path resolves the same stable entry point as the emitted-invoker path (MethodDesc::GetMultiCallableAddrOfCode), so a P/Invoke target dispatches correctly through either path, and an UnmanagedCallersOnly target fails identically in both (the reverse-P/Invoke check lives at the callee's entry point, not in the caller). Also removes the redundant declaringType.ContainsGenericParameters check on constructors: method.ContainsGenericParameters, checked earlier in the same method, already covers the declaring type for constructors. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
There was a problem hiding this comment.
Copilot review overview
🟡 Changes recommended
Unresolved constructor initialization, constructor validation, and UnmanagedCallersOnly handling issues block approval.
Get a fresh assessment by requesting another Copilot review.
Review effort: Lite
Findings: 1


Summary
Layer 1 of the stack for #123864, building on am11's #126542 and the requested split in #126542 (comment). The dependent layers are #133658 (custom attributes), #133659 (func-eval), and #133660 (dead native machinery cleanup).
MethodInfo.Invoke,ConstructorInfo.Invoke,MethodInvoker, andConstructorInvokershapes instead of the native reflection dispatcher.RuntimeFeature.IsDynamicCodeCompiled, not merelyIsDynamicCodeSupported.No public API is added.
Validation
The threshold update passed 1,964 reflection + 631 forced-emitted + 631 forced-shared cases on Windows x64 Checked CoreCLR. Exact boundary tests cover the object, span, and byref strategies; the old 100-call runtime fails those tests as expected. A controlled CoreLib configuration with dynamic code supported but not compiled passed another 631 + 631 forced-mode cases and retained shared invocation beyond 10,000 calls. That configuration is policy coverage, not a claim of executing a native compiler-less platform.
The bounded matrix includes virtual/interface/generic-virtual/default-interface dispatch, delegate
DynamicInvokewith static and multicast callbacks, enum widths/results, reference out-parameter success/false/exception behavior, moving GC, unsupported-shape boundaries, and collectible methods. Later stack validation also executed the invocation suites on Windows x86. Browser/WASI managed CoreLib/ABI generation was checked; actual browser/WASI and big-endian runtime execution was not performed.Performance evidence
Local measurements used a Windows x64 EPYC 7763 Hyper-V VM. The ASP.NET Core survey at
dotnet/aspnetcorerevisiona2ac63c3a56destablishes source-level invocation demand, not measured performance rankings.Adding the bounded families avoided approximately 1.5–5 KiB of managed allocation per first invocation versus the earlier stack's emitted fallback. An actual ASP.NET Core 10.0.9
UseMiddleware/ActivatorUtilitieseight-reference-constructor startup probe measured median 16.8753 → 14.4399 ms and 9,560 → 5,376 bytes across seven paired fresh-process runs. This did not measure ordinary compiled request dispatch or default-constructorActivatorpaths.For the separate 100 → 10,000 threshold comparison, matching Release native hosts and baseline/changed CoreLib were run with ReadyToRun disabled for both. Seventy-two fresh-process observations confirmed the actual boundary. Selected warmed BenchmarkDotNet results were:
These are short VM-local measurements, not universal throughput guarantees. Earlier expanded-shape testing at threshold 100 exposed a profile-sensitive warmed
ConstructorInfo.Invokeeight-argument case at approximately 1.30× baseline; tiering-disabled/direct-loop controls and the actualConstructorInvokermiddleware API did not reproduce that effect. The threshold comparison above is a separate experiment.Stack created with GitHub Stacks CLI • Give Feedback 💬
Note
This PR description and implementation were prepared with GitHub Copilot. Commits that reuse am11's code retain the requested co-author attribution.