Convert COMCustomAttribute from MDCS to UCO#126542
Conversation
58cc7e0 to
a59dd87
Compare
a59dd87 to
7a3eaba
Compare
|
@jkotas, @AaronRobinsonMSFT, I tried a few approaches to avoid reflection stack, and ended up with this cached stub-gen. Is this the right path forward? The AI agent I was working analyzed and found that we can potentially piggyback on this approach for funceval (the last MDCS usage). |
AaronRobinsonMSFT
left a comment
There was a problem hiding this comment.
A bunch of style nits.
U1ARRAYREF -> BOOLARRAYREF
IntPtr -> void*
I'm still trying to understand this approach. I know it is following the newer Reflection approach and it looks very similar to what Jan has attempted to explain to me a few times. I just don't have the intuition yet. I'd wager it is closer to correct than not though.
It is duplicating the newer reflection approach. I do not think we want to have a different strategy for dynamic invoke thunks for each reflection invoke-like path, each with different bugs and perf characteristics. I think we want to have one way to do this and have all reflection invoke-like paths to call into it. Avoiding reflection stack completely is a non-goal. We want to avoid code duplication. It is fine to refactor the reflection stack to make it possible. Also, it is fine to reduce sharing of the reflection stack with Mono if it makes the refactoring easier. |
|
The custom attribute instantiation reuses the reflection stack already. object[], etc). We have internal InvokePropertySetter method to avoid MethodInfo.Invoke overhead, but the heavy lifting that deals with actual argument passing is reused from MethodInfo.Invoke. This is the kind of approach I have in mind for this.
|
|
It may work better to get rid of CallDescrWorker use in RuntimeMethodHandle.InvokeMethod first. I do not have a strong opinion about the order. Also, both RuntimeMethodHandle.InvokeMethod and this one will need extra scrutiny to avoid perf regressions (both startup and throughput). |
In previous conversation, you mentioned reflection stack is heavy and InvokeMethod uses CDW (because it affects app startup perf and eats up JIT budget). That's why I opted for the IL stub caching approach. In last commit, I have switch to |
|
@EgorBot -osx_arm64 -linux_x64 -windows_x64 using System;
using BenchmarkDotNet.Attributes;
using BenchmarkDotNet.Running;
BenchmarkSwitcher.FromAssembly(typeof(CustomAttributeCtorBench).Assembly).Run(args);
[MemoryDiagnoser]
public class CustomAttributeCtorBench
{
[Benchmark]
public int SingleArgCtor()
{
var a = (SingleArgAttribute)Attribute.GetCustomAttribute(
typeof(SingleArgTarget),
typeof(SingleArgAttribute),
inherit: false)!;
return a.A;
}
[Benchmark]
public int MultiArgCtor()
{
var a = (MultiArgAttribute)Attribute.GetCustomAttribute(
typeof(MultiArgTarget),
typeof(MultiArgAttribute),
inherit: false)!;
return a.Sum;
}
[SingleArg(42)]
private sealed class SingleArgTarget { }
[MultiArg(1, 2, 3, 4, 5, 6, 7, 8)]
private sealed class MultiArgTarget { }
[AttributeUsage(AttributeTargets.Class, AllowMultiple = false)]
private sealed class SingleArgAttribute : Attribute
{
public readonly int A;
public SingleArgAttribute(int a) => A = a;
}
[AttributeUsage(AttributeTargets.Class, AllowMultiple = false)]
private sealed class MultiArgAttribute : Attribute
{
public readonly int Sum;
public MultiArgAttribute(int a, int b, int c, int d, int e, int f, int g, int h)
=> Sum = a + b + c + d + e + f + g + h;
}
} |
|
@EgorBot -osx_arm64 -linux_x64 -windows_x64 using System;
using BenchmarkDotNet.Attributes;
using BenchmarkDotNet.Running;
BenchmarkSwitcher.FromAssembly(typeof(CustomAttributeCtorBench).Assembly).Run(args);
[MemoryDiagnoser]
public class CustomAttributeCtorBench
{
[Benchmark]
public object ZeroArgCtor()
{
return Attribute.GetCustomAttribute(
typeof(ZeroArgTarget),
typeof(ZeroArgAttribute),
inherit: false)!;
}
[Benchmark]
public int SingleArgCtor()
{
var a = (SingleArgAttribute)Attribute.GetCustomAttribute(
typeof(SingleArgTarget),
typeof(SingleArgAttribute),
inherit: false)!;
return a.A;
}
[Benchmark]
public object FiveArgCtor()
{
return Attribute.GetCustomAttribute(
typeof(FiveArgTarget),
typeof(FiveArgAttribute),
inherit: false)!;
}
[ZeroArg]
private sealed class ZeroArgTarget { }
[SingleArg(42)]
private sealed class SingleArgTarget { }
[FiveArg(123, 3.14, "hello", FiveArgAttribute.Kind.Alpha, typeof(string))]
private sealed class FiveArgTarget { }
[AttributeUsage(AttributeTargets.Class, AllowMultiple = false)]
private sealed class ZeroArgAttribute : Attribute
{
public ZeroArgAttribute() { }
}
[AttributeUsage(AttributeTargets.Class, AllowMultiple = false)]
private sealed class SingleArgAttribute : Attribute
{
public readonly int A;
public SingleArgAttribute(int a) => A = a;
}
[AttributeUsage(AttributeTargets.Class, AllowMultiple = false)]
private sealed class FiveArgAttribute : Attribute
{
public enum Kind { Alpha, Beta, Gamma }
public readonly int I;
public readonly double D;
public readonly string S;
public readonly Kind K;
public readonly Type T;
public FiveArgAttribute(int i, double d, string s, Kind k, Type t)
{
I = i;
D = d;
S = s;
K = k;
T = t;
}
}
} |
|
@EgorBot -osx_arm64 -linux_x64 -windows_x64 using System;
using BenchmarkDotNet.Attributes;
using BenchmarkDotNet.Running;
BenchmarkSwitcher.FromAssembly(typeof(CustomAttributeCtorBench).Assembly).Run(args);
[MemoryDiagnoser]
public class CustomAttributeCtorBench
{
[Benchmark]
public object ZeroArgCtor()
{
return Attribute.GetCustomAttribute(
typeof(ZeroArgTarget),
typeof(ZeroArgAttribute),
inherit: false)!;
}
[Benchmark]
public int SingleArgCtor()
{
var a = (SingleArgAttribute)Attribute.GetCustomAttribute(
typeof(SingleArgTarget),
typeof(SingleArgAttribute),
inherit: false)!;
return a.A;
}
[Benchmark]
public object FiveArgCtor()
{
return Attribute.GetCustomAttribute(
typeof(FiveArgTarget),
typeof(FiveArgAttribute),
inherit: false)!;
}
[ZeroArg]
private sealed class ZeroArgTarget { }
[SingleArg(42)]
private sealed class SingleArgTarget { }
[FiveArg(123, 3.14, "hello", FiveArgAttribute.Kind.Alpha, typeof(string))]
private sealed class FiveArgTarget { }
[AttributeUsage(AttributeTargets.Class, AllowMultiple = false)]
private sealed class ZeroArgAttribute : Attribute
{
public ZeroArgAttribute() { }
}
[AttributeUsage(AttributeTargets.Class, AllowMultiple = false)]
private sealed class SingleArgAttribute : Attribute
{
public readonly int A;
public SingleArgAttribute(int a) => A = a;
}
[AttributeUsage(AttributeTargets.Class, AllowMultiple = false)]
private sealed class FiveArgAttribute : Attribute
{
public enum Kind { Alpha, Beta, Gamma }
public readonly int I;
public readonly double D;
public readonly string S;
public readonly Kind K;
public readonly Type T;
public FiveArgAttribute(int i, double d, string s, Kind k, Type t)
{
I = i;
D = d;
S = s;
K = k;
T = t;
}
}
} |
|
With the original (native stub-gen +caching), f3e9b82, there is no regression EgorBot/Benchmarks#92 (although I haven't measured timing of startup/first run). With regular reflection invoke, that involves |
|
I had a thought of another approach: Instead of dynamically figuring out the shape of the constructor and constructing the correct arguments for the constructor dynamically, what if we were to emit a UCO IL stub to construct the attribute and generate the IL stub based on the specific CustomAttributeBlob? Then we could have a known signature, avoid going through the reflection stack entirely, and ensure that we don't introduce another usage of |
We run into a startup perf problem when we tried to implement MethodInfo.Invoke using this strategy. MethodInfo.Invoke has a lot of cases where given method is invoked once or a very few times. Emitting method-specific IL stubs by default is too expensive during startup. The plan for MethodInfo.Invoke has been to R2R pre-compile the stubs for like 100 most common signatures, and JIT for less common signatures or when the given method is executed a lot. #115345 has WIP implementation that we were not able to land in time. I think this should use the same tiered strategy as MethodInfo.Invoke, ideally by sharing the code so that we do not have to maintain and tune implementation of multiple of these system. |
I think this delta is closest to what it should look like. Have you tried to profile it to see where the time is spent and opportunities to optimize it? For example, the casts like |
|
I've tried converting InvokeMethod to UCO path using dynamic/cached IL stub approach: main...am11:runtime:feature/MDCS-to-UCOA-pattern3. It is passing reflection and runtime tests on osx-arm64. |
This is duplicating what managed reflection side does using DynamicMethods. It would introduce a lot of JITing in common scenarios during startup. It won't pass the perf gates - #126542 (comment) . |
|
I have looked at this again. I do not think the startup hit from the extra JITing is going to be acceptable. A simple repro is: It is JITing an extra method with this change. It has worse impact than the current I think the next step here is to partially resurrect #115345 in a separate PR. I will try to get copilot going on it. |
I've ported Steve's changes here and adapted without MDCS/CDW (it just falls back to emit). |
73da9d2 to
dea74f6
Compare
@jkotas, the first call is now cached, but the difference is still there (however, the steady-state is faster):
|
|
@EgorBot -osx_arm64 -linux_x64 -windows_x64 using System;
using BenchmarkDotNet.Attributes;
using BenchmarkDotNet.Running;
BenchmarkSwitcher.FromAssembly(typeof(CustomAttributeCtorBench).Assembly).Run(args);
[MemoryDiagnoser]
public class CustomAttributeCtorBench
{
[Benchmark]
public object ZeroArgCtor()
{
return Attribute.GetCustomAttribute(
typeof(ZeroArgTarget),
typeof(ZeroArgAttribute),
inherit: false)!;
}
[Benchmark]
public int SingleArgCtor()
{
var a = (SingleArgAttribute)Attribute.GetCustomAttribute(
typeof(SingleArgTarget),
typeof(SingleArgAttribute),
inherit: false)!;
return a.A;
}
[Benchmark]
public object FiveArgCtor()
{
return Attribute.GetCustomAttribute(
typeof(FiveArgTarget),
typeof(FiveArgAttribute),
inherit: false)!;
}
[ZeroArg]
private sealed class ZeroArgTarget { }
[SingleArg(42)]
private sealed class SingleArgTarget { }
[FiveArg(123, 3.14, "hello", FiveArgAttribute.Kind.Alpha, typeof(string))]
private sealed class FiveArgTarget { }
[AttributeUsage(AttributeTargets.Class, AllowMultiple = false)]
private sealed class ZeroArgAttribute : Attribute
{
public ZeroArgAttribute() { }
}
[AttributeUsage(AttributeTargets.Class, AllowMultiple = false)]
private sealed class SingleArgAttribute : Attribute
{
public readonly int A;
public SingleArgAttribute(int a) => A = a;
}
[AttributeUsage(AttributeTargets.Class, AllowMultiple = false)]
private sealed class FiveArgAttribute : Attribute
{
public enum Kind { Alpha, Beta, Gamma }
public readonly int I;
public readonly double D;
public readonly string S;
public readonly Kind K;
public readonly Type T;
public FiveArgAttribute(int i, double d, string s, Kind k, Type t)
{
I = i;
D = d;
S = s;
K = k;
T = t;
}
}
} |
| Init(typeForwardedToAttribute); | ||
| else | ||
| Init(attribute); | ||
| if (attribute is DllImportAttribute dllImportAttribute) |
There was a problem hiding this comment.
Could you please revert the custom attribute from this PR so that this PR is focused on MethodInfo.Invoke only?
We can revert the revert into a new to resurrect the delta once MethodInfo.Invoke is in.
I think it will work best to land this in this order:
- MethodInfo.Invoke
- Custom attributes
- Func eval
| BOOL IsActivationNeeded() | ||
| { | ||
| LIMITED_METHOD_CONTRACT; | ||
| return (m_dwFlags & METHOD_INVOKE_NEEDS_ACTIVATION) != 0; |
There was a problem hiding this comment.
src\coreclr\tools\Common\CallingConvention\ArgIterator.cs(1855):// METHOD_INVOKE_NEEDS_ACTIVATION = 0x0040, // Flag used by ArgIteratorForMethodInvoke
src\coreclr\vm\callingconvention.h(1057):METHOD_INVOKE_NEEDS_ACTIVATION = 0x0100, // Flag used by ArgIt
This can be delete too
| emitDelegate = CreateInvokeDelegate_RefArgs(_method); | ||
| } | ||
|
|
||
| // Don't cache for collectible assemblies: the DynamicMethod holds token |
There was a problem hiding this comment.
I do not understand why we need to skip caching for collectible assemblies. The DynamicMethod should only ever reference types used by the method signature that cannot have shorter lifetime than the method itself.
| _invokeFunc_RefArgs = InterpretedInvoke; | ||
| } | ||
|
|
||
| private unsafe object? InterpretedInvoke(object? obj, IntPtr* args) |
There was a problem hiding this comment.
It is confusing to call this InterpretedInvoke given that there is nothing interpreted. I am not sure whether we want to do something about it in this PR.
| // Cached intrinsic thunk from a previous call: dispatch directly. | ||
| if (_intrinsicThunk is not null) | ||
| { | ||
| return _intrinsicThunk(_intrinsicFn, obj, args, _method.DeclaringType); |
There was a problem hiding this comment.
I think we should JIT the thunk if it is very hot reflection method - maintain a counter and dynamically generate the custom thunk after like 100 invocations (we can run some tests to figure out the threshold - I think 100 should be in the ballpark).
Otherwise, we will see regression compare to what we have today.
|
|
||
| private unsafe object? InterpretedInvoke_Method(object? obj, IntPtr* args) => | ||
| RuntimeMethodHandle.InvokeMethod(obj, (void**)args, _signature!, isConstructor: false); | ||
| // Emit fallback. The QCALL interpreted dispatcher was removed in this branch as part of |
There was a problem hiding this comment.
Comments that talk about the past shape of the code are not useful
| else if (((strategy & InvokerStrategy.HasBeenInvoked_ObjSpanArgs) == 0) && !Debugger.IsAttached) | ||
| { | ||
| // The first time, ignoring race conditions, use the slow path, except for the case when running under a debugger. | ||
| // This is a workaround for the debugger issues with understanding exceptions propagation over the slow path. |
There was a problem hiding this comment.
These debugger issues were specific to the CoreCLR interpreted path. This special casing of the debugger can be deleted now
|
@jkotas unfortunately I won't have time to work on this anytime soon, as a lot of work (and massive follow up cleanups) was done on a machine which I don't have. Please ask the AI agent to cherry-pick and split appropriately. I started it off as custom attributes conversion and it has evolved into "implementing the missing pieces new reflection stack". At this point splitting the PR is doable, but remembering how many times I had to debug Checked vs. Debug, Windows vs Unix to hold all the cards together, it's non-trivial and I'd rather have AI do the honors. :) |
|
Workflow state for the Holistic Review Orchestrator. {
"version": 5,
"last_dispatched_commit": "ef06e026694c0e2f6ea46787ad861cb9ed7da972",
"last_dispatched_base_ref": "main",
"last_dispatched_base_sha": "3366f6acb14d67cbff4663cc6021a6207582ad65",
"last_reviewed_commit": "ef06e026694c0e2f6ea46787ad861cb9ed7da972",
"last_reviewed_base_ref": "main",
"last_reviewed_base_sha": "3366f6acb14d67cbff4663cc6021a6207582ad65",
"last_recorded_worker_run_id": "29686402378",
"review_attempt_commit": "",
"review_attempt_base_ref": "",
"review_attempt_count": 0,
"max_review_attempts": 5,
"review_history_format": "holistic-review-disclosure-v1",
"review_history": [
{
"commit": "ef06e026694c0e2f6ea46787ad861cb9ed7da972",
"review_id": 4730745705
}
]
} |
There was a problem hiding this comment.
Holistic Review
Motivation: This PR contributes to the prio3 effort (#123864) to eliminate the remaining MethodDescCallSite (MDCS) usages in the runtime. It converts reflection method/constructor invocation and custom-attribute constructor invocation away from the native QCall RuntimeMethodHandle_InvokeMethod MDCS pattern toward a managed UnmanagedCallersOnly (UCO) entry point plus the existing emit/intrinsic dynamic-invoke thunk machinery. This removes ~587 lines from reflectioninvocation.cpp and drops DispatchCallSimple/CallDefaultConstructor from callhelpers.cpp.
Approach: The change adds a per-shape intrinsic thunk fast-path (IntrinsicInvokeHelper.cs), routes custom-attribute ctor invocation through a new managed CustomAttribute.InvokeCustomAttributeCtor UCO method (called from a rewritten CustomAttribute_CreateCustomAttributeInstance that GCPROTECTs a boxed-arg OBJECTREF[] and passes a NativeCtorInvokeContract), and inlines the former CallDefaultConstructor helper at its three native call sites using the existing UnmanagedCallersOnlyCaller. Mono compatibility is preserved via #if MONO branching that keeps the legacy backwardsCompat parameter while CoreCLR drops it.
Summary: The refactor is well-executed at the mechanical level: the GCPROTECT discipline in the rewritten customattribute.cpp looks correct, the intrinsic-thunk classifier only compiles thunks that are actually used, and the collectible-assembly non-caching decision is sound. However, this is a large change to a correctness-sensitive VM/reflection boundary, and the maintainer (jkotas) has already raised a substantive design-direction concern on the PR: that this introduces another separate dynamic-invoke-thunk strategy rather than reusing the single existing reflection invoke stack, and that the custom-attribute path should reuse that stack (as InvokePropertySetter does) instead of duplicating it. That architectural question is unresolved in-thread and dominates the review. Combined with the minor issues below, I am marking this
Detailed Findings
RuntimeCustomAttributeData.cs line 1881 throws new InvalidOperationException("Invalid custom attribute constructor.") with a hardcoded English literal instead of an SR resource, and the exception type is questionable. See the inline comment on that line.
Behavior change: throw → _ASSERTE for missing default constructor (cross-cutting, not actionable inline) — The removed native CallDefaultConstructor threw kMissingMethodException when the type had no default ctor. The three inlined call sites (clrex.cpp EEException::CreateThrowable, runtimecallablewrapper.cpp ~line 1678, reflectioninvocation.cpp RuntimeTypeHandle_CreateInstanceForAnotherGenericParameter ~line 100) replace that with _ASSERTE(pMT->HasDefaultConstructor()). I verified each site guards the precondition beforehand (e.g. RCW throws kArgumentException at runtimecallablewrapper.cpp:1644; the generic-parameter path asserts on an already-validated type), so in practice this is safe. Still worth a maintainer confirming that no release-build path can reach these asserts with a type lacking a default ctor, since the old code hard-threw.
Design direction (architectural, blocking discussion) — Per jkotas's PR comments, the preferred direction is to reuse the single existing reflection invoke stack (the MethodInfo.Invoke / InvokePropertySetter machinery) for the custom-attribute path rather than adding a parallel thunk strategy. The new InvokeCustomAttributeCtor does route through MethodBaseInvoker.InvokeWith*Args, which is a step toward that, but the added IntrinsicInvokeHelper fast-path is the duplication being questioned. This should be reconciled with the maintainer before merge.
Test coverage — No test files are included. The change relies entirely on existing reflection and custom-attribute test suites plus CI. Given the VM/native surface (GC protection, UCO calling convention, exception propagation across the managed/native boundary, array pseudo-constructor invocation after InvokeArrayConstructor removal), a targeted regression test for reflective constructor invocation on an existing instance and for custom-attribute ctors with object-typed null arguments would materially reduce risk.
✅ Positives — GCPROTECT usage in the rewritten CustomAttribute_CreateCustomAttributeInstance correctly protects the boxed-argument array and result; the argCount switch (0/1/2-4/many) correctly matches MethodBaseInvoker.MaxStackAllocArgCount == 4; the intrinsic-thunk classifier avoids compiling unused thunks; and not caching invokers for collectible assemblies (DynamicMethod token pinning) is the right call.
Note
This review was generated by this repository's Holistic Review agentic workflow to complement the built-in Copilot review.
Generated by Holistic Review · 483.2 AIC · ⌖ 11.3 AIC · ⊞ 10K
Contributes to prio3 #123864