-
Notifications
You must be signed in to change notification settings - Fork 1.5k
Use a dedicated thread, rather than the thread-pool, for the backlog processor #1854
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
Merged
Conversation
This file contains hidden or 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
philon-msft
approved these changes
Sep 1, 2021
NickCraver
approved these changes
Sep 1, 2021
Collaborator
NickCraver
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looking good 👍
WeihanLi
reviewed
Sep 1, 2021
TimLovellSmith
approved these changes
Sep 1, 2021
Collaborator
Author
|
That (test) is a tricky one, as the scenario where it matters is "we're
desperately unwell"
…On Thu, 2 Sep 2021, 06:04 Weihan Li, ***@***.***> wrote:
***@***.**** commented on this pull request.
------------------------------
In src/StackExchange.Redis/PhysicalBridge.cs
<#1854 (comment)>
:
> @@ -770,7 +770,14 @@ private void StartBacklogProcessor()
_backlogProcessorRequestedTime = Environment.TickCount;
#endif
_backlogStatus = BacklogStatus.Activating;
- Task.Run(ProcessBacklogAsync);
+
+ // start the backlog processor; this is a bit unorthadox, as you would *expect* this to just
+ // be Task.Run; that would work fine when healthy, but when we're falling on our face, it is
+ // easy to get into a thread-pool-starvation "spiral of death" if we rely on the thread-pool
+ // to unblock the thread-pool when there could be sync-over-async callers. Note that in reality,
+ // the initial "enough" of the back-log processor is typically sync, which means that the thread
+ // we start is actually useful, despite thinking "but that will just go async and back to the pool"
+ new Thread(s => ((PhysicalBridge)s).ProcessBacklogAsync().RedisFireAndForget()).Start(this);
Just a question, not sure if it's better, a test may be needed to
determine.
And with Thread, maybe declare it as a background thread is more
meaningful.
—
You are receiving this because you authored the thread.
Reply to this email directly, view it on GitHub
<#1854 (comment)>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/AAAEHMAP6Y2NMND7PR5VWYLT74AUNANCNFSM5DGVYOPQ>
.
Triage notifications on the go with GitHub Mobile for iOS
<https://apps.apple.com/app/apple-store/id1477376905?ct=notification-email&mt=8&pt=524675>
or Android
<https://play.google.com/store/apps/details?id=com.github.android&referrer=utm_campaign%3Dnotification-email%26utm_medium%3Demail%26utm_source%3Dgithub>.
|
NickCraver
approved these changes
Sep 2, 2021
NickCraver
added a commit
that referenced
this pull request
Jan 17, 2022
In #1854 we switched to a Thread here to prevent the pile-up case, however under concurrent load this incurs a 10-11% penalty given backlogs aren't all that uncommon (due to the lock contention case). In .NET 6.0+, let's use the thread pool growth semantics to handle the stall case and use the cheaper Task.Run() approach.
NickCraver
added a commit
that referenced
this pull request
Jan 18, 2022
In #1854 we switched to a `Thread` here to prevent the pile-up case, however under concurrent load this incurs a 10-11% penalty given backlogs aren't all that uncommon (due to the lock contention case). In .NET 6.0+, let's use the thread pool growth semantics to handle the stall case and use the cheaper `Task.Run()` approach. Running the benchmarks from #1164 I was experimenting with on lock perf, I noticed a large perf sink in thread creation as a result of lock contention. For that benchmark as an example (high concurrent load) we're talking about a ~93 second -> ~84 second run reduction and similar wins in concurrent load tests.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
Pain point in the last release was a spiral of death triggered (or at least: exacerbated) by the backlog processor