Hi Johan, On the refactoring bit, my reasoning for it is to pay some of our technical debt in that area as we go, spending time on that part of the codebase makes you the honorary expect on it ;) I’m onboard on the specifics of how you want to approach.
There are two reasons for us to make a certain feature optional on mono. It’s either because we can’t support it on a specific configuration (SRE without a JIT) or because the benefit/cost ratio is horrible (System.Configuration on mobile). In the case of ICastable, by design we’ll have ubiquitous support and the cost of having it across the board is minimal (a slower slow path and little code/footprint we could eliminate). To sum up, I don’t see us disabling ICastable on any of Xamarin’s targets, so we gain nothing by making it optional. -- Rodrigo From: Johan Lorensson <lateralusx.git...@gmail.com> Date: Wednesday, May 17, 2017 at 4:41 AM To: Rodrigo Kumpera <rokum...@microsoft.com> Cc: "mono-devel-list@lists.dot.net" <mono-devel-list@lists.dot.net> Subject: Re: [Mono-dev] Supporting ICastable on Mono Windows x64. 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 <rokum...@microsoft.com<mailto:rokum...@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-boun...@lists.dot.net<mailto:mono-devel-list-boun...@lists.dot.net>> on behalf of Johan Lorensson <lateralusx.git...@gmail.com<mailto:lateralusx.git...@gmail.com>> Date: Tuesday, May 16, 2017 at 2:44 AM To: "mono-devel-list@lists.dot.net<mailto:mono-devel-list@lists.dot.net>" <mono-devel-list@lists.dot.net<mailto:mono-devel-list@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/68f72dd2587c3365a9fe74d1991f93612c3bc62a/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/84aab5f59d023849b9ccdc290e698e4a61ac75a2/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/68f72dd2587c3365a9fe74d1991f93612c3bc62a/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.
_______________________________________________ Mono-devel-list mailing list Mono-devel-list@lists.dot.net http://lists.dot.net/mailman/listinfo/mono-devel-list
