Skip to content
This repository was archived by the owner on Jan 23, 2023. It is now read-only.

WIP: Move LowLevelMonitor functions from CoreRT to System.Native#35060

Closed
filipnavara wants to merge 3 commits into
dotnet:masterfrom
filipnavara:monitor1
Closed

WIP: Move LowLevelMonitor functions from CoreRT to System.Native#35060
filipnavara wants to merge 3 commits into
dotnet:masterfrom
filipnavara:monitor1

Conversation

@filipnavara
Copy link
Copy Markdown
Member

@filipnavara filipnavara commented Feb 2, 2019

The functions are renamed, implementation is converted from C++ to C.

Original source:

Two new functions are exposed - SystemNative_MonitorTryAcquire and SystemNative_MonitorBroadcastAndRelease. Both were implemented in CoreRT native code, but they were never used. They will likely not be necessary, but I added them in the off-chance that they would be used since the implementation is small and trivial.

The need for SystemNative_MonitorTryAcquire is avoided by keeping the track of locked state in the managed code and using Interlocked methods to handle it there. SystemNative_MonitorBroadcastAndRelease is similar to Monitor.PulseAll, which is not exposed in the LowLevelMonitor API.

Naming is not necessarily final, suggestions are welcome.

@stephentoub
Copy link
Copy Markdown
Member

Two new functions are exposed - SystemNative_MonitorTryAcquire and SystemNative_MonitorBroadcastAndRelease. Both were implemented in CoreRT native code, but they were never used. They will likely not be necessary, but I added them in the off-chance that they would be used since the implementation is small and trivial.

Let's only add them when we know we need them. Thanks.

@stephentoub
Copy link
Copy Markdown
Member

What's going to use all of this? Something that's going to be added to corefx? If so, can we add the things using them at the same time?

@filipnavara
Copy link
Copy Markdown
Member Author

filipnavara commented Feb 2, 2019

Let's only add them when we know we need them.

Ok, removed them.

What's going to use all of this?

CoreRT implementation of LowLevelMonitor (https://github.com/dotnet/corert/blob/master/src/System.Private.CoreLib/src/System/Threading/LowLevelMonitor.cs). Possibly Mono if the file ends up being moved to the shared partition. It's my plan to do that eventually to share the ThreadPool implementation, but I am still couple of steps away from that.

Based on this guidance: dotnet/corert#6880 (comment)

@filipnavara
Copy link
Copy Markdown
Member Author

filipnavara commented Feb 2, 2019

I started removing the CoreRT native functions in System.Private.CoreLib.Native that were already moved or independently implemented in System.Native (dotnet/corert#6932, #35031).
Aside from the ones included in this PR the remaining ones in CoreRT are the following:

  • CoreLibNative_LoadLibrary, CoreLibNative_LoadLibrary, CoreLibNative_FreeLibrary (used by internal InteropHelpers, but at some point it may end up backing NativeLibrary APIs, which may be sharable with Mono; also, CoreFX already binds some of these APIs to managed code by linking directly to Libdl)
  • CoreLibNative_GetEnv, CoreLibNative_Exit, CoreLibNative_GetEnviron (used by System.Environment implementation; also could be used by Mono if the CoreRT implementation is taken)
  • CoreLibNative_GetErrNo, CoreLibNative_ClearErrNo
  • CoreLibNative_GetExecutableAbsolutePath
  • CoreLibNative_MemAlloc, CoreLibNative_MemAllocWithZeroInitialize, CoreLibNative_MemReAlloc, CoreLibNative_MemFree, CoreLibNative_MemSet (MemSet already exists in System.Native; they back some of the Marshal allocation APIs in CoreRT which are now being moved by @marek-safar to shared CoreLib partition, so maybe there would be case for moving them too)
  • CoreLibNative_RuntimeThread_CreateThread, CoreLibNative_SchedGetCpu (used by RuntimeThread implementation)
  • CoreLibNative_GetTickCount64 (could be replaced with existing APIs in System.Native, but would lose a performance optimization of using CLOCK_MONOTONIC_COARSE; moving it as-is could reduce duplicated code)

It may be time to rethink whether to keep System.Private.CoreLib.Native at all or absorb the few remaining functions into System.Native. If this PR goes through and the first two items on the list get moved too (based on the assumption that they would be usable by Mono, which may or may not be correct) then the remaining API surface gets really small.

@filipnavara
Copy link
Copy Markdown
Member Author

If so, can we add the things using them at the same time?

I am very uneasy about adding things to a repository that are not directly used + tested by the CI. In this case I am not sure how to accomplish that. I am not going to drop the "WIP" tag until I either find a way to do that or submit the counter-part CoreRT code that is locally tested.

@jkotas
Copy link
Copy Markdown
Member

jkotas commented Feb 3, 2019

keep System.Private.CoreLib.Native at all or absorb the few remaining functions into System.Native

The other option is to absorb some of System.Private.CoreLib.Native functions into the unmanaged runtime - in cases where the unmanaged runtime has to have them for its own purpose anyway, or when the methods are coupled more with the runtime than with the framework. Good examples of these are CoreLibNative_RuntimeThread_CreateThread, CoreLibNative_GetTickCount64 or CoreLibNative_GetExecutableAbsolutePath.

@filipnavara
Copy link
Copy Markdown
Member Author

If you think the removal of System.Private.CoreLib.Native is worth exploring I can open an issue for that for discussion. I originally didn't want to pursue it, but there are various reasons that may make it worth doing. I started hitting places where the code is useful for sharing with Mono and looking at the remaining functions it seems that it will come up again. Then there's the fact that the library still unnecessarily links to the C++ runtime library which can be addressed at the same time as part of clean-up. I agree there are various ways to pursue it and I am not sure which one is the best one.

@filipnavara
Copy link
Copy Markdown
Member Author

Closing for now. I will reopen it once I asses all the dependencies of the CoreRT thread pool code and come up with a plan to resolve them.

@karelz karelz added this to the 3.0 milestone Mar 18, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants