SpinLock.TryEnter fail fast for timeout 0

[benaadams]

Previously the timeout 0 would Interlocked.Add to set the waiters then CAS spin to unset it immediately after; now it exits before trying to set the waiters so skips both.

As timeout zero is tested early, removed the first time check as the next minimum time of 1ms is unlikely to have passed.

Moved the second time check into the spinning if, as if not spinning, again, 1ms is unlikely to have passed by this point.

Passes corefx tests

1M iters (single thread, uncontended but locked) for code

bool lockTaken = false;
var s = new SpinLock(false);
s.Enter(ref lockTaken);

method	pre (ms)	post (ms)	improvement
s.TryEnter(0, ref lockTaken)	24.40	5.87	x 4.1

[benaadams]

@stephentoub seeing similar effects to the previous for threadpool i.e. not much change or improvements

SubTask Chain Return 
Testing 2,621,440 calls, with GCs after 262,144 calls.
Operations per second
                                                                           Parallelism
                             Serial          2x         16x         64x        512x
i5 4 core no HT
Pre
SubTask Chain Return      655.733 k     1.303 M     4.549 M     4.256 M     4.542 M
- Depth    2              529.755 k     1.513 M     3.249 M     4.663 M     5.090 M
- Depth   16              735.980 k     2.084 M     4.164 M     4.383 M     5.851 M
- Depth   64              573.045 k     1.817 M     5.784 M     5.533 M     5.614 M
- Depth  512              624.174 k     1.606 M     3.755 M     4.285 M     5.296 M

Post
SubTask Chain Return      690.480 k     1.622 M     4.079 M     4.014 M     4.151 M
- Depth    2              744.904 k     2.021 M     4.656 M     4.710 M     4.729 M
- Depth   16              986.041 k     2.480 M     5.564 M     5.534 M     5.752 M
- Depth   64              985.141 k     2.617 M     5.050 M     5.591 M     5.658 M
- Depth  512                1.017 M     2.643 M     5.736 M     5.775 M     5.884 M

[benaadams]

The threadpool contention it resolves is when a single threadpool thread has a full local queue of dependent sub tasks and all the other queues are empty. Most other situations its fine with.

[danmoseley]

@kouvel @jkotas can you review this change ?

[danmoseley]

I see @stephentoub reviewed #6944

[benaadams]

@danmosemsft this was a less dramatic change that produced mostly the same overall effect (as suggested by @stephentoub)

[benaadams]

Build machine went offline

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

[jkotas]

The millisecondsTimeout == 0 check should be after EndCriticalRegion - it will break the full framework build otherwise.

[benaadams]

Fixed

[jkotas]

Would it make sense to check for millisecondsTimeout in TryEnter as well - right before ContinueTryEnter?

[benaadams]

Probably rather than attempting a double acquire. It would mean adding another Thread.EndCriticalRegion() on a not acquire branch.

However its a pretty heavily stacked function which does currently always inline; maybe revisit as an enhancement? (i.e. would need some balancing to get right and keep it inlining)

[jkotas]

Make sense.