Fix IDynamicInterfaceCastable with shared generic code #108235
Conversation
Fixes dotnet#72909. Internal team ran into this. Turns out CsWinRT also needs this, but they're were working around instead pushing on a fix. The big problem with this one is that we have an interface call to a default interface method that requires generic context. This means we need some kind of instantiating thunk (since callsite didn't provide generic context because it didn't know it). The normal default interface case uses an instantiating thunk that simply indexes into the interface list of `this`. We know the index of the interface (we don't know the concrete type because `T`s could be involved), but we can easily compute it at runtime from `this`. The problem with `IDynamicInterfaceCastable` is that `this` is useless (the class doesn't know anything about the interface). So we need to get the generic context from somewhere else. In this PR, I'm using the thunkpool as "somewhere else". When we finish interface lookup and find out `IDynamicInterfaceCastable` provided a shared method, we create a thunkpool thunk that stashes away the context. We then call the "default interface method instantiating thunk" and instead of indexing into interface list of `this`, we index into interface list of whatever was stashed away. So there are two thunks before we reach the point of executing the method body.
|
I see there is a certain amount of code added to support downstream (WASM). The dynamic instantiation thunk approach won't work on WASM - the thunk pool doesn't work on WASM. Does implementing this fundamentally require the ability to generate new code at runtime, like with |
Yes, I mindlessly copied the fat function pointer indication bit since dispatch maps never have fat function pointers (if they had, this problem wouldn't exist).
The problem is that we need to inject an extra parameter into method calls. I don't see any way around that. CoreCLR JITs new instantiation thunks on demand. We can't. |
| if (result == IntPtr.Zero) | ||
| { | ||
| IDynamicCastableGetInterfaceImplementationFailure(instance, interfaceType, implType); | ||
| } | ||
|
|
||
| if (genericContext != null) |
There was a problem hiding this comment.
This looks same as the delegate thunk pool. Should we just have one shared thunk pool?
There was a problem hiding this comment.
It feels more risky to touch the delegate thunk pool. We might need to service this to .NET 9. With that in mind would you still prefer we use the same pool?
There was a problem hiding this comment.
I do not have strong opinion. We can clean it up later. There is quite a bit leftovers from when the thunk pool was in the shared MRT.
|
/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). |
|
Is this going to introduce measurable binary size regression for typical apps on Apple platforms? I would expect that the thunk pool and the associated templates were not linked into basic apps on Apple platforms before this change, and it is going to be linked in most of the time after this change. (I tried submitting https://github.com/MichalStrehovsky/rt-sz job to get some binary size numbers. It does not seem to work.) |
It was broken due to .NET 9 -> .NET 10 renaming. Tweaked it to represent current point it time but it will get broken in the coming weeks again.
Would we expect that to be disproportionate compared to non-Apple platforms (they have the same problem)? Looks like this is single digit kB outside Apple. rt-sz doesn't measure Apple platforms because that support is a PITA to maintain and usually corresponds to Linux anyway. Cc @ivanpovazan @matouskozak FYI if we see a regression in automation after this merges. |
Fixes dotnet#72909. Internal team ran into this. Turns out CsWinRT also needs this, but they're were working around instead pushing on a fix. The big problem with this one is that we have an interface call to a default interface method that requires generic context. This means we need some kind of instantiating thunk (since callsite didn't provide generic context because it didn't know it). The normal default interface case uses an instantiating thunk that simply indexes into the interface list of `this`. We know the index of the interface (we don't know the concrete type because `T`s could be involved), but we can easily compute it at runtime from `this`. The problem with `IDynamicInterfaceCastable` is that `this` is useless (the class doesn't know anything about the interface). So we need to get the generic context from somewhere else. In this PR, I'm using the thunkpool as "somewhere else". When we finish interface lookup and find out `IDynamicInterfaceCastable` provided a shared method, we create a thunkpool thunk that stashes away the context. We then call the "default interface method instantiating thunk" and instead of indexing into interface list of `this`, we index into interface list of whatever was stashed away. So there are two thunks before we reach the point of executing the method body.
Yes, because of this: https://github.com/dotnet/runtime/blob/main/src/coreclr/nativeaot/Runtime/Full/CMakeLists.txt#L9-L15 The static binary cost of |
…108376) These had to be excluded in dotnet#108235 but dotnet#108328 fixed them in the meantime. One of these PRs might be serviced for .NET 9 and the other may not, so I'm just reenabling this in a separate PR to make backports easier.
There is a regression for nativeAOT on iOS of around ~600 kB dotnet/perf-autofiling-issues#42390. |
Did you mean to say ~60 kB? |
I did, yes only 60kB, sorry. 600 kB would be too much :) |
|
Seems like the relevant number is 2% of app size, which does not feel particularly impactful. |
…108376) These had to be excluded in dotnet#108235 but dotnet#108328 fixed them in the meantime. One of these PRs might be serviced for .NET 9 and the other may not, so I'm just reenabling this in a separate PR to make backports easier.
|
/backport to release/9.0-staging |
|
Started backporting to release/9.0-staging: https://github.com/dotnet/runtime/actions/runs/11892744623 |
Fixes #72909.
Internal team ran into this. Turns out CsWinRT also needs this, but they were silently working around instead of pushing on a fix.
The big problem with this one is that we have an interface call to a default interface method that requires generic context. This means we need some kind of instantiating thunk (since callsite didn't provide generic context because it didn't know it). The normal default interface case uses an instantiating thunk that simply indexes into the interface list of
this. We know the index of the interface (we don't know the concrete type becauseTs could be involved), but we can easily compute it at runtime fromthis.The problem with
IDynamicInterfaceCastableis thatthisis useless (the class doesn't know anything about the interface). So we need to get the generic context from somewhere else. In this PR, I'm using the thunkpool as "somewhere else". When we finish interface lookup and find outIDynamicInterfaceCastableprovided a shared method, we create a thunkpool thunk that stashes away the context. We then call the "default interface method instantiating thunk" and instead of indexing into interface list ofthis, we index into interface list of whatever was stashed away. So there are two thunks before we reach the point of executing the method body.Submitting as a draft. This needs one more thing - a cache of all the thunks we already generated. I just wanted to run the general approach by everyone before making it even more complicated. This is already too complicated for my taste so maybe someone has a better idea.
Cc @dotnet/ilc-contrib @dotnet/interop-contrib