Skip to content
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

Closed
AdamBraden opened this issue May 29, 2020 · 25 comments
Closed

Make use of IDynamicInterfaceCastable #292

AdamBraden opened this issue May 29, 2020 · 25 comments
Assignees
Labels
enhancement New feature or request
Milestone

Comments

@AdamBraden
Copy link
Contributor

dotnet/runtime#36654
dotnet/runtime#37042

@AaronRobinsonMSFT
Copy link
Member

AaronRobinsonMSFT commented May 30, 2020

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.

/cc @jkoritzinsky @Scottj1s @jkotas

@Scottj1s Scottj1s changed the title Make use of ICastableObject Make use of IDynamicInterfaceCastable Jun 11, 2020
@manodasanW manodasanW added the enhancement New feature or request label Jun 15, 2020
@Scottj1s
Copy link
Member

Scottj1s commented Jul 1, 2020

Proposed implementation outline:

Define interface IWinRTDynamic : IDynamicInterfaceCastable
And implement this for all projected runtimeclasses and interfaces

Observation - majority of interface implementations are declared in metadata

  • Exclusive-Tos should never by dynamically cast
  • Polymorphics might need to be, although likely rare
  • Remaining (type-erased event args, ComImports, etc) are few

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:

  • Call new CastExtensions.As(object, RuntimeTypeHandle) variant and if successful...
  • CreateOrAdd Impl type via TypeBuilder.CreateType, with CastableObjectImplementation attribute
  • Assign static RuntimeTypeHandle s_TypeHandle member of Impl type
  • Cache QI result back onto object (RC or interface) via CWT<object, Map<RuntimeTypeHandle,object> >
  • Return Impl RuntimeTypeHandle

Dynamically generated Impl type members would then:

  • Fetch cached QI result from 'this' by s_TypeHandle
  • Call thru QI result

@jkotas
Copy link
Member

jkotas commented Jul 1, 2020

generating the Impl type via TypeBuilder.CreateType and caching. A bit slower on first call but smaller projection

Are there startup performance goals that you are aiming for? Generating types via Reflection.Emit tends to be startup performance bottleneck.

@jkoritzinsky
Copy link
Member

A ref-emit design would also be very linker-unfriendly and have low/no debuggability.

@AaronRobinsonMSFT
Copy link
Member

and have low/no debuggability.

This is a huge issue with attempting to debug the built-in system.

@Scottj1s
Copy link
Member

Scottj1s commented Jul 1, 2020

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?

@StephenHodgson
Copy link

A ref-emit design would also be very linker-unfriendly and have low/no debuggability.

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.

@AaronRobinsonMSFT
Copy link
Member

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?

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 IDynamicInterfaceCastable. If we start down this path with the very narrow scope and it remains, then sure we are good. However if the proposal goes forward we could wind up in a place that requires fundamental changes in order to address the above 3 concerns because they will cripple the system at scale.

@Scottj1s
Copy link
Member

Scottj1s commented Jul 1, 2020

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.

@jkoritzinsky
Copy link
Member

jkoritzinsky commented Jul 1, 2020

Here's the first draft of the my proposal:

C#/WinRT IDynamicInterfaceCastable Design

Design Principles

  • Linker-friendly
  • Public surface area of projection assemblies should be as close to the public surface area of WinMDs as possible.

Design

Define a IWinRTObject interface that implements IDynamicInterfaceCastable:

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 WinRT.IInspectable would explicitly implement IWinRTObject.

Interface types

Given 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 IFoo.

[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 classes

Runtime classes would implement IWinRTObject to get IDynamicInterfaceCastable support. To get a reference to their "default" interface, they would use their IDynamicInterfaceCastable implementation to cast to the right type. Their methods that are implemented in terms of the "default" interface would still be implemented the same way as today.

[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 discovery

With this new design, the only possible types to instantiate are runtime classes, delegates, and WinRT.IInspectable. Since delegates don't implement IInspectable and are handled separately, we can ignore them for now. The first change to make would be that if the type is an interface type, we fall back to WinRT.IInpsectable. Additionally, if we make runtime classes constructable from IObjectReferences similar to above, we can reduce our type name parsing significantly. Since runtime classes cannot be parameterized, we could bail out early if our type name is parameterized and immediately fall back to WinRT.IInspectable. This would significantly reduce type name parsing and lookup times. Additionally, we should switch to using FromAbi for instantiating runtime class objects.

Linker Safety

With the DynamicDependency linker attribute above, switching to FromAbi to construct runtime classes, and presumed linker support for default interface methods and IDynamicInterfaceCastable (which already have filed issues), this design should be linker safe by construction with the provided linker hint attributes.

@AaronRobinsonMSFT
Copy link
Member

@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 Vftbl type approach. This doesn't help a lot with safety, but does/can add a fair bit of unnecessary metadata. As long as we are in here doing this work, it might be prudent to convert it to a void*, index into it, and cast at dispatch time. Since this design does hide all the details quite well this could be deferred until later if it represents a complex wrinkle in the current proposal.

@AaronRobinsonMSFT
Copy link
Member

Since runtime classes cannot be parameterized

I thought WinRT supported generic classes?

@jkoritzinsky
Copy link
Member

My first path through I like it. I need to read through it again though. One thing that CsWinRT should also consider is the Vftbl type approach. This doesn't help a lot with safety, but does/can add a fair bit of unnecessary metadata. As long as we are in here doing this work, it might be prudent to convert it to a void*, index into it, and cast at dispatch time. Since this design does hide all the details quite well this could be deferred until later if it represents a complex wrinkle in the current proposal.

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 Delegate.CreateDelegate, which is still expensive. If we go the void* route, we'd have to create these delegate allocations on the fly. C#/WinRT could optimize the non-generic cases, but I don't know if the reduced metadata win is worth it for the reduction in generated code readability (which I feel is a strong component of debuggability) and the cost of duplicating how ObjectReference Vftbl calculation works.

Since runtime classes cannot be parameterized

I thought WinRT supported generic classes?

From the WinRT Type System document:

WinRT supports type parameterization of interfaces and delegates. The parameterized types permit a family of interfaces to be defined that may be processed polymorphically in programming languages that support parametric polymorphism.

Unless that document is out of date, runtime classes cannot be parameterized.

@AaronRobinsonMSFT
Copy link
Member

but I don't know if the reduced metadata win is worth it for the reduction in generated code readability (which I feel is a strong component of debuggability)

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 void* that we index into. However, we can do the math for the metadata cost. Let's assume the best case of IInpsectable - 6 function slots including IUnknown (3).

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 void* field so come up with 2 bytes for the field plus whatever the length of the name vtable as UTF-8 is 6 + 1 for null. A total of 9 bytes of metadata for the description of a vtable of any length.

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 a to preserve space. In the case of the IInpsectable we exceed the 9 bytes just by naming the entries - 6 * 2 (character and null) = 12 bytes. We didn't even start to encode the signatures or types and we have already exceeded the constant case above and that is only for the trivial 6 slot type.

I am all for debuggability but those numbers don't add up to me. Especially when there are already concerns about size.

@jkotas
Copy link
Member

jkotas commented Jul 2, 2020

Can you get most of the debuggability benefits by adding comments for the slot numbers, e.g. vtbl[6 /* IMyInterface.MyMethod */] ?

@jkoritzinsky
Copy link
Member

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.

@AaronRobinsonMSFT
Copy link
Member

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.

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);
}

@jkoritzinsky
Copy link
Member

Yeah something similar to that. In your case the function pointer would be delegate* unmanaged<IntPtr, out int, int>, but basically yeah. I personally don't like it but it's not something that I feel super strongly about.

@Scottj1s
Copy link
Member

Scottj1s commented Jul 2, 2020

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:

  • Users define not just the ComImport interface, but the corresponding ComImportImpl - hardly worth the trouble
  • GetInterfaceImplementation uses CreateType to generate the Impl - debuggability/linking concerns as mentioned
  • Users required to use As<> (or some variant - Jeremy suggested AsComImport<> for clarity)

I vote for the last - AsComImport<> cast

@AaronRobinsonMSFT
Copy link
Member

I'm not sure how this discussion veered into function pointers

That was me being difficult - sorry. The conversation can/should be deferred once we start investigating the size concerns.

Yes, runtime classes are all concrete (not parametric - that still only applies to interfaces and delegates).

Thanks for the confirmation. I must be misremembering some previous conversations. I will follow-up offline.

I vote for the last - AsComImport<> cast

Seems reasonable. Is this the fallback logic to the runtime built-in COM support or are these one off support scenarios provided by CsWinRT?

@jkoritzinsky
Copy link
Member

The .AsComImport<> cast method would be the fallback to the built-in COM support.

@AaronRobinsonMSFT
Copy link
Member

The .AsComImport<> cast method would be the fallback to the built-in COM support.

Nice. Can I assume that an analyzer can detect possible misused by checking if a type inherits from IInspectable? For example, if the type doesn't inherit from IInspectable then As<T>() or C-style cast will probably fail and we can recommend to the user to try .AsComImport<T>()? The converse is also possible if we detect it does inherit from IInspectable we can point to using a C-style cast when CSWinRT is involved?

@jkoritzinsky
Copy link
Member

Yes, an analyzer should be able to detect using .AsComImport<T>() incorrectly (with an interface not marked with [ComImport]) and error out and add a code fix to change it to a cast. It is likely that the .As<T>() method will move to being only available on the .NET Standard 2.0 projection (since it's use cases on .NET 5 would be covered by IDynamicInterfaceCastable and .AsComObject<T>() fully).

@reflectronic
Copy link

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 IInitializeWithWindow, and spit out something like

[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);
    }
}

@Scottj1s
Copy link
Member

PR #369

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

No branches or pull requests

9 participants