Runtime: Moving more code to shared partition

[marek-safar]

It'd be very useful to clean up and move more of the following types to shared partition for easier sharing with other runtimes.

In no particular order

• [x] [AsyncVoidMethodBuilder](https://github.com/dotnet/coreclr/blob/master/src/System.Private.CoreLib/src/System/Runtime/CompilerServices/AsyncMethodBuilder.cs)
• [x] [AsyncTaskMethodBuilder](https://github.com/dotnet/coreclr/blob/master/src/System.Private.CoreLib/src/System/Runtime/CompilerServices/AsyncMethodBuilder.cs)
• [x] [AsyncTaskMethodBuilder](https://github.com/dotnet/coreclr/blob/master/src/System.Private.CoreLib/src/System/Runtime/CompilerServices/AsyncMethodBuilder.cs)
• [x] [Contract](https://github.com/dotnet/coreclr/blob/master/src/System.Private.CoreLib/src/System/Diagnostics/Contracts/ContractsBCL.cs)
• [x] [ContractFailedEventArgs](https://github.com/dotnet/coreclr/blob/master/src/System.Private.CoreLib/src/System/Diagnostics/Contracts/ContractsBCL.cs)
• [x] [ContractException](https://github.com/dotnet/coreclr/blob/master/src/System.Private.CoreLib/src/System/Diagnostics/Contracts/ContractsBCL.cs)
• [x] [ContractHelper](https://github.com/dotnet/coreclr/blob/master/src/System.Private.CoreLib/src/System/Diagnostics/Contracts/ContractsBCL.cs)
• [x] [TaskFactory](https://github.com/dotnet/coreclr/blob/master/src/System.Private.CoreLib/src/System/Threading/Tasks/FutureFactory.cs)
• [x] [TaskStatus](https://github.com/dotnet/coreclr/blob/master/src/System.Private.CoreLib/src/System/Threading/Tasks/Task.cs)
• [x] [Task](https://github.com/dotnet/coreclr/blob/master/src/System.Private.CoreLib/src/System/Threading/Tasks/Task.cs)
• [x] [TaskFactory](https://github.com/dotnet/coreclr/blob/master/src/System.Private.CoreLib/src/System/Threading/Tasks/TaskFactory.cs)
• [x] [GCSettings](https://github.com/dotnet/coreclr/blob/master/src/System.Private.CoreLib/src/System/Runtime/GcSettings.cs)
• [x] [Timer](https://github.com/dotnet/coreclr/blob/master/src/System.Private.CoreLib/src/System/Threading/Timer.cs)
• [x] [TimerCallback](https://github.com/dotnet/coreclr/blob/master/src/System.Private.CoreLib/src/System/Threading/Timer.cs)
• [x] [TimerHolder](https://github.com/dotnet/coreclr/blob/master/src/System.Private.CoreLib/src/System/Threading/Timer.cs)
• [x] [TimerQueueTimer](https://github.com/dotnet/coreclr/blob/master/src/System.Private.CoreLib/src/System/Threading/Timer.cs)
• [x] [ResourceManager](https://github.com/dotnet/coreclr/blob/master/src/System.Private.CoreLib/src/System/Resources/ResourceManager.cs)
• [x] [Marshal](https://github.com/dotnet/coreclr/blob/master/src/System.Private.CoreLib/src/System/Runtime/InteropServices/Marshal.cs)
• [x] [SynchronizationContext](https://github.com/dotnet/coreclr/blob/master/src/System.Private.CoreLib/src/System/Threading/SynchronizationContext.cs)
• [x] [EnvironmentVariableTarget](https://github.com/dotnet/coreclr/blob/master/src/System.Private.CoreLib/src/System/Environment.cs)
• [x] [Environment](https://github.com/dotnet/coreclr/blob/master/src/System.Private.CoreLib/src/System/Environment.cs)
• [x] [GCHandleType](https://github.com/dotnet/coreclr/blob/master/src/System.Private.CoreLib/src/System/Runtime/InteropServices/GcHandle.cs)
• [x] [GCHandle](https://github.com/dotnet/coreclr/blob/master/src/System.Private.CoreLib/src/System/Runtime/InteropServices/GcHandle.cs)
• [x] [Array](https://github.com/dotnet/coreclr/blob/master/src/System.Private.CoreLib/src/System/Array.cs)
• [x] [CustomAttributeNamedArgument](https://github.com/dotnet/coreclr/blob/master/src/System.Private.CoreLib/src/System/Reflection/CustomAttribute.cs)
• [x] [CustomAttributeTypedArgument](https://github.com/dotnet/coreclr/blob/master/src/System.Private.CoreLib/src/System/Reflection/CustomAttribute.cs)
• [x] [CustomAttributeExtensions](https://github.com/dotnet/coreclr/blob/master/src/System.Private.CoreLib/src/System/Reflection/CustomAttributeExtensions.cs)
• [ ] [CustomAttributeData](https://github.com/dotnet/coreclr/blob/master/src/System.Private.CoreLib/src/System/Reflection/CustomAttribute.cs)
• [x] [AssemblyName](https://github.com/dotnet/coreclr/blob/master/src/System.Private.CoreLib/src/System/Reflection/AssemblyName.cs)
• [x] [TypeLoadException](https://github.com/dotnet/coreclr/blob/master/src/System.Private.CoreLib/src/System/TypeLoadException.cs)
• [x] [Enum](https://github.com/dotnet/coreclr/blob/master/src/System.Private.CoreLib/src/System/Enum.cs)
• [x] [Buffer](https://github.com/dotnet/coreclr/blob/master/src/System.Private.CoreLib/src/System/Buffer.cs)
• [x] [Exception](https://github.com/dotnet/coreclr/blob/master/src/System.Private.CoreLib/src/System/Exception.cs)
• [x] [Activator](https://github.com/dotnet/coreclr/blob/master/src/System.Private.CoreLib/src/System/Activator.cs)
• [x] [WaitHandle](https://github.com/dotnet/coreclr/blob/master/src/System.Private.CoreLib/src/System/Threading/WaitHandle.cs)
• [ ] [TypedReference](https://github.com/dotnet/coreclr/blob/master/src/System.Private.CoreLib/src/System/TypedReference.cs)
• [x] [SafeHandle](https://github.com/dotnet/coreclr/blob/master/src/System.Private.CoreLib/src/System/Runtime/InteropServices/SafeHandle.cs)
• [x] [WeakReference](https://github.com/dotnet/coreclr/blob/master/src/System.Private.CoreLib/src/System/WeakReference.cs)

Note: It's useful to check https://github.com/dotnet/corert for different implementation of the types before doing the work in this repo as some of the types have been improved or cleaned up there already.

[marek-safar]

/cc @jkotas @stephentoub @filipnavara

[filipnavara]

I'm working on ResourceManager (and related classes) at the moment. It's relatively easy, but I want to clean-up the CoreRT part and test the APPX code before submitting a PR.

[danmosemsft]

What in your mind determines whether something cannot be ported to shared? Originally it was (generally speaking) that it did direct calls into the runtime, or vice versa (or runtime had special knowledge of its shape). But some of the things above call directly into the runtime, right?

[filipnavara]

But some of the things above call directly into the runtime, right?

Some of them do, but in many cases there are significant parts that could be shared as partial class. I admit I didn't go through all of them, so I cannot judge whether all of them make sense. I know you made the effort to open other issues for the sharing (https://github.com/dotnet/coreclr/issues/9474 & linked), so this list is far from definitive or comprehensive.

[marek-safar]

@danmosemsft I think all code should start in shared and then you ask a question what portion of it has to be runtime specific. If that portion would make a significant chunk of the file/implementation then perhaps it's better to keep it in non-shareable partition fully otherwise I think we should have it in shared even if it's not a full implementation

[danmosemsft]

Got it, makes sense, I think it's great that you are finding more that can be shared.

[filipnavara]

Another easy one is MemoryFailPoint. It is nearly identical in CoreCLR and CoreRT except for the different implementation of GetMemorySettings (internal call on CoreCLR, code from constructor and static variable assignment in CoreRT).

[marek-safar]

@filipnavara I agree just not sure if the type is useful (it has minimal public surface)

[filipnavara]

@marek-safar Actually there's many more of such things in System.Runtime. Many of them have much larger footprint (eg. ConditionalWeakTable is > 40Kb). I'll try to make a list or just submit PRs for the easy ones. For the harder ones (Marshal.cs) there is a lot of code diversion between CoreRT/CoreCLR, but it may still be worth investigating since the actual method implementations often still do exactly the same thing. There's already an issue tracking some of them (https://github.com/dotnet/coreclr/issues/17904), but the list is incomplete.

Update: There's only around 3-4 classes that could be shared in straightforward way. I will just submit PRs.

[filipnavara]

Btw, any opinions on ThreadPool? I would like to move the common code and the portable implementation from CoreRT to shared partition (along with the FeaturePortableThreadPool flag).

[marek-safar]

@filipnavara I'd love to see TP shared but it could take few rounds to get us there

[filipnavara]

I agree that it may take a couple of rounds, but the part in ThreadPool.cs is nearly the same in CoreRT and CoreCLR save for some logging and small things that diverged over time. I'd start by extracting the CoreCLR specific parts into ThreadPool.CoreCLR.cs and moving ThreadPool.cs to shared partition. As long as the division between files stays the same as in CoreRT nowadays (which is very likely) the CoreRT implementation can basically be moved to shared as-is. Actually switching CoreCLR to use it is another thing, which will require more effort, testing and benchmarks, but it doesn't necessarily have to block Mono from using the CoreRT code early.

[filipnavara]

I'll work on Timer once the portable ThreadPool lands (currently blocked by CoreFX packages not being published to MyGet). The implementation in CoreRT looks like a cleaned up version of CoreCLR code with some bits moved to managed code. In fact I think Timer.Unix.cs should be renamed to Timer.Portable.cs since it has no Unix specific code.

[stephentoub]

The implementation in CoreRT looks like a cleaned up version of CoreCLR code with some bits moved to managed code.

There have been several performance improvements in coreclr that I don't believe are in corert.

[filipnavara]

There have been several performance improvements in coreclr that I don't believe are in corert.

I will look into that and do some benchmarks. I don't have a plan yet for how to land it, but I may try to do the same thing as with ThreadPool where common code and portable implementation is moved to the shared partition. The portable implementation would be under Feature flag and it would be used in CoreRT/Unix and Mono. CoreCLR would keep the current implementation for now.

[stephentoub]

Examples:
https://github.com/dotnet/coreclr/commit/cfe51dd00e069e9fcbee6843e0eee2e80c9b3bd1
https://github.com/dotnet/coreclr/commit/4b201c5d956abc1ec5ff5e7c60f87138698ce78c
https://github.com/dotnet/coreclr/commit/6279aee1a2249e301b33e7d0fbaded42e041f100
https://github.com/dotnet/coreclr/commit/d9732f493c359b404bd5b45117f4d211148c591a

I would rather any timer code start with what's in coreclr to ensure we don't regress here. Some of these changes were important enough to be ported to netfx.
cc: @kouvel

[filipnavara]

Thanks much! These PRs largely affect the common code part, so it just means that the Timer.cs in CoreCLR is better baseline. Only one of them touched the unmanaged code and it looks like it was due to related interface changes and not due to performance issue.

[stephentoub]

Great, thanks.

[marek-safar]

@filipnavara thank you very much for looking into that

[stephentoub]

it was due to related interface changes and not due to performance issue

Yeah, as part of a perf change that resulted in multiple native timers, we needed to pass an id from managed to native so that it could be later passed back to identify which native timer was firing.

Eventually it could be nice to switch to an entirely managed implementation. But that should be done separately from this shared work and heavily scrutinized.

Thanks!

[filipnavara]

PRs for the Timer submitted. Hopefully I didn't break any build. I assume I will get some feedback about variable naming and there is still one function that I think should be moved to the common code, but it's not ready yet.

Eventually it could be nice to switch to an entirely managed implementation. But that should be done separately from this shared work and heavily scrutinized.

Agreed completely. It will be relatively easy to switch the implementation. The hard part is benchmarking and addressing any possible regressions.

[filipnavara]

@marek-safar, @jkotas Any opinions on unifying [Event]WaitHandle methods? The Timer and ThreadPool each have one place that needs SetEvent-like API on WaitHandle. I am tempted to just cast to EventWaitHandle, but for compatibility reasons that's really not an option on Windows where the WaitHandle can be created directly from native handles. Currently there's no unified way of doing it. CoreCLR exposes Interop.Windows.Kernel32 on Unix that calls to PAL layer in unmanaged code. Mono historically had a similar approach, but it was in a class named NativeEventCalls. CoreRT has #if mix of Interop.Windows.Kernel32 (on Windows) and WaitSubsystem (on Unix).

[jkotas]

Add a internal static method to EventWaitHandle.Windows.cs/EventWaitHandle.Unix.cs that ThreadPool calls to avoid the ifdef?

[filipnavara]

JFYI planning to look into WaitHandle.

[danmosemsft]

Moving to Future as (although work continues) there is nothing here that must happen for the 3.0 product

[jkotas]

A lot of these are done already. If there is anything left worth doing, it should be tracked by dedicated issues / PRs.

[danmosemsft]

Well hooray for everyone that contributed to all this work. We are now at 82% shared (Jan's original guesstimate was that 75% was possible, I think). 190KLOC shared, 42KLOC not.