[Mono-dev] Supporting ICastable on Mono Windows x64.

Wed May 17 11:41:51 UTC 2017

Hi Rodrigo,

Thanks for feedback! I believe I could adopt using the IMT approach instead
of patching the vtable (will simplify the solution). When I tried this in
the past it didn't work out as expected so then I started to look at the
transparent proxy solution, since it seemed to be quite close to what I was
looking for. Based on your feedback I did a quick tests of switching to IMT
together with the rest of ICastable changes + armed with a deeper knowledge
around Mono's implementation of vtable's and type system, and it seems to
work, getting pass on ICastable test suite, + MCG sample. I already had
most code in place, so what I did was to make sure we get a IMT fail tramp
for vtables supporting ICastable (will be the same fail tramp as other
cases, callbacks.get_imt_trampoline). I already have logic
in mono_vcall_trampoline to take care of resolving methods on ICastable
objects and all seems to be working as expected. For methods implemented
directly by a class that also implements ICastable, I already had solution
making sure standard method resolve still kicks in, so the slow path will
only be hit when calling methods not directly implemented by class, doing
call to ICastable::GetImplType to get to the implementation.

When it comes to the broader refactoring idea I think we could do this work
in several steps. First step is to getting ICastable done together with the
rest of changes needed for the MCG use case to work on the platforms we
initial need it. Once all that is working we can move back to do an
overhaul of the type checking and casting unifying it where it makes sense.
We could keep patches at our end until all is done and then upstream, or we
could issue PR's upstream in several steps as we go. One thing I'm a little
worried around is the regression impact refactoring the casting logic. The
ICastable changes are still quite isolated to this use case and won't
change other casting scenarios, but doing the bigger refactoring will of
course open up for more potential problems. I assume we have tests covering
most of the scenarios, special array, interfaces, TP and ones ICastable is
in place, we will have a test suite covering that as well, to mitigate
potential regression problems in areas not hit that frequent. This might be
another reason for doing the change in more than one step, making it easier
to track potential regression problems related to changes.

Regarding ICastable and reflection, as far as I know and have seen, it is
not supported in CoreCLR for ICastable and doesn't seem to be needed by use
cases as MCG. The ICastable test suite in CoreCLR (that I'm using as well)
don't include any tests for reflection in it.

Regarding making ICastable feature mandatory in Mono? CoreCLR guards it's
implementation with a define that needs to be set in order to get ICastable
runtime support, FEATURE_ICASTABLE. If ICastable should be considered a
supported feature in Mono, maybe it could still make sense to be able to
disable it using a define like DISABLE_ICASTABLE?

Thanks,

Johan Lorensson

On Wed, May 17, 2017 at 12:36 AM, Rodrigo Kumpera <rokumper at microsoft.com>
wrote:

> Hi Johan,
>
>
>
> Transparent proxy needs to extend the vtable due to virtual methods.
>
> ICastable (IC), IIRC, doesn't need to, as it's only possible to dispatch
> to interface methods.
>
>
>
> Beyond that, vtable patching has a lot of other complications, it breaks
> invariants such as that two instances
>
> of the same class must have the same vtable pointer - we compare them all
> over the place.
>
>
>
> My suggestion is the following:
>
>
>
> Types implementing IC would force their IMT thunks to have a fail tramp,
> in a similar fashion as its done for other cases.
>
> The imt fail trampoline code would then be adjusted to handle IC. It's a
> much simpler and maintainable change.
>
> That would not require changes to our compilation pipeline.
>
>
>
> The comes the second part, type testing. We should be taking this
> opportunity to clean up the casting code a bit.
>
>
>
> My suggestion is that we use space in the vtable to indicate whether type
> testing has a slow path.
>
> Right now, we have a couple of cases: special array interfaces and TP.
>
>
>
> My suggestion is to unify the TP and the ICastable checks in the common
> path and outline the resolution of those.
>
> We should them experiment applying the same idea to special array
> interfaces and see whether performance numbers hold.
>
>
>
> We'd need to add checks for IC to all places we do today for TP. They
> might require big code changes
>
> as the TP checks don't require calling into managed while IC does. The
> comments in that C# file don't mention impact
>
> on other parts of the runtime, like reflection, which we would need to
> verify as well.
>
>
>
> The above model is how I think TP should be implemented, but that code
> predates IMTs.
>
>
>
> Finally, there's no point in making ICastable an optional feature as it
> should be supported across the board.
>
>
>
> --
>
> Rodrigo
>
>
>
>
>
> *From: *Mono-devel-list <mono-devel-list-bounces at lists.dot.net> on behalf
> of Johan Lorensson <lateralusx.github at gmail.com>
> *Date: *Tuesday, May 16, 2017 at 2:44 AM
> *To: *"mono-devel-list at lists.dot.net" <mono-devel-list at lists.dot.net>
> *Subject: *[Mono-dev] Supporting ICastable on Mono Windows x64.
>
>
>
> Hi,
>
>
>
> Below is a proposed solution to implement support of CoreCLR ICastable
> feature on Mono Windows x64.
>
>
>
> *Why:*
>
>
>
> ICastable is supported by CoreCLR and used by MCG (Microsoft interop
> codegen tooling for WinRT API's) in order to call WinRT API's from managed
> code on Windows platforms. MCG consumes winmd files + a closure of WinRT
> API used by an application generating managed interop code. Supporting
> ICastable is a prerequisite in order to support MCG on Mono Windows x64.
>
>
>
> The requirements on the ICastable feature is described in the interface:
>
>
>
> https://github.com/dotnet/coreclr/blob/68f72dd2587c3365a9fe74d1991f93
> 612c3bc62a/src/mscorlib/src/System/Runtime/CompilerServices/ICastable.cs
> <https://na01.safelinks.protection.outlook.com/?url=https%3A%2F%2Fgithub.com%2Fdotnet%2Fcoreclr%2Fblob%2F68f72dd2587c3365a9fe74d1991f93612c3bc62a%2Fsrc%2Fmscorlib%2Fsrc%2FSystem%2FRuntime%2FCompilerServices%2FICastable.cs&data=02%7C01%7Crokumper%40microsoft.com%7Cd254047ed900449f843e08d49c40321e%7C72f988bf86f141af91ab2d7cd011db47%7C1%7C0%7C636305246941657781&sdata=t8tOm1Cvn9Klm4JwgxF%2BbeH%2Bqk%2B8sZbDOpIF2Q4t4Cc%3D&reserved=0>
>
>
>
> There is also a test case that will be used to validate Mono's
> implementation of ICastable:
>
>
>
> https://github.com/dotnet/coreclr/blob/84aab5f59d023849b9ccdc290e698e
> 4a61ac75a2/tests/src/Interop/ICastable/Castable.cs
> <https://na01.safelinks.protection.outlook.com/?url=https%3A%2F%2Fgithub.com%2Fdotnet%2Fcoreclr%2Fblob%2F84aab5f59d023849b9ccdc290e698e4a61ac75a2%2Ftests%2Fsrc%2FInterop%2FICastable%2FCastable.cs&data=02%7C01%7Crokumper%40microsoft.com%7Cd254047ed900449f843e08d49c40321e%7C72f988bf86f141af91ab2d7cd011db47%7C1%7C0%7C636305246941657781&sdata=srFnEjYYl0MlveNf0dOKlydPiuXnb8Yy44XwVncoNWM%3D&reserved=0>
>
>
>
> The ICastable implementation + additional Mono extensions will also be
> used to run a couple of MCG->WinRT samples testing MCG's usage of Mono's
> ICastable implementation.
>
>
>
> All should work on Windows x64 JIT + full AOT.
>
>
>
> *How:*
>
>
>
> ICastable is a specific feature with several restrictions and constraints.
> The purpose is to dynamically, at runtime, add interfaces and
> implementation to a type that is not part of the types original metadata.
> The added interfaces are then implemented by a complete different unrelated
> type returned by implementation of ICastable interface. That put some
> specific requirements on both the runtime but also on the implementation of
> the class implementing the ICastable interface and its dispatch targets.
> Due to this, the feature should be considered "Private" limiting it's use
> and support to specific use cases. The main initial target is MCG stubs and
> interopability with WinRT API's on Windows platforms.
>
>
>
> *Extending Mono Runtime:*
>
>
>
> Looking into the current Mono source there is currently a feature that has
> similar characteristics and behavior as ICastable, transparent proxy
> support used by remoting. The plan is to use that as inspiration but not
> depending on current implementation (should not need to include remoting
> support to work). The underlying mechanism is still similar, dynamically
> extend an objects vtable if an objects gets casted to an interface
> indirectly supported by the underlying implementation of ICastable (or
> IRemotingTypeInfo.CanCastTo in remoting use case). The idea is to
> generalize and share the code already doing this logic in remoting making
> sure we only include one implementation of key features like rebuilding the
> vtable (currently done in mono_class_proxy_vtable). ICastable
> implementation will also include a cache similar to the one used in the
> remoting use case (see mono_upgrade_remote_class for details).
>
>
>
> All code related to the ICastable feature will be guarded by a defined,
> FEATURE_ICASTABLE, so all platforms not needing this support (currently
> only needed on WinRT platforms), won't be affected by this change.
>
>
>
> *Extending Mono BCL:*
>
>
>
> The ICastable implementation + helper class can be included from CoreCLR
> into Mono's mscorlib with a small adjustment to the helper class:
>
>
>
> https://github.com/dotnet/coreclr/blob/68f72dd2587c3365a9fe74d1991f93
> 612c3bc62a/src/mscorlib/src/System/Runtime/CompilerServices/ICastable.cs
> <https://na01.safelinks.protection.outlook.com/?url=https%3A%2F%2Fgithub.com%2Fdotnet%2Fcoreclr%2Fblob%2F68f72dd2587c3365a9fe74d1991f93612c3bc62a%2Fsrc%2Fmscorlib%2Fsrc%2FSystem%2FRuntime%2FCompilerServices%2FICastable.cs&data=02%7C01%7Crokumper%40microsoft.com%7Cd254047ed900449f843e08d49c40321e%7C72f988bf86f141af91ab2d7cd011db47%7C1%7C0%7C636305246941657781&sdata=t8tOm1Cvn9Klm4JwgxF%2BbeH%2Bqk%2B8sZbDOpIF2Q4t4Cc%3D&reserved=0>
>
>
>
> Part of supporting ICastable includes adding the above assembly and
> classes to Mono's BCL. It should build under net_4_x and winAOT profiles.
>
>
>
> *Examples of things that needs to be done to support ICastable:*
>
>
>
>    - Extend MonoVTable with a field indicating support for ICastable
>    (optimization for fast lookups and checks). Since this field is checked
>    both by C code and generated code (handle_isinst, handle_castclass) it
>    needs to be addressable. Field will also be used to include more meta
>    information when an object implements ICastable and has been casted to
>    different interfaces (needed by both caching and vtable rebuild).
>    - Extend mono_object_handle_isinst_mbyref to have fallback for objects
>    supporting ICastable.
>    - Extend handle_isinst and handle_castclass to have fallback for
>    objects supporting ICastable.
>    - Extend objects vtable with new interfaces implemented by a different
>    type indicated by ICastable::IsInstanceOfInterface. Similar to how
>    mono_upgrade_remote_class works for remote objects.
>    - Add support to mono_vcall_trampoline to resolve method
>    implementation for ICastable objects when not directly implemented by type.
>    Implementing type for method is returned by ICastable::GetImplType.
>    - Cache dynamically generated vtables based on object class +
>    additional supported interface(s).
>    - Add support for CoreCLR ICastable interface and support class in
>    Mono's mscorlib (net_4_x, winAOT profiles).
>
> I have implemented a POC using the above approach and it solve the use
> case, getting pass on all tests in CoreCLR ICastable test suite and working
> with MCG stub samples, so above suggestion seems at least to be a way
> moving forward implementing support for ICastable on Mono Windows x64.
>
>
>
> Comments, thoughts and ideas are much appreciated.
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.dot.net/pipermail/mono-devel-list/attachments/20170517/e44069a5/attachment-0001.html>