[bcl][jit] implement Interlocked.Exchange<T> in terms of object #17341

lambdageek · 2019-10-15T20:57:17Z

Exchange<T> is supposed to be picked up as an intrinsic. But if it isn't (until #17338 is merged),
since T has a class constraint, call the Exchange (ref object, ref object, ref object) overload instead.

This works around a limitation in the JIT:

mono/mono/mini/mini-trampolines.c

Lines 704 to 713 in 82a0727

    
           if (plt_entry) { 
        
           	if (generic_shared) { 
        
           		target_ji = 
        
           			mini_jit_info_table_find (mono_domain_get (), (char *)mono_get_addr_from_ftnptr (compiled_method), NULL); 
        
           		if (!ji) 
        
           			ji = mini_jit_info_table_find (mono_domain_get (), (char*)code, NULL); 
        
           		if (ji && ji->has_generic_jit_info) { 
        
           			if (target_ji && !target_ji->has_generic_jit_info) { 
        
           				no_patch = TRUE;

if we're in the common call trampoline, and the caller is generic method and the callee is a generic method, if we're using generic sharing for the caller, but there's no generic jit info for the callee, we will not patch the call site.

In this case we had Exchange<T> calling Exchange_T<T>
where the callee is an icall (and evidently didn't have generic jit info).

So every time we called Exchange<T>, we would go through the trampoline when trying to call Exchange_T<T> without patching the call site.

Addresses part of #17334

lambdageek · 2019-10-15T20:59:14Z

H/T to @lewurm who did the hard work of identifying the cause.

Exchange<T> is supposed to be picked up as an intrinsic. But if it isn't, since T has a class constraint, call the `Exchange (ref object, ref object, ref object)` overload instead. This works around a limitation in the JIT: https://github.com/mono/mono/blob/82a07273c22b996b08fa88ee2e6632d782ac2242/mono/mini/mini-trampolines.c#L704-L713 if we're in the common call trampoline, and the caller is generic method and the callee is a generic method, if we're using generic sharing for the caller, but there's no generic jit info for the callee, we will not patch the call site. In this case we had Exchange<T> calling Exchange_T<T> where the callee is an icall (and evidently didn't have generic jit info). So every time we called Exchange<T>, we would go through the trampoline when trying to call Exchange_T<T> without patching the call site. Addresses part of mono#17334

lambdageek · 2019-10-15T21:03:03Z

@monojenkins build deb with monolite

lambdageek · 2019-10-15T21:03:27Z

I should also get rid of CompareExchange_T<T> for the same reason, actually

lambdageek · 2019-10-15T21:07:52Z

@monojenkins build deb with monolite

vargaz · 2019-10-15T21:26:17Z

Not sure this will work:
src/System.Threading/Interlocked.cs(94,25): error CS1503: Argument 1: cannot convert from 'ref T' to 'ref object' [/Users/vargaz/git/netcore/netcore/System.Private.CoreLib/System.Private.CoreLib.csproj]

vargaz · 2019-10-15T21:26:33Z

It might need some Unsafe method usage.

jaykrell · 2019-10-15T21:27:00Z

Did I break it when I did coop conversion?

jaykrell · 2019-10-15T21:27:35Z

I don't think so, since I thought I preserved the layering. Only change should be, when it wasn't inlined/intrinsic. Let me double check..

lambdageek · 2019-10-15T21:34:50Z

Yea this doesn't work.
@vargaz I don't think there's an Unsafe method that does this.

@jaykrell the issue is that there's an extra generic method->generic icall callsite now. And it's hitting a condition where the trampoline code is deciding whether it can patch that callsite. Separately, it happens that Exchange<T> is an intrinsic that isn't being intrinsifies correctly, and that's exposing this second issue. See #17335

vargaz · 2019-10-15T21:35:11Z

I.e.:
CompareExchange (ref Unsafe.As<T, object> (ref location1), ref Unsafe.As<T, object> (ref value), ref Unsafe.As<T, object> (ref comparand), ref Unsafe.As<T, object> (ref result));

lambdageek · 2019-10-15T21:37:18Z

@vargaz thanks!

vargaz · 2019-10-15T21:42:18Z

Should probably add some [Intrinsic] attributes as well.

jaykrell · 2019-10-15T21:48:35Z

So, it wasn't being intrinsified. Not my fault. A bit slow.
But then, when I changed it, I also broke patching, making it much slower.
Both are fixable.
?

Mark the non-icall methods that we expect the JIT to treet as intrinsics with [Intrinsic]

jaykrell · 2019-10-15T21:49:32Z

mono/mini/aot-runtime.c

@@ -4810,34 +4810,14 @@ mono_aot_get_method (MonoDomain *domain, MonoMethod *method, MonoError *error)
 		gboolean volatil = FALSE;
 		MonoMethodSignature *sig;

-		/* Same for CompareExchange<T> and Exchange<T> */
 		/* Same for Volatile.Read<T>/Write<T> */


Maybe similar to do here?

Oh yea that looks like dead code. We already have method bodies like this.

In framework Mono:

mono/mcs/class/corlib/System.Threading/Volatile.cs

Lines 166 to 172 in 73ae9db

[Intrinsic]

public static T Read<T>(ref T location) where T : class =>

Unsafe.As<T>(Unsafe.As<T, VolatileObject>(ref location).Value);

[Intrinsic]

public static void Write<T>(ref T location, T value) where T : class =>

Unsafe.As<T, VolatileObject>(ref location).Value = value;

and in netcore Mono:

mono/netcore/System.Private.CoreLib/shared/System/Threading/Volatile.cs

Lines 220 to 229 in 73ae9db

[Intrinsic]

[NonVersionable]

[return: NotNullIfNotNull("location")]

public static T Read<T>(ref T location) where T : class? =>

Unsafe.As<T>(Unsafe.As<T, VolatileObject>(ref location).Value);

[Intrinsic]

[NonVersionable]

public static void Write<T>([NotNullIfNotNull("value")] ref T location, T value) where T : class? =>

Unsafe.As<T, VolatileObject>(ref location).Value = value;

lambdageek · 2019-10-15T21:58:51Z

Does comparand need it?

Could do:

   object v = value;
   object c = comparand;
   Exchange (...., ref v, ref c, ...);

Both of those methods are implemented as calls to the object overload of Volatile.Read/Write

lambdageek · 2019-10-15T22:01:52Z

@monojenkins build deb with monolite

vargaz · 2019-10-16T01:03:11Z

@monojenkins build failed

lambdageek · 2019-10-16T02:19:15Z

@monojenkins build

vargaz · 2019-10-16T04:53:52Z

The netcore build is missing a using:
https://dev.azure.com/dnceng/public/_build/results?buildId=388963

netcore/System.Private.CoreLib/src/System.Threading/Interlocked.cs

jaykrell · 2019-10-16T09:40:30Z

netcore errors.

  Restore completed in 337.08 ms for /home/vsts/work/1/s/netcore/System.Private.CoreLib/System.Private.CoreLib.csproj.
/home/vsts/.nuget/packages/microsoft.sourcelink.common/1.0.0-beta2-19367-01/build/Microsoft.SourceLink.Common.targets(50,5): warning : Source control information is not available - the generated source link is empty. [/home/vsts/work/1/s/netcore/System.Private.CoreLib/System.Private.CoreLib.csproj]
src/System.Threading/Interlocked.cs(94,25): error CS0103: The name 'Unsafe' does not exist in the current context [/home/vsts/work/1/s/netcore/System.Private.CoreLib/System.Private.CoreLib.csproj]
src/System.Threading/Interlocked.cs(94,68): error CS0103: The name 'Unsafe' does not exist in the current context [/home/vsts/work/1/s/netcore/System.Private.CoreLib/System.Private.CoreLib.csproj]
src/System.Threading/Interlocked.cs(94,106): error CS0103: The name 'Unsafe' does not exist in the current context [/home/vsts/work/1/s/netcore/System.Private.CoreLib/System.Private.CoreLib.csproj]
src/System.Threading/Interlocked.cs(94,148): error CS0103: The name 'Unsafe' does not exist in the current context [/home/vsts/work/1/s/netcore/System.Private.CoreLib/System.Private.CoreLib.csproj]
src/System.Threading/Interlocked.cs(119,18): error CS0103: The name 'Unsafe' does not exist in the current context [/home/vsts/work/1/s/netcore/System.Private.CoreLib/System.Private.CoreLib.csproj]
src/System.Threading/Interlocked.cs(119,59): error CS0103: The name 'Unsafe' does not exist in the current context [/home/vsts/work/1/s/netcore/System.Private.CoreLib/System.Private.CoreLib.csproj]
src/System.Threading/Interlocked.cs(119,97): error CS0103: The name 'Unsafe' does not exist in the current context [/home/vsts/work/1/s/netcore/System.Private.CoreLib/System.Private.CoreLib.csproj]

Build FAILED.

/home/vsts/.nuget/packages/microsoft.sourcelink.common/1.0.0-beta2-19367-01/build/Microsoft.SourceLink.Common.targets(50,5): warning : Source control information is not available - the generated source link is empty. [/home/vsts/work/1/s/netcore/System.Private.CoreLib/System.Private.CoreLib.csproj]
src/System.Threading/Interlocked.cs(94,25): error CS0103: The name 'Unsafe' does not exist in the current context [/home/vsts/work/1/s/netcore/System.Private.CoreLib/System.Private.CoreLib.csproj]
src/System.Threading/Interlocked.cs(94,68): error CS0103: The name 'Unsafe' does not exist in the current context [/home/vsts/work/1/s/netcore/System.Private.CoreLib/System.Private.CoreLib.csproj]
src/System.Threading/Interlocked.cs(94,106): error CS0103: The name 'Unsafe' does not exist in the current context [/home/vsts/work/1/s/netcore/System.Private.CoreLib/System.Private.CoreLib.csproj]
src/System.Threading/Interlocked.cs(94,148): error CS0103: The name 'Unsafe' does not exist in the current context [/home/vsts/work/1/s/netcore/System.Private.CoreLib/System.Private.CoreLib.csproj]
src/System.Threading/Interlocked.cs(119,18): error CS0103: The name 'Unsafe' does not exist in the current context [/home/vsts/work/1/s/netcore/System.Private.CoreLib/System.Private.CoreLib.csproj]
src/System.Threading/Interlocked.cs(119,59): error CS0103: The name 'Unsafe' does not exist in the current context [/home/vsts/work/1/s/netcore/System.Private.CoreLib/System.Private.CoreLib.csproj]
src/System.Threading/Interlocked.cs(119,97): error CS0103: The name 'Unsafe' does not exist in the current context [/home/vsts/work/1/s/netcore/System.Private.CoreLib/System.Private.CoreLib.csproj]
    1 Warning(s)

In mono#17341 we change the caller of CompareExchange_T<T>(ref T, ...) to call CompareExchange (ref object, ...), but didn't delete the declaration. It's already deleted in unmanaged and in the netcore version of this file.

In #17341 we change the caller of CompareExchange_T<T>(ref T, ...) to call CompareExchange (ref object, ...), but didn't delete the declaration. It's already deleted in unmanaged and in the netcore version of this file.

migueldeicaza · 2019-10-17T17:14:33Z

From Nathan, with this fix and this one [1] Mono/LLVM/.NET5 configuration stands like this:

Mono/LLVM/.NET5 before change (req/sec):  1,478,095.55
Mono/LLVM/.NET5 after chang (req/sec) :   5,414,536.91
.NET Core 3/RyuJIT (req/sec):             6,626,015.90

So about 80% of the performance of .NET Core 3 now, looking good!

[1] #17338

…ect (#17372) * [bcl][jit] implement Interlocked.Exchange<T> in terms of object Exchange<T> is supposed to be picked up as an intrinsic. But if it isn't, since T has a class constraint, call the `Exchange (ref object, ref object, ref object)` overload instead. This works around a limitation in the JIT: https://github.com/mono/mono/blob/82a07273c22b996b08fa88ee2e6632d782ac2242/mono/mini/mini-trampolines.c#L704-L713 if we're in the common call trampoline, and the caller is generic method and the callee is a generic method, if we're using generic sharing for the caller, but there's no generic jit info for the callee, we will not patch the call site. In this case we had Exchange<T> calling Exchange_T<T> where the callee is an icall (and evidently didn't have generic jit info). So every time we called Exchange<T>, we would go through the trampoline when trying to call Exchange_T<T> without patching the call site. Addresses part of #17334 * Bump mono corlib version * Also drop CompareExchange_T<T> * Sprinkle some Unsafe magic. Mark the non-icall methods that we expect the JIT to treet as intrinsics with [Intrinsic] * aot-runtime doesn't need Volatile.Read<T>/Write<T> code Both of those methods are implemented as calls to the object overload of Volatile.Read/Write * fix netcore build * one more IntrinsicAttribute * [bcl] Remove CompareExchange_T In #17341 we change the caller of CompareExchange_T<T>(ref T, ...) to call CompareExchange (ref object, ...), but didn't delete the declaration. It's already deleted in unmanaged and in the netcore version of this file. * [bcl] Add null reference checks to Interlocked.Exchange<T> and Interlocked.CompareExchange<T> This is to mimic the old behavior in unmanaged when the intrinsic cannot be used. The issue was detected via the coreclr acceptance test: https://github.com/mono/coreclr/blob/mono/tests/src/baseservices/threading/interlocked/exchange/exchangetneg.il * and netcore * Another Mono corlib version bump

…/mono#17341) * [bcl][jit] implement Interlocked.Exchange<T> in terms of object Exchange<T> is supposed to be picked up as an intrinsic. But if it isn't, since T has a class constraint, call the `Exchange (ref object, ref object, ref object)` overload instead. This works around a limitation in the JIT: https://github.com/mono/mono/blob/mono/mono@82a07273c22b996b08fa88ee2e6632d782ac2242/mono/mini/mini-trampolines.c#L704-L713 if we're in the common call trampoline, and the caller is generic method and the callee is a generic method, if we're using generic sharing for the caller, but there's no generic jit info for the callee, we will not patch the call site. In this case we had Exchange<T> calling Exchange_T<T> where the callee is an icall (and evidently didn't have generic jit info). So every time we called Exchange<T>, we would go through the trampoline when trying to call Exchange_T<T> without patching the call site. Addresses part of mono/mono#17334 * Bump Mono * Also drop CompareExchange_T<T> * Sprinkle some Unsafe magic. Mark the non-icall methods that we expect the JIT to treet as intrinsics with [Intrinsic] * aot-runtime doesn't need Volatile.Read<T>/Write<T> code Both of those methods are implemented as calls to the object overload of Volatile.Read/Write Commit migrated from mono/mono@f017835

Commit migrated from mono/mono@91f10ae

lambdageek added 2 commits October 15, 2019 16:59

Bump Mono

357c9c8

vargaz approved these changes Oct 15, 2019

View reviewed changes

lambdageek force-pushed the fix-exchange_tref_icall branch from 85f5b4b to 357c9c8 Compare October 15, 2019 21:02

Also drop CompareExchange_T<T>

8ff494e

Sprinkle some Unsafe magic.

224fcb4

Mark the non-icall methods that we expect the JIT to treet as intrinsics with [Intrinsic]

jaykrell reviewed Oct 15, 2019

View reviewed changes

aot-runtime doesn't need Volatile.Read<T>/Write<T> code

7925cd6

Both of those methods are implemented as calls to the object overload of Volatile.Read/Write

vargaz approved these changes Oct 15, 2019

View reviewed changes

lambdageek marked this pull request as ready for review October 16, 2019 02:19

lambdageek requested review from akoeplinger, EgorBo and lewurm as code owners October 16, 2019 02:19

lambdageek requested review from marek-safar, SamMonoRT, steveisok and thaystg as code owners October 16, 2019 02:19

filipnavara reviewed Oct 16, 2019

View reviewed changes

netcore/System.Private.CoreLib/src/System.Threading/Interlocked.cs Show resolved Hide resolved

lewurm and others added 2 commits October 16, 2019 03:09

fix netcore build

12616dc

one more IntrinsicAttribute

0b471f0

lewurm mentioned this pull request Oct 16, 2019

mono_magic_trampoline being called more than expected #17334

Closed

lambdageek merged commit f017835 into mono:master Oct 16, 2019

lambdageek mentioned this pull request Oct 16, 2019

[bcl] Remove CompareExchange_T #17370

Merged

lambdageek mentioned this pull request Oct 16, 2019

[2019-10][bcl][jit] implement Interlocked.Exchange<T> in terms of object #17372

Merged

lambdageek mentioned this pull request Oct 17, 2019

Investigate heavy stack from System_Threading_Interlocked_Exchange_T_REF_T_REF__T_REF #17335

Closed

filipnavara added a commit to filipnavara/mono that referenced this pull request Oct 19, 2019

Remove dead code left out in mono#17341

f4f8323

vargaz pushed a commit that referenced this pull request Oct 20, 2019

Remove dead code left out in #17341 (#17433)

91f10ae

ManickaP pushed a commit to ManickaP/runtime that referenced this pull request Jan 20, 2020

Remove dead code left out in mono/mono#17341 (mono/mono#17433)

8a81360

Commit migrated from mono/mono@91f10ae

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[bcl][jit] implement Interlocked.Exchange<T> in terms of object #17341

[bcl][jit] implement Interlocked.Exchange<T> in terms of object #17341

lambdageek commented Oct 15, 2019

lambdageek commented Oct 15, 2019

lambdageek commented Oct 15, 2019

lambdageek commented Oct 15, 2019

lambdageek commented Oct 15, 2019

vargaz commented Oct 15, 2019

vargaz commented Oct 15, 2019

jaykrell commented Oct 15, 2019

jaykrell commented Oct 15, 2019

lambdageek commented Oct 15, 2019 •

edited

vargaz commented Oct 15, 2019

lambdageek commented Oct 15, 2019

vargaz commented Oct 15, 2019

jaykrell commented Oct 15, 2019 •

edited

jaykrell Oct 15, 2019

lambdageek Oct 15, 2019

lambdageek commented Oct 15, 2019

lambdageek commented Oct 15, 2019

vargaz commented Oct 16, 2019

lambdageek commented Oct 16, 2019

vargaz commented Oct 16, 2019

jaykrell commented Oct 16, 2019

migueldeicaza commented Oct 17, 2019

	if (plt_entry) {
	if (generic_shared) {
	target_ji =
	mini_jit_info_table_find (mono_domain_get (), (char *)mono_get_addr_from_ftnptr (compiled_method), NULL);
	if (!ji)
	ji = mini_jit_info_table_find (mono_domain_get (), (char*)code, NULL);

	if (ji && ji->has_generic_jit_info) {
	if (target_ji && !target_ji->has_generic_jit_info) {
	no_patch = TRUE;

	[Intrinsic]
	public static T Read<T>(ref T location) where T : class =>
	Unsafe.As<T>(Unsafe.As<T, VolatileObject>(ref location).Value);

	[Intrinsic]
	public static void Write<T>(ref T location, T value) where T : class =>
	Unsafe.As<T, VolatileObject>(ref location).Value = value;

	[Intrinsic]
	[NonVersionable]
	[return: NotNullIfNotNull("location")]
	public static T Read<T>(ref T location) where T : class? =>
	Unsafe.As<T>(Unsafe.As<T, VolatileObject>(ref location).Value);

	[Intrinsic]
	[NonVersionable]
	public static void Write<T>([NotNullIfNotNull("value")] ref T location, T value) where T : class? =>
	Unsafe.As<T, VolatileObject>(ref location).Value = value;

[bcl][jit] implement Interlocked.Exchange<T> in terms of object #17341

[bcl][jit] implement Interlocked.Exchange<T> in terms of object #17341

Conversation

lambdageek commented Oct 15, 2019

lambdageek commented Oct 15, 2019

lambdageek commented Oct 15, 2019

lambdageek commented Oct 15, 2019

lambdageek commented Oct 15, 2019

vargaz commented Oct 15, 2019

vargaz commented Oct 15, 2019

jaykrell commented Oct 15, 2019

jaykrell commented Oct 15, 2019

lambdageek commented Oct 15, 2019 • edited

vargaz commented Oct 15, 2019

lambdageek commented Oct 15, 2019

vargaz commented Oct 15, 2019

jaykrell commented Oct 15, 2019 • edited

jaykrell Oct 15, 2019

Choose a reason for hiding this comment

lambdageek Oct 15, 2019

Choose a reason for hiding this comment

lambdageek commented Oct 15, 2019

lambdageek commented Oct 15, 2019

vargaz commented Oct 16, 2019

lambdageek commented Oct 16, 2019

vargaz commented Oct 16, 2019

jaykrell commented Oct 16, 2019

migueldeicaza commented Oct 17, 2019

lambdageek commented Oct 15, 2019 •

edited

jaykrell commented Oct 15, 2019 •

edited