Skip to content

Conversation

@Sergio0694
Copy link
Member

This PR includes two tweaks to Marshaler<T>'s static cctor:

  • Replace some labdas with method group expressions to generate less stub classes (see here)
  • Don't cache Type in the static cctor, but rather use typeof(T) every time to help the linker

cc. @MichalStrehovsky @jkoritzinsky

@Sergio0694 Sergio0694 added performance Related to performance work trimming AOT labels Dec 12, 2023
@Sergio0694
Copy link
Member Author

@manodasanW it seems this test is very flaky, I'm getting random failures always from that one across PRs 🤔

Failed UnitTest.TestCSharp.TestProxiedDelegate [1 s]
Error Message:
 System.Runtime.InteropServices.COMException : 
Stack Trace:
   at WinRT.ExceptionHelpers.<ThrowExceptionForHR>g__Throw|39_0(Int32 hr)
 at WinRT.ExceptionHelpers.ThrowExceptionForHR(Int32 hr)
 at ABI.Windows.Foundation.AsyncActionCompletedHandler.NativeDelegateWrapper.Invoke(IAsyncAction asyncInfo, AsyncStatus asyncStatus)
 at UnitTest.OOPAsyncAction.Close() in D:\a\1\s\src\Tests\UnitTest\OOPObject.cs:line 107
 at UnitTest.TestCSharp.TestProxiedDelegate() in D:\a\1\s\src\Tests\UnitTest\TestComponentCSharp_Tests.cs:line 2927
 at System.RuntimeMethodHandle.InvokeMethod(Object target, Void** arguments, Signature sig, Boolean isConstructor)
 at System.Reflection.MethodBaseInvoker.InvokeWithNoArgs(Object obj, BindingFlags invokeAttr)

@manodasanW
Copy link
Member

@manodasanW it seems this test is very flaky, I'm getting random failures always from that one across PRs 🤔

Failed UnitTest.TestCSharp.TestProxiedDelegate [1 s]
Error Message:
 System.Runtime.InteropServices.COMException : 
Stack Trace:
   at WinRT.ExceptionHelpers.<ThrowExceptionForHR>g__Throw|39_0(Int32 hr)
 at WinRT.ExceptionHelpers.ThrowExceptionForHR(Int32 hr)
 at ABI.Windows.Foundation.AsyncActionCompletedHandler.NativeDelegateWrapper.Invoke(IAsyncAction asyncInfo, AsyncStatus asyncStatus)
 at UnitTest.OOPAsyncAction.Close() in D:\a\1\s\src\Tests\UnitTest\OOPObject.cs:line 107
 at UnitTest.TestCSharp.TestProxiedDelegate() in D:\a\1\s\src\Tests\UnitTest\TestComponentCSharp_Tests.cs:line 2927
 at System.RuntimeMethodHandle.InvokeMethod(Object target, Void** arguments, Signature sig, Boolean isConstructor)
 at System.Reflection.MethodBaseInvoker.InvokeWithNoArgs(Object obj, BindingFlags invokeAttr)

Yes, that test is indeed a known flaky one. It got flakier recently for some reason (probably a timing issue). A rerun typically makes it pass. That test does needs to be looked into to make it more reliable but so far it has been hard to repro locally.

@jlaanstra
Copy link
Collaborator

jlaanstra commented Dec 12, 2023

How is repeating the same expression typeof(T) multiple times helping the linker? Is typeof(T) cheap enough to just repeat it a bunch of times?

@Sergio0694
Copy link
Member Author

"How is repeating the same expression typeof(T) multiple times helping the linker?"

typeof(T) is a JIT time constant and the IL scanner can also infer information from it. If we cache it in a local instead, the flow analysis loses track of it and then it can no longer eliminate branches, because it can't know they won't ever be taken.

"Is typeof(T) cheap enough to just repeat it a bunch of times?"

It's essentially free, yes. On .NET 8+:

  • For direct type comparisons for value type generics, it's a constant
  • For direct type comparisons for reference type generics, it's a direct method table comparison
  • Anywhere else, it's just loading the baked address of the Type instance from the frozen object heap

So essentially it's super fast and you should never cache it, that's effectively a de-optimization. It can technically be beneficial to cache it on .NET Framework since that doesn't apply there, but I don't think we really care about that anyway.

@Sergio0694 Sergio0694 merged commit 4b40c13 into staging/AOT Dec 13, 2023
@Sergio0694 Sergio0694 deleted the user/sergiopedri/marshaller-t-tweaks branch December 13, 2023 18:39
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

AOT performance Related to performance work trimming

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants