Monitor.TryEnter asm timeout != 0 before spin

[benaadams]

With this change Monitor.TryEnter(o, 0) moves from the slowest opportunistic locking to the fastest (when using the inline asm path)

corefx tests pass, not sure how to trigger the TRACK_SYNC flag?

1M iterations:

method	pre (ms)	post (ms)	improvement
Monitor.TryEnter(o, 0)	221,739.76	5.49	x 40389.8

Usage from apisof.net

method	API port	nuget
Monitor.TryEnter(object)	1.5%	0.8%
Monitor.TryEnter(object, bool)	1.2%	0.3%
Monitor.TryEnter(object, int)	2.1%	1.0%
Monitor.TryEnter(object, int, bool)	0.2%	0.1%
Monitor.TryEnter(object, timespan)	0.4%	0.3%
Monitor.TryEnter(object, timespan, bool)	0.1%	0.1%

Resolves https://github.com/dotnet/coreclr/issues/6950
Resolves aspnet/KestrelHttpServer#1068

[jkotas]

There is also portable C++ implementation in vm/jithelpers.cpp. Does it have the bug as well?

I doubt that the hand-written assembly version has significant perf advantage. It may be best to switch to use the portable C++ implementation everywhere. Defining FEATURE_IMPLICIT_TLS in clrdefinitions.cmake should do the trick. And then do some perf measurements to understand the impact of switching to C++ implementation.

cc @rahku

[benaadams]

JIT_MonTryEnter_Portable doesn't have the bug, and is fairly straightforward; after checking the timeout (and failing fast) it calls into ObjHeader::EnterObjMonitorHelperSpin in syncbl.inl which does the spinning.

From what I'm understanding JIT_MonTryEnter_InlineGetThread is an assembly tail call for the managed code, which then falls back to a call into AwareLock::TryEnter in syncblk.cpp via a chain of calls.

So its advantage is for the uncontended acquire where its calling semantics would be like an inlined intrinsic CAS for the caller; so with very little overhead.

I thought the simplest route would be to introduce a new register like rsi as JIT_MonTryEnter_Slow does but its prologue is a little more complicated so restoring its value would be also (i.e. it doesn't use rsp until exit)

[benaadams]

Actually... If I understand it correctly, I can do that.

[benaadams]

Works with a twist..

[benaadams]

@jkotas updated description as this now fixes the issue for both parts.

[jkotas]

I do not think you need the extra register. The timeout is stored at [rsp + 10h] already, so the two places that need to check it can just do "cmp dword ptr [rsp + 10h], 0"

[benaadams]

Its stored at one of two places [rsp + 10h] or [rsp + MON_ENTER_STACK_SIZE_INLINEGETTHREAD + 10h] so code would be messier and use more ifdef?

Also it would be a stack compare for every loop spin rather than a register compare? (not sure if that would make a huge difference)

[jkotas]

I do not see a problem with adding extra ifdef in a few more places.

The shortest codepath through this helper is the non-contended case that acquires lock successfully. We should not be making this path longer. Once there is a contention - that will always burn a lot of cycles underneath, accessing stack instead of register is not a problem.

[benaadams]

Ok

[benaadams]

If doing the cmp directly, I don't think I need to restore rdx before exit? (Its not done in the thinlock)

[jkotas]

I agree.

[benaadams]

Runs even faster without the extra register. Have updated timings. Corefx tests pass.

FreeBSD error is "threading.WaitForSingleObject.WFSOExSemaphoreTest"

Expected thread to wait for 2000 ms (and get interrupted). Thread waited for 1912 ms! (Acceptable delta: 300)

Which seems unusual but unrelated? Test doesn't use Monitor

[jkotas]

There is "mov rdx, [rsp + 10h]" instruction right before "jmp JITutil_MonTryEnter" that will take care of restoring rdx. This change should not be necessary.

[benaadams]

ah, and its already done the rsp restore so it is in the right place.

[jkotas]

LGTM modulo last few comments. Thanks!

[benaadams]

@dotnet-bot test Linux ARM Emulator Cross Release Build
@dotnet-bot test OSX x64 Checked Build and Test