This repository has been archived by the owner on Jan 23, 2023. It is now read-only.
Don't Sleep(1) in some spin-wait loops #21722
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Member
kouvel
commented
Dec 30, 2018
•
kouvel
commented
- In spin-wait loops that are not expected to last too long, Sleep(1) significantly delays threads from completing the operation
- From eliminating Sleep(1) in such spin-wait loops, there can be a tradeoff, better fairness vs worse throughput, because Sleep(1) removes threads from contention and in some cases fewer threads can make faster progress at the cost of delaying the Sleep(1) threads relatively significantly. Eliminating the Sleep(1) in such spin-wait loops seems to be a good tradeoff.
- Fixes https://github.com/dotnet/corefx/issues/29595
- In spin-wait loops that are not expected to last too long, Sleep(1) significantly delays threads from completing the operation - From eliminating Sleep(1) in such spin-wait loops, there can be a tradeoff, better fairness vs worse throughput, because Sleep(1) removes threads from contention and in some cases fewer threads can make faster progress at the cost of delaying the Sleep(1) threads relatively significantly. Eliminating the Sleep(1) in such spin-wait loops seems to be a good tradeoff.
|
With a short delay between operations (more realistic), there are mostly improvements. ConcurrentStack's Pop in particular reaches Sleep(1) very quickly and so was very unfair to begin with. With no delay between operations (less realistic), the tradeoff is more visible in throughput regressions. In a scenario where multiple operations are done frequently in short order on a concurernt collection, beyond a threshold a lock with a non-concurrent collection may perform better. Code for tests is here: #13670 (comment) |
kouvel
added a commit
to kouvel/corefx
that referenced
this pull request
Dec 30, 2018
Fx counterpart to dotnet/coreclr#21722
|
I also experimented with removing the spin-wait in compare-exchange loops in some cases where a thread is always guaranteed to make progress (compare-exchanging a recently observed state to a new state unconditionally), it appeared to be worse in fairness and throughput so I left in the spin-waits. |
stephentoub
approved these changes
Dec 31, 2018
jkotas
approved these changes
Dec 31, 2018
|
@Anipik Looks like the CoreLib mirror is down. Could you please take a look? |
|
There was a merge conflict due to this dotnet/corert@dfb9267 i tried to use the corert version |
Dotnet-GitSync-Bot
pushed a commit
to Dotnet-GitSync-Bot/corert
that referenced
this pull request
Dec 31, 2018
- In spin-wait loops that are not expected to last too long, Sleep(1) significantly delays threads from completing the operation - From eliminating Sleep(1) in such spin-wait loops, there can be a tradeoff, better fairness vs worse throughput, because Sleep(1) removes threads from contention and in some cases fewer threads can make faster progress at the cost of delaying the Sleep(1) threads relatively significantly. Eliminating the Sleep(1) in such spin-wait loops seems to be a good tradeoff. Signed-off-by: dotnet-bot <dotnet-bot@microsoft.com>
Dotnet-GitSync-Bot
pushed a commit
to Dotnet-GitSync-Bot/mono
that referenced
this pull request
Dec 31, 2018
- In spin-wait loops that are not expected to last too long, Sleep(1) significantly delays threads from completing the operation - From eliminating Sleep(1) in such spin-wait loops, there can be a tradeoff, better fairness vs worse throughput, because Sleep(1) removes threads from contention and in some cases fewer threads can make faster progress at the cost of delaying the Sleep(1) threads relatively significantly. Eliminating the Sleep(1) in such spin-wait loops seems to be a good tradeoff. Signed-off-by: dotnet-bot <dotnet-bot@microsoft.com>
|
The CoreCLR version of the file is the correct one. I have force pushed the fix to #21724. Is that ok? |
|
i think that should be fine. But i think that would made the corert version out of sync. I will correct everything tomorrow morning when i have access to the commit database. |
jkotas
pushed a commit
to dotnet/corert
that referenced
this pull request
Dec 31, 2018
- In spin-wait loops that are not expected to last too long, Sleep(1) significantly delays threads from completing the operation - From eliminating Sleep(1) in such spin-wait loops, there can be a tradeoff, better fairness vs worse throughput, because Sleep(1) removes threads from contention and in some cases fewer threads can make faster progress at the cost of delaying the Sleep(1) threads relatively significantly. Eliminating the Sleep(1) in such spin-wait loops seems to be a good tradeoff. Signed-off-by: dotnet-bot <dotnet-bot@microsoft.com>
marek-safar
pushed a commit
to mono/mono
that referenced
this pull request
Dec 31, 2018
- In spin-wait loops that are not expected to last too long, Sleep(1) significantly delays threads from completing the operation - From eliminating Sleep(1) in such spin-wait loops, there can be a tradeoff, better fairness vs worse throughput, because Sleep(1) removes threads from contention and in some cases fewer threads can make faster progress at the cost of delaying the Sleep(1) threads relatively significantly. Eliminating the Sleep(1) in such spin-wait loops seems to be a good tradeoff. Signed-off-by: dotnet-bot <dotnet-bot@microsoft.com>
stephentoub
pushed a commit
to dotnet/corefx
that referenced
this pull request
Jan 2, 2019
Fx counterpart to dotnet/coreclr#21722
This was referenced Oct 30, 2020
picenka21
pushed a commit
to picenka21/runtime
that referenced
this pull request
Feb 18, 2022
- In spin-wait loops that are not expected to last too long, Sleep(1) significantly delays threads from completing the operation - From eliminating Sleep(1) in such spin-wait loops, there can be a tradeoff, better fairness vs worse throughput, because Sleep(1) removes threads from contention and in some cases fewer threads can make faster progress at the cost of delaying the Sleep(1) threads relatively significantly. Eliminating the Sleep(1) in such spin-wait loops seems to be a good tradeoff. Commit migrated from dotnet/coreclr@7bd31ac
picenka21
pushed a commit
to picenka21/runtime
that referenced
this pull request
Feb 18, 2022
Fx counterpart to dotnet/coreclr#21722 Commit migrated from dotnet/corefx@4b6cc4f
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.