v2.5 work: .NET 6.0: add build defaulting to the thread pool #1939
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
This moves net6.0 to using the default thread pool for pipe scheduling rather than our scheduler. Namely due to 2 major changes since this was introduced: 1. Thread theft has long since been resolved (never an issue in .NET Core+) 2. Sync-over-async is an app problem, but should be much better in .NET 6.0+ which has changes specifically for this in thread pool growth (see https://devblogs.microsoft.com/dotnet/performance-improvements-in-net-6/#threading) Due to this combo of changes, we want to see if this behaves much better in the next alpha. The advantages are we move from 10 threads by default shared between connections to no dedicated threads for the scheduler. It has the following benfits: 1. While these threads were mostly idle (waiting for data), people saw them in profiler traces and attribute them as working/CPU-consuming though that's not what a profiler is really saying. 2. The default of 10 threads is a best guess, but the world varies widely. Some users are deploying many connections on 20-100 core VMs and it's a bad default limiting them (by default - not if configured). On the other end of the spectrum, a _lot_ of people run small 1-4 core VMs or containers and the default size is bigger than needed. Instead of the above tradeoffs meant to unblock users, want to simplify, let the main threadpool scale, and hope the default is a net win for most or all users. If a consumer application has a _ton_ of sync-over-async (e.g. 100,000 tasks queued suddenly), this may be a net negative change for them - that's why we want to alpha and see how this behaves with workloads we may not have predicted.
This has what we need already: https://www.appveyor.com/docs/windows-images-software/#net-framework
This method is sensitive to load, making it a bit less sensitive.
NickCraver
commented
Jan 3, 2022
| @@ -643,7 +643,7 @@ public async Task Issue38() | |||
| } | |||
| } | |||
|
|
|||
| internal static Task AllowReasonableTimeToPublishAndProcess() => Task.Delay(100); | |||
| internal static Task AllowReasonableTimeToPublishAndProcess() => Task.Delay(500); | |||
There was a problem hiding this comment.
Note: this isn't related, but is an intermittent test failure (unrelated to this PR at all) under load due to the minimal wait for publish observations vs. over large tests. Sneaking a fix in here.
NickCraver
commented
Jan 3, 2022
| "unknown-" | ||
| #endif | ||
| + Path.GetFileNameWithoutExtension(filePath) + "-" + caller; | ||
| Environment.Version.ToString() + Path.GetFileNameWithoutExtension(filePath) + "-" + caller; |
There was a problem hiding this comment.
This isn't a 1:1 replacement, but the goal is to simple make it unique per version - let's go simpler and never update this with TFMs again.
NickCraver
commented
Jan 3, 2022
| @@ -1,5 +1,5 @@ | |||
| { | |||
| "version": "2.2", | |||
| "version": "2.5-prerelease", | |||
There was a problem hiding this comment.
Note: this change also exists in #1912 - whichever lands first wins the v2.5-prelease versioning kick.
NickCraver
changed the title
Closed
mgravell
reviewed
Jan 5, 2022
mgravell
approved these changes
Jan 5, 2022
|
@mgravell ahahaha, |
eerhardt
added a commit
to eerhardt/StackExchange.Redis
that referenced
this pull request
Jun 29, 2023
This allows for using new APIs introduced in net6.0. In order to enable building for net6.0, we also need to revert the thread pool changes in StackExchange#1939 and StackExchange#1950. This was already effectively reverted in StackExchange#1992 by not building for net6.0. Now that we are building for net6.0 again, these if-defs need to be removed.
Merged
NickCraver
pushed a commit
that referenced
this pull request
Jul 1, 2023
This allows for using new APIs introduced in net6.0. In order to enable building for net6.0, we also need to revert the thread pool changes in #1939 and #1950. This was already effectively reverted in #1992 by not building for net6.0. Now that we are building for net6.0 again, these if-defs need to be removed.
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.
This adds a
net6.0build (and test run) that uses the default thread pool for pipe scheduling rather than our scheduler. Namely due to 2 major changes since this was introduced:Due to this combo of changes, we want to see if this behaves much better in the next alpha. The advantages are we move from 10 threads by default shared between connections to no dedicated threads for the scheduler. It has the following benefits:
Instead of the above tradeoffs meant to unblock users, want to simplify, let the main thread pool scale, and hope the default is a net win for most or all users. If a consumer application has a ton of sync-over-async (e.g. 100,000 tasks queued suddenly), this may be a net negative change for them - that's why we want to alpha and see how this behaves with workloads we may not have predicted.