-
Notifications
You must be signed in to change notification settings - Fork 107
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Make use of IDynamicInterfaceCastable #292
Comments
It would be a good idea during this work to consult with the .NET team about potential updates to the CsWinRT generated source. Implementing this currently would result in a substantial code size increase, but updating some of the generated source patterns may yield a trivial code size increase. |
Proposed implementation outline: Define interface IWinRTDynamic : IDynamicInterfaceCastable Observation - majority of interface implementations are declared in metadata
Conclusion - consider dynamically generating the Impl type via TypeBuilder.CreateType and caching. A bit slower on first call but smaller projection, etc. Default implementation of IWinRTDynamic.GetInterfaceImplementation would:
Dynamically generated Impl type members would then:
|
Are there startup performance goals that you are aiming for? Generating types via Reflection.Emit tends to be startup performance bottleneck. |
A ref-emit design would also be very linker-unfriendly and have low/no debuggability. |
This is a huge issue with attempting to debug the built-in system. |
I think we need to consider scope with this feature. How much code in the startup path do we imagine would be using dynamic casts? Again, this should mostly be limited to ComImports, and type-erased params/returns, right? As for debugging, these generated types would be very simply - basically just casting wrappers from Impl to QI result. Is that a huge concern? |
Not only that but is it AOT unfriendly in environments like the Unity editor? I remember Reflection.Emit being very problematic in libraries like Json.NET. |
I'm not sure that is the best framing of the concern. I agree in principle that if this mechanism was only used in a narrow scenario then the proposal could be acceptable. The issue/concern is platform design I think. The AOT, linker, and debuggability concerns are all related to the shape and design of the platform rather than a specific usage of the |
Just chatted with Jeremy, and he suggested an elegant solution where we change the internal shape of generated interface ABIs to match that required by IDynamicInterfaceCastable. That would then eliminate any bloat concerns as our ABI would simply use dynamic casting under the hood. And it would also address issues of debuggability, linking, etc. I'll let Jeremy elaborate when he fleshes out his prototype a bit more. |
Here's the first draft of the my proposal: C#/WinRT IDynamicInterfaceCastable DesignDesign Principles
DesignDefine a public interface IWinRTObject : IDynamicInterfaceCastable
{
bool IDynamicInterfaceCastable.IsInterfaceImplemented(RuntimeTypeHandle interfaceType, bool throwIfNotImplemented)
{
// Call QI
// Type.MakeGenericType to get strongly typed objref
// Insert into cache
}
RuntimeTypeHandle IDynamicInterfaceCastable.GetInterfaceImplementation(RuntimeTypeHandle interfaceType)
{
// TODO: Add ComImport path (dynamically generated wrapper? Require these interfaces to use the old .As pattern?)
return Type.GetTypeFromHandle(interfaceType).GetHelperType().TypeHandle;
}
protected IObjectReference NativeObject { get; }
protected ConcurrentDictionary<RuntimeTypeHandle, IObjectReference> QueryInterfaceCache { get; }
IObjectReference GetObjectReferenceForType(RuntimeTypeHandle type)
{
return QueryInterfaceCache[type];
}
} All generated runtime classes as well as Interface typesGiven interfaces defined as follows: namespace Windows.Foundation
{
public interface IRequired
{
void RequiredMethod();
}
public interface IFoo : IRequired
{
void Bar();
}
} The following would be the implementation of the methods of [DynamicInterfaceCastableImplementation]
[DynamicDependency(DynamicallyAccessedMemberTypes.PublicNestedTypes)]
internal interface IFoo : global::Windows.Foundation.IFoo
{
public struct Vftbl
{
// ....
}
void global::Windows.Foundation.IFoo.Bar()
{
IWinRTObject _this = (IWinRTObject)this;
ObjectReference<Vftbl> objRef = (ObjectReference<Vftbl>)_this.GetObjectReferenceForType(typeof(global::Windows.Foundation.IFoo).TypeHandle);
ExceptionHelpers.ThrowExceptionForHR(objRef.Vftbl.Bar_0(objRef.ThisPtr));
}
void global::Windows.Foundation.IRequired.RequiredMethod()
{
// Force calls to required interfaces to go through their ABI types by going through IDynamicInterfaceCastable
((global::Windows.Foundation.IRequired)(IWinRTObject)this).RequiredMethod();
}
} With this design, none of the interface ABI types need to be public. They can all stay as internal types and not be exposed publicly. Runtime classesRuntime classes would implement [DynamicDependency("FromAbi")]
class Foo : IWinRTObject
{
private IObjectReference _objRef;
IObjectReference IWinRTObject.NativeObject => _objRef;
// Current implementation design for composability support, not part of this proposal specifically.
private Lazy<IFoo> _defaultLazy;
private IFoo _default => _defaultLazy.Value;
public static Foo FromAbi(IObjectReference objRef)
{
return new Foo(objRef.As<ABI.Windows.Foundation.IFoo.Vftbl>());
}
internal Foo(ObjectReference<ABI.Windows.Foundation.IFoo.Vftbl> objRef)
{
_objRef = objRef;
_defaultLazy = new Lazy<IFoo>(() => (IFoo)this);
}
ConcurrentDictionary<RuntimeTypeHandle, IObjectReference> IWinRTObject.QueryInterfaceCache { get; } = new ConcurrentDictionary<RuntimeTypeHandle, IObjectReference>();
// ...
} RCW type discoveryWith this new design, the only possible types to instantiate are runtime classes, delegates, and Linker SafetyWith the |
@jkoritzinsky My first path through I like it. I need to read through it again though. One thing that CsWinRT should also consider is the |
I thought WinRT supported generic classes? |
Until function pointers come in, using Vtftbl types helps save delegate allocations significantly. And even once it comes in, parameterized interfaces will still have to use delegates that are created through
From the WinRT Type System document:
Unless that document is out of date, runtime classes cannot be parameterized. |
I can get behind all of the arguments except for the debuggability one in this case. There really is nothing to debug. It is a contiguously allocated array of With those numbers on a 64-bit machine the entire vtable itself requires 6 * sizeof(void*) = 48 bytes and can be described with minimal metadata cost since it is all primitive types. We count the primitive Lets take the same case for a fully typed vtable on a 64-bit machine. We still have a possible ideal 48 bytes for the value type. But the description of that type is immense compared to the 9 bytes needed above and isn't constant as we need to expand the type for each entry. Let's say we name each entry starting with I am all for debuggability but those numbers don't add up to me. Especially when there are already concerns about size. |
Can you get most of the debuggability benefits by adding comments for the slot numbers, e.g. |
Yes, that would mostly cover it. We'd still have the readability issue of the index being cast to the function pointer and then called right away, which I personally would dislike but is workable. That wouldn't solve the delegate allocation aspect, but that might not be that much of a problem to do separate code paths. |
Just so I am clear the code could look similar to the following, correct? int IMyInterface.MyMethod()
{
var fptr = (delegate* unmanaged<IntPtr,int>)vtbl[6 /* IMyInterface.MyMethod */];
return fptr(this._this);
} |
Yeah something similar to that. In your case the function pointer would be |
I'm not sure how this discussion veered into function pointers. We should keep that work distinct from dynamic casting support (although I agree with the comments). Yes, runtime classes are all concrete (not parametric - that still only applies to interfaces and delegates). In general, ComImport interfaces can't be described in WinRT metadata, thus can't be projected by cswinrt, so alternatives are:
I vote for the last - AsComImport<> cast |
That was me being difficult - sorry. The conversation can/should be deferred once we start investigating the size concerns.
Thanks for the confirmation. I must be misremembering some previous conversations. I will follow-up offline.
Seems reasonable. Is this the fallback logic to the runtime built-in COM support or are these one off support scenarios provided by CsWinRT? |
The |
Nice. Can I assume that an analyzer can detect possible misused by checking if a type inherits from |
Yes, an analyzer should be able to detect using |
Couldn't something like a source generator be used for the ComImport support? For code that looks something like [ComImport]
[InterfaceType(ComInterfaceType.InterfaceIsIUnknown)]
[Guid("3E68D4BD-7135-4D10-8018-9FB6D9F33FA1")]
public interface IInitializeWithWindow
{
void Initialize(IntPtr hwnd);
}
// ...
var dialog = new MessageDialog();
var withWindow = (IInitializeWithWindow) myMessageDialog; this source generator could see that a cast is being made to [DynamicInterfaceCastableImplementation]
internal interface IInitializeWithWindowImpl : IInitializeWithWindow
{
void IInitializeWithWindow.Initialize(IntPtr hwnd)
{
using var objRef = GetRefForObject(this);
Marshal.QueryInterface(objRef.ThisPtr, ref iid, out IntPtr comPtr);
try
{
((IInitializeWithWindow) Marshal.GetObjectForIUnknown(comPtr)).Initialize(hwnd);
}
finally
{
Marshal.Release(comPtr);
}
}
[ModuleInitializer]
public static void AddDynamicInterfaceCastableImpl()
{
ComImportSupport.AddDynamicInterfaceCastableImplForComImport(typeof(IInitializeWithWindow), typeof(IInitializeWithWindowImpl));
}
} C#/WinRT would need to be slightly augmented: public static class ComImportSupport
{
// Is this unloading safe?
private static ConditionalWeakTable<Type, Type> ImplementationsForComImport = new();
public static void AddDynamicInterfaceCastableImplForComImport(Type comImport, Type dynamicInterfaceCastableImplementation)
{
ImplementationsForComImport.Add(comImport, dynamicInterfaceCastableImplementation);
}
public static Type? GetDynamicInterfaceCastableImplForComImport(Type comImport, bool throwIfNotFound)
{
if (ImplementationsForComImport.TryGetValue(comImport, out Type dynamicInterfaceCastableImpl)
{
return dynamicInterfaceCastableImpl;
}
else if (throwIfNotFound)
{
throw new InvalidCastException(...);
}
else
{
return null;
}
}
}
public interface IWinRTObject : IDynamicInterfaceCastable
{
RuntimeTypeHandle IDynamicInterfaceCastable.GetInterfaceImplementation(RuntimeTypeHandle interfaceType)
{
// check for Windows Runtime types first...
return ComImportSupport.GetDynamicInterfaceCastableImplForComImport(Type.GetTypeFromHandle(interfaceType), throwIfNotFound: true);
}
} |
PR #369 |
dotnet/runtime#36654
dotnet/runtime#37042
The text was updated successfully, but these errors were encountered: