Downgrade MethodTables used in reflection invoke #111610
Conversation
|
/azp run runtime-nativeaot-outerloop |
|
Azure Pipelines successfully started running 1 pipeline(s). |
Reverts #92994 (manually since there were many merge conflicts due to EETypePtr deletion.)
|
/azp run runtime-nativeaot-outerloop |
|
Azure Pipelines successfully started running 1 pipeline(s). |
|
/azp run runtime-nativeaot-outerloop |
|
Azure Pipelines successfully started running 1 pipeline(s). |
Size statisticsPull request #111610
|
| @@ -62,6 +62,7 @@ interface IFoo3<T, K, J> | |||
| int Bar3 { get; set; } | |||
| } | |||
|
|
|||
| [Kept (By = Tool.NativeAot)] | |||
There was a problem hiding this comment.
This test is not actually testing anything relevant, so I'm not worried about "regression" here.
- In ILLinker, looks like
preserve=fieldsis going to skip over compiler generated backing fields. Not sure where that logic is but we clearly don't do that in native AOT and we do preserve them. Won't lose my sleep over that. - The test infrastructure doesn't check field preservation on native AOT side since we don't really "trim" fields (we vacate space for them no matter what, there's leeway whether reflection metadata is generated).
So the only observable effect in this test is whether the type of the field survived and now it by design survives if the field was a target of reflection.
There was a problem hiding this comment.
The test failures look like a real problem introduced by this change:
System.NotSupportedException : 'System.Nullable`1[System.EnvironmentVariableTarget]' is missing native code or metadata. This can happen for code that is not compatible with trimming or AOT. Inspect and fix trimming and AOT related warnings that were generated when the app was published. For more information see https://aka.ms/nativeaot-compatibility
at System.Reflection.Runtime.TypeInfos.RuntimeTypeInfo.get_TypeHandle() + 0x94
at System.Reflection.DynamicInvokeInfo..ctor(MethodBase, IntPtr) + 0xdc
at Internal.Reflection.Execution.ExecutionEnvironmentImplementation.TryGetMethodInvokeInfo(RuntimeTypeHandle, QMethodDefinition, RuntimeTypeHandle[], MethodBase, MethodSignatureComparer&, CanonicalFormKind) + 0x168
at Internal.Reflection.Execution.ExecutionEnvironmentImplementation.TryGetMethodInvoker(RuntimeTypeHandle, QMethodDefinition, RuntimeTypeHandle[]) + 0xcb
at Internal.Reflection.Core.Execution.ExecutionEnvironment.GetMethodInvoker(RuntimeTypeInfo, QMethodDefinition, RuntimeTypeInfo[], MemberInfo, Exception&) + 0xc7
at System.Reflection.Runtime.MethodInfos.NativeFormat.NativeFormatMethodCommon.GetUncachedMethodInvoker(RuntimeTypeInfo[], MemberInfo, Exception&) + 0x49
at System.Reflection.Runtime.MethodInfos.RuntimeNamedMethodInfo`1.GetUncachedMethodInvoker(RuntimeTypeInfo[], MemberInfo) + 0x1b
at System.Reflection.Runtime.MethodInfos.RuntimeMethodInfo.Invoke(Object, BindingFlags, Binder, Object[], CultureInfo) + 0x2b
--- End of stack trace from previous location ---
That's annoying. We don't keep track of unconstructed generic So reflection cannot find the |
|
Yeah, as expected, forcing all the generic types back into existence makes this no longer save much. In particular it brings back the problem with "ValueTuple in a signature of a reflection-targeted method is surprisingly expensive". Now I wonder if we should undo dotnet/runtimelab#1319 and fix that some other way. This would lead to Size statisticsPull request #111610
|
It would be fine with me if it comes with good error message. What is the error message we generate to unusable type handles? I do not have a strong opinion. |
Grabbing TypeHandle will take us to the same old MissingMetadata-like experience here (we now instruct people to inspect publish-time warnings): I have a sketch of a fix at #112986. |
|
How come this saves 10 KB in kestrel minimal, but regresses 1.5 KB in CsWinRT? |
The relevant table is the 512 byte regression: #111610 (comment) The 1.5 kB regression is the current state of the PR, which is not what I want to merge. I can't tell what the problem is because rt-sz doesn't upload mstat files for the WinRT projects. I think I fixed it in MichalStrehovsky/rt-sz@5b17679 but I haven't rerun the measurement since. |
There was a problem hiding this comment.
LGTM, assuming the tests pass and the original size savings still hold #111610 (comment)
Yup, latest rt-sz results are supportive! |
|
/azp run runtime-nativeaot-outerloop |
|
Azure Pipelines successfully started running 1 pipeline(s). |
|
Outerloop found an issue in one of the pri-1 tests. Trying to mark f1 as reflected on brings in the recursive dependency. Previously the optimization from #92994 that I'm rolling back in dc34018 would avoid the dependency because Expansive2 is a reference type but we now need to check for recursion. class Expansive2<A,B>
{
public Expansive2<Expansive2<A,B>, B > f1;
public Expansive2(Expansive2<Expansive2<A,B>, B> a1) { f1 = a1; }
} |
|
/azp run runtime-nativeaot-outerloop |
|
Azure Pipelines successfully started running 1 pipeline(s). |
|
|
||
| if (!(RuntimeImports.AreTypesAssignable(srcEEType, dstEEType) || | ||
| (dstEEType->IsInterface && arg is System.Runtime.InteropServices.IDynamicInterfaceCastable castable | ||
| if (!(srcEEType == dstEEType || |
There was a problem hiding this comment.
Just wondering: Since this conditional clause is identical to the one in the parallel method, would extracting a helper method to ensure they never diverge? Seems like this whole block doesn't reference the parameters (except by the indexed-to arg) so it should be possible to extract 682-705.
There was a problem hiding this comment.
We would need to validate it doesn't deoptimize codegen, this code is quite performance sensitive.
When we make a method reflection-callable, we have to make sure the reflection stack can get type handles of types within the method signature. The type handles are used as part of reflection activation to do castability checks (is it valid to call the method with this parameter?) and to do boxing of return values.
In the past, what we did:
This was too much, so we restricted it in #92994 into:
This is still a bit too much, as demonstrated in #111544 (comment).
In this PR, I'm restricting this further to:
I'm deleting the "GC pointer could have null methodtable" complication since it doesn't help much in practice once we downgraded to necessary method tables (that revert is in dc34018 and rt-sz measurements show it really doesn't affect much).
Cc @dotnet/ilc-contrib