[NativeAOT] Remove ThunkPool crst.

[VSadov]

We can use a regular lock in ThunkPool instead of pInvoking a Crst from the native runtime. The only thing that was stopping us was that ThunkPool is in the Base library and Test.CorLib did not have support for lock statement.

But, since Base and Test.CorLib themselves do not use ThunkPool, we can move the thunkpool to the full Corelib where we can use regular locks.

[ghost]

Tagging subscribers to this area: @agocke, @MichalStrehovsky, @jkotas
See info in area-owners.md if you want to be subscribed.

Issue Details

---

We can use a regular lock in ThunkPool instead of pInvoking a Crst from the native runtime. The only thing that was stopping us was that ThunkPool is in the Base library and Test.CorLib did not have support for lock statement.
But, since Test.CorLib does not support threading, it can have a very simple no-op Monitor implementation.

We can also assume that any CoreLib that supports threading will also have support for lock.

Author:	VSadov
Assignees:	VSadov
Labels:	area-NativeAOT-coreclr
Milestone:	-

[SingleAccretion]

This is nominally Runtime.Base code. Using a CoreLib-based lock seems like it violates layering.

[MichalStrehovsky]

This is nominally Runtime.Base code. Using a CoreLib-based lock seems like it violates layering.

Yup, we'd need a Monitor class in Runtime.Base for this to keep Runtime.Base buildable as a standalone library. We don't build it because Jan didn't want me to because Test.CoreLib kind of guards the layering. But it makes it non-obvious that this "poor man's lock" is going to be the implementation of locking in the shipping runtime if/once we restore the .NET Native layering.

[MichalStrehovsky]

Now that I'm looking at it, we also added a no-op cast cache to Test.CoreLib that has also the same kind of problem:

runtime/src/coreclr/nativeaot/Test.CoreLib/src/System/Runtime/CompilerServices/CastCache.cs

Line 11 in a23c7a1

internal static unsafe class CastCache

The code we write for inclusion in Test.CoreLib needs to be shippable.

I think we should start building Runtime.Base to make it 100% clear this needs to be shippable code, it's not just test code.

[VSadov]

Yup, we'd need a Monitor class in Runtime.Base

I do not think this could work for the complete Monitor implementation. Monitor must live in a Core Library, at least because it allocates things.

[VSadov]

As we discussed with Michal this needs more thought.
First of all we can't guarantee there is only one thread as threads can attach via reverse pinvoke. The lock we use can be something simpler than lock, but needs to be functional (in terms of providing mutual exclusion to threads)

I'll probably close this, if there is no simple enough solution.

[jkotas]

it makes it non-obvious that this "poor man's lock" is going to be the implementation of locking in the shipping runtime if/once we restore the .NET Native layering.

The .NET Native layering was not great. It was driven by:

• Attempt to hide all type system implementation details. EEType was not visible outside the shared piece in the original design. Everything had to go through a helper. It was not very practical and it came with large performance overhead.
• Implementation language. All C/C++ must have been in the shared piece. There was no provision for having C/C++ in the non-shared piece (kind of like we have with System.Native and friends today).

If we were to do this again, we would want to look at the principled approach. For example, define the shared piece as GC and thread suspension only and everything else to be app-local. A litmus test for well-designed shared piece is whether it can be usable by both regular CoreCLR and native AOT running in the same process.

With this in mind, I think it would ok to move the thunk allocator out of Runtime.Base. I do not think we would want to have the thunk allocator in the shared piece if we were to attempt to build it again.

[MichalStrehovsky]

With this in mind, I think it would ok to move the thunk allocator out of Runtime.Base. I do not think we would want to have the thunk allocator in the shared piece if we were to attempt to build it again.

That could work too. From a quick look it doesn't have to live in Runtime.Base. It was probably just convenient to put it there because the native/assembly part of it has to live somewhere and we have Runtime.WorkstationGC/ServerGC. If we pull the managed part into CoreLib and the unmanaged part into a separate lib file, that would work. We had a System.Private.TypeLoader.Native lib file for "native stuff System.Private.TypeLoader needs to call into". We can have a similar System.Private.CoreLib.Native lib file with "native stuff CoreLib needs to call into" (it doesn't feel right to stash it into libSystem.Native, the main reason being that we don't build it on Windows).

[VSadov]

If we were to do this again, we would want to look at the principled approach. For example, define the shared piece as GC and thread suspension only and everything else to be app-local. A litmus test for well-designed shared piece is whether it can be usable by both regular CoreCLR and native AOT running in the same process.

That makes sense. I just wonder where that puts Test.CoreLib?
I kind of always assumed it to be a minimal/sample CoreLib implementation and the minimal API was driven by the needs of Base. (since Base has managed code, it needs some kind of CoreLib)

[jkotas]

Test.CoreLib is minimum viable CoreLib for test purposes. It should be correct, but it does not need to be complete or high performance. For example, if it needed locks, I think it would be ok to implement them as simple spin locks.

[MichalStrehovsky]

Iirc the thunk pools are only needed for delegate marshaling. Test.corelib already doesn't have that.

[VSadov]

Iirc the thunk pools are only needed for delegate marshaling. Test.corelib already doesn't have that.

yes, it would make it technically ok to have a lock that just throws NotImplementedException, but that feels like defeating the intent of having Test.CoreLib

[jkotas]

The idea is to have the thunkpool in the full System.Private.CoreLib only, and omit them in runtime base and Test.CoreLib. There is no throwing implementation of the lock in this plan.

We do not need thunkpool in Test.CoreLib. It is not required for testing core runtime.

[VSadov]

I have moved the thunkpool to the full System.Private.CoreLib. It did make things simpler.

[jkotas]

Thanks

[VSadov]

Thanks!!

[MichalStrehovsky]

We don't need to move the C++/assembly pieces (RhAllocateThunksMapping, etc.) into a separate library that is "owned" by CoreLib instead of the runtime?

[jkotas]

We can start "native stuff CoreLib needs to call into" library. Ideally, it would be shared by all runtimes, or at least by regular CoreCLR and NAOT. ThunkPools are not the only CoreLib supporting part that lives in runtime just because it was convenient to put it there.

[VSadov]

Would that result in every compilation unit having a separate thunk pool?

[jkotas]

Every compilation unit has its own thunk pool after this PR. It is just like any other CoreLib code, every compilation unit gets its own copy.

Every compilation would get its own copy of the thunk pool helpers with the proposed change. It is just a drop in the bucket.