New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

SpinLock.TryEnter fail fast for timeout 0 #6952

Merged
merged 3 commits into from Oct 14, 2016

Conversation

Projects
None yet
5 participants
@benaadams
Collaborator

benaadams commented Aug 27, 2016

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 benaadams changed the title from SpinLock.TryEnter fail fast for timeout 0 (2) to SpinLock.TryEnter fail fast for timeout 0 Aug 28, 2016

@benaadams

This comment has been minimized.

Show comment
Hide comment
@benaadams

benaadams Aug 28, 2016

Collaborator

@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
Collaborator

benaadams commented Aug 28, 2016

@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

This comment has been minimized.

Show comment
Hide comment
@benaadams

benaadams Aug 28, 2016

Collaborator

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.

Collaborator

benaadams commented Aug 28, 2016

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.

@danmosemsft

This comment has been minimized.

Show comment
Hide comment
@danmosemsft

danmosemsft Oct 13, 2016

Member

@kouvel @jkotas can you review this change ?

Member

danmosemsft commented Oct 13, 2016

@kouvel @jkotas can you review this change ?

@danmosemsft

This comment has been minimized.

Show comment
Hide comment
@danmosemsft

danmosemsft Oct 13, 2016

Member

I see @stephentoub reviewed #6944

Member

danmosemsft commented Oct 13, 2016

I see @stephentoub reviewed #6944

@benaadams

This comment has been minimized.

Show comment
Hide comment
@benaadams

benaadams Oct 13, 2016

Collaborator

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

Collaborator

benaadams commented Oct 13, 2016

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

@benaadams

This comment has been minimized.

Show comment
Hide comment
@benaadams

benaadams Oct 14, 2016

Collaborator

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

Collaborator

benaadams commented Oct 14, 2016

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

return;
}
#if !FEATURE_CORECLR
Thread.EndCriticalRegion();
#endif
if (millisecondsTimeout == 0)

This comment has been minimized.

@jkotas

jkotas Oct 14, 2016

Member

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

@jkotas

jkotas Oct 14, 2016

Member

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

This comment has been minimized.

@benaadams

benaadams Oct 14, 2016

Collaborator

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)

@benaadams

benaadams Oct 14, 2016

Collaborator

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)

This comment has been minimized.

@jkotas

jkotas Oct 14, 2016

Member

Make sense.

@jkotas

jkotas Oct 14, 2016

Member

Make sense.

@jkotas

jkotas approved these changes Oct 14, 2016

@jkotas jkotas merged commit 0855b70 into dotnet:master Oct 14, 2016

12 checks passed

CentOS7.1 x64 Debug Build and Test Build finished.
Details
FreeBSD x64 Checked Build Build finished.
Details
Linux ARM Emulator Cross Debug Build Build finished.
Details
Linux ARM Emulator Cross Release Build Build finished.
Details
OSX x64 Checked Build and Test Build finished.
Details
Ubuntu x64 Checked Build and Test Build finished.
Details
Ubuntu x64 Formatting Build finished.
Details
Windows_NT x64 Debug Build and Test Build finished.
Details
Windows_NT x64 Formatting Build finished.
Details
Windows_NT x64 Release Priority 1 Build and Test Build finished.
Details
Windows_NT x86 legacy_backend Checked Build and Test Build finished.
Details
Windows_NT x86 ryujit Checked Build and Test Build finished.
Details

sergign60 added a commit to sergign60/coreclr that referenced this pull request Nov 14, 2016

SpinLock.TryEnter fail fast for timeout 0 (#6952)
* SpinLock.TryEnter fail fast for timeout 0 (2)

* Don't over check SpinLock timeout

When 1ms unlikely to have passed

@karelz karelz modified the milestone: 2.0.0 Aug 28, 2017

@benaadams benaadams deleted the benaadams:spinlock-failfast-less branch Mar 27, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment