Skip to content
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

Rollback: remove .NET 6 build #1992

Merged
merged 1 commit into from
Feb 15, 2022
Merged

Rollback: remove .NET 6 build #1992

merged 1 commit into from
Feb 15, 2022

Conversation

NickCraver
Copy link
Collaborator

@NickCraver NickCraver commented Feb 12, 2022

This removes the .NET 6 specific build from the main library. In testing many previous performance issues filed around past releases (vs. 1.2.6), I was testing this example:

using StackExchange.Redis;
using System;
using System.Diagnostics;
using System.IO;
using System.Threading;
using System.Threading.Tasks;

var options = ConfigurationOptions.Parse("localhost,abortConnect=false");
using ConnectionMultiplexer muxer = ConnectionMultiplexer.Connect(options);
var subscriber = muxer.GetSubscriber();
int count = 0;
var sw = Stopwatch.StartNew();
Parallel.For(0, 1000, x =>
{
	subscriber.Publish("cache-events:cache-testing", "hey");
	Interlocked.Increment(ref count);
});
sw.Stop();
Console.WriteLine($"Total in {sw.ElapsedMilliseconds} ms");

This parallel thread contention state is one of the things we hoped the .NET 6 behavior for thread pools would help with. It comes from 2 main factors: defaulting to the primary thread pool (rather than our own) and the backlog moving back to Task.Run instead of a thread (which has ~11% impact on throughput due to startup cost). This happens here:

In the net5.0 build the sample takes ~100-105ms on an 8-core machine, running under .NET 6. With the net6.0 build in play (before this PR) and using those changes, we end up in the 20,000-30,000ms territory. With only the socket manager change (leaving the Backlog as a Task.Run), we're in the 400-500ms territory.

In short, trying to use the .NET 6 changes to our advantage here greatly regresses some use cases drastically enough I advise we do not enable it. We should look at these cases further before adding the net6.0 paths to any release build. I can go further than this PR and remove the code paths completely, but I'd like to have these in play easily to discuss with the .NET team as a use case to look at and get some advice.

cc @stephentoub on this one (happy to sync up and help repro)

@stephentoub
Copy link

cc: @kouvel

@mgravell
Copy link
Collaborator

To check: the real issue here is the #if NET6_OR_GREATER right? And we're leaving those in so that we can continue to repro and investigate if useful?

@NickCraver
Copy link
Collaborator Author

@mgravell Yep exactly, maybe this can help on thread pool knobs and throttle thoughts in some way.

@NickCraver NickCraver merged commit cbc7cc9 into main Feb 15, 2022
@NickCraver NickCraver deleted the craver/remove-net6 branch February 15, 2022 15:34
@mangod9
Copy link

mangod9 commented May 17, 2022

Hi @NickCraver, trying to clarify your comment above:

In the net5.0 build the sample takes ~100-105ms on an 8-core machine, running under .NET 6. With the net6.0 build in play (before this PR) and using those changes, we end up in the 20,000-30,000ms territory. With only the socket manager change (leaving the Backlog as a Task.Run), we're in the 400-500ms territory

Does this indicate that targeting 5, but running on 6 runtime is still good? We are trying to determine whether the enablement of portable (managed) threadpool in 6 is causing the issues you are observing. So any details on how to measure would be helpful. Thx!

@NickCraver
Copy link
Collaborator Author

@mangod9 Correct running is fine - it was trying to take advantage of the net6 threadpool (instead of our own) that was a major regression in some scenarios. Details in the rollback PR here: #1992

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.
@eerhardt eerhardt mentioned this pull request Jun 29, 2023
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
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants