From e60579d5020c62d250c6863cb5ea876d4df72ebb Mon Sep 17 00:00:00 2001 From: Nick Craver Date: Mon, 3 Jan 2022 14:53:18 -0500 Subject: [PATCH 1/7] Add net6.0 build defaulting to the thread pool 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. --- src/StackExchange.Redis/CommandTrace.cs | 4 ++-- .../ConnectionMultiplexer.ReaderWriter.cs | 24 +++++++++++++++++-- .../StackExchange.Redis.csproj | 2 +- tests/StackExchange.Redis.Tests/Config.cs | 2 +- .../StackExchange.Redis.Tests.csproj | 2 +- tests/StackExchange.Redis.Tests/TestBase.cs | 11 +++------ version.json | 2 +- 7 files changed, 31 insertions(+), 16 deletions(-) diff --git a/src/StackExchange.Redis/CommandTrace.cs b/src/StackExchange.Redis/CommandTrace.cs index de3cd1849..dc60a2a8d 100644 --- a/src/StackExchange.Redis/CommandTrace.cs +++ b/src/StackExchange.Redis/CommandTrace.cs @@ -50,7 +50,7 @@ public string GetHelpUrl() const string BaseUrl = "https://redis.io/commands/"; - string encoded0 = Uri.EscapeUriString(((string)Arguments[0]).ToLowerInvariant()); + string encoded0 = Uri.EscapeDataString(((string)Arguments[0]).ToLowerInvariant()); if (Arguments.Length > 1) { @@ -62,7 +62,7 @@ public string GetHelpUrl() case "config": case "debug": case "pubsub": - string encoded1 = Uri.EscapeUriString(((string)Arguments[1]).ToLowerInvariant()); + string encoded1 = Uri.EscapeDataString(((string)Arguments[1]).ToLowerInvariant()); return BaseUrl + encoded0 + "-" + encoded1; } } diff --git a/src/StackExchange.Redis/ConnectionMultiplexer.ReaderWriter.cs b/src/StackExchange.Redis/ConnectionMultiplexer.ReaderWriter.cs index f0e9fa6b1..0781963fb 100644 --- a/src/StackExchange.Redis/ConnectionMultiplexer.ReaderWriter.cs +++ b/src/StackExchange.Redis/ConnectionMultiplexer.ReaderWriter.cs @@ -1,4 +1,6 @@ -namespace StackExchange.Redis +using System; + +namespace StackExchange.Redis { public partial class ConnectionMultiplexer { @@ -6,7 +8,7 @@ public partial class ConnectionMultiplexer partial void OnCreateReaderWriter(ConfigurationOptions configuration) { - SocketManager = configuration.SocketManager ?? SocketManager.Shared; + SocketManager = configuration.SocketManager ?? GetDefaultSocketManager(); } partial void OnCloseReaderWriter() @@ -14,5 +16,23 @@ partial void OnCloseReaderWriter() SocketManager = null; } partial void OnWriterCreated(); + + private static readonly Version SharedThreadpoolMinVersion = new(6, 0, 0); + + /// + /// .NET 6.0+ has changes to sync-over-async stalls in the .NET primary thread pool + /// If we're in that environment, by default remove the overhead of our own threadpool + /// This will eliminate some context-switching overhead and better-size threads on both large + /// and small environments, from 16 core machines to single core VMs where the default 10 threads + /// isn't an ideal situation. + /// + internal static SocketManager GetDefaultSocketManager() + { +#if NET6_0_OR_GREATER + return SocketManager.ThreadPool; +#else + return SocketManager.Shared; +#endif + } } } diff --git a/src/StackExchange.Redis/StackExchange.Redis.csproj b/src/StackExchange.Redis/StackExchange.Redis.csproj index a571adf1d..9f2fbb9cd 100644 --- a/src/StackExchange.Redis/StackExchange.Redis.csproj +++ b/src/StackExchange.Redis/StackExchange.Redis.csproj @@ -1,7 +1,7 @@  - net461;netstandard2.0;net472;netcoreapp3.1;net5.0 + net461;netstandard2.0;net472;netcoreapp3.1;net5.0;net6.0 High performance Redis client, incorporating both synchronous and asynchronous usage. StackExchange.Redis StackExchange.Redis diff --git a/tests/StackExchange.Redis.Tests/Config.cs b/tests/StackExchange.Redis.Tests/Config.cs index 201af69c9..d119923e4 100644 --- a/tests/StackExchange.Redis.Tests/Config.cs +++ b/tests/StackExchange.Redis.Tests/Config.cs @@ -457,7 +457,7 @@ public void DefaultThreadPoolManagerIsDetected() EndPoints = { { IPAddress.Loopback, 6379 } }, }; using var muxer = ConnectionMultiplexer.Connect(config); - Assert.Same(SocketManager.Shared.Scheduler, muxer.SocketManager.Scheduler); + Assert.Same(ConnectionMultiplexer.GetDefaultSocketManager().Scheduler, muxer.SocketManager.Scheduler); } [Theory] diff --git a/tests/StackExchange.Redis.Tests/StackExchange.Redis.Tests.csproj b/tests/StackExchange.Redis.Tests/StackExchange.Redis.Tests.csproj index 184407fd9..5cf895e50 100644 --- a/tests/StackExchange.Redis.Tests/StackExchange.Redis.Tests.csproj +++ b/tests/StackExchange.Redis.Tests/StackExchange.Redis.Tests.csproj @@ -1,6 +1,6 @@  - net472;netcoreapp3.1 + net472;netcoreapp3.1;net6.0 StackExchange.Redis.Tests true true diff --git a/tests/StackExchange.Redis.Tests/TestBase.cs b/tests/StackExchange.Redis.Tests/TestBase.cs index c31a72178..21bf84f55 100644 --- a/tests/StackExchange.Redis.Tests/TestBase.cs +++ b/tests/StackExchange.Redis.Tests/TestBase.cs @@ -361,14 +361,7 @@ public static ConnectionMultiplexer CreateDefault( } public static string Me([CallerFilePath] string filePath = null, [CallerMemberName] string caller = null) => -#if NET472 - "net472-" -#elif NETCOREAPP3_1 - "netcoreapp3.1-" -#else - "unknown-" -#endif - + Path.GetFileNameWithoutExtension(filePath) + "-" + caller; + Environment.Version.ToString() + Path.GetFileNameWithoutExtension(filePath) + "-" + caller; protected static TimeSpan RunConcurrent(Action work, int threads, int timeout = 10000, [CallerMemberName] string caller = null) { @@ -417,7 +410,9 @@ void callback() for (int i = 0; i < threads; i++) { var thd = threadArr[i]; +#if !NET6_0_OR_GREATER if (thd.IsAlive) thd.Abort(); +#endif } throw new TimeoutException(); } diff --git a/version.json b/version.json index 4d664c308..8191d532a 100644 --- a/version.json +++ b/version.json @@ -1,5 +1,5 @@ { - "version": "2.2", + "version": "2.5-prerelease", "versionHeightOffset": -1, "assemblyVersion": "2.0", "publicReleaseRefSpec": [ From 92482509063fe61722a7d4917c312dd52de6bc39 Mon Sep 17 00:00:00 2001 From: Nick Craver Date: Mon, 3 Jan 2022 14:57:29 -0500 Subject: [PATCH 2/7] Oh right, workflows --- .github/workflows/CI.yml | 20 +++++++++----------- 1 file changed, 9 insertions(+), 11 deletions(-) diff --git a/.github/workflows/CI.yml b/.github/workflows/CI.yml index 5dacdc4d2..d8687ed74 100644 --- a/.github/workflows/CI.yml +++ b/.github/workflows/CI.yml @@ -16,14 +16,13 @@ jobs: steps: - name: Checkout code uses: actions/checkout@v1 - - name: Setup .NET Core 3.x - uses: actions/setup-dotnet@v1 - with: - dotnet-version: '3.1.x' - - name: Setup .NET 5.x + - name: Setup .NET Core uses: actions/setup-dotnet@v1 with: - dotnet-version: '5.0.x' + dotnet-version: | + 3.1.x + 5.0.x + 6.0.x - name: .NET Build run: dotnet build Build.csproj -c Release /p:CI=true - name: Start Redis Services (docker-compose) @@ -50,11 +49,10 @@ jobs: - name: Setup .NET Core 3.x uses: actions/setup-dotnet@v1 with: - dotnet-version: '3.1.x' - - name: Setup .NET 5.x - uses: actions/setup-dotnet@v1 - with: - dotnet-version: '5.0.x' + dotnet-version: | + 3.1.x + 5.0.x + 6.0.x - name: .NET Build run: dotnet build Build.csproj -c Release /p:CI=true - name: Start Redis Services (v3.0.503) From 20c82e55c3b0c1b7bbfef72aeacbfeed35ef4b1f Mon Sep 17 00:00:00 2001 From: Nick Craver Date: Mon, 3 Jan 2022 15:07:43 -0500 Subject: [PATCH 3/7] AppVeyor: try VS 2022 image This has what we need already: https://www.appveyor.com/docs/windows-images-software/#net-framework --- appveyor.yml | 4 +--- 1 file changed, 1 insertion(+), 3 deletions(-) diff --git a/appveyor.yml b/appveyor.yml index 2cbf38d46..fa644bfd1 100644 --- a/appveyor.yml +++ b/appveyor.yml @@ -1,13 +1,11 @@ image: -- Visual Studio 2019 +- Visual Studio 2022 init: - git config --global core.autocrlf input install: - cmd: >- - choco install dotnet-sdk --version 5.0.100 - cd tests\RedisConfigs\3.0.503 redis-server.exe --service-install --service-name "redis-6379" "..\Basic\master-6379.conf" From 0c3a02bbad28a76f2af069fc1028adba78dc9db0 Mon Sep 17 00:00:00 2001 From: Nick Craver Date: Mon, 3 Jan 2022 15:09:21 -0500 Subject: [PATCH 4/7] Nope. "Image 'Visual Studio 2022' is not supported by the target cloud." Whatever...we'll change up the build later. --- appveyor.yml | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-) diff --git a/appveyor.yml b/appveyor.yml index fa644bfd1..51d5f56a4 100644 --- a/appveyor.yml +++ b/appveyor.yml @@ -1,11 +1,15 @@ image: -- Visual Studio 2022 +- Visual Studio 2019 init: - git config --global core.autocrlf input install: - cmd: >- + choco install dotnet-sdk --version 5.0.404 + + choco install dotnet-sdk --version 6.0.101 + cd tests\RedisConfigs\3.0.503 redis-server.exe --service-install --service-name "redis-6379" "..\Basic\master-6379.conf" From 00f3ab3c912a478cb1cce22382fdb01877a344cc Mon Sep 17 00:00:00 2001 From: Nick Craver Date: Mon, 3 Jan 2022 15:18:21 -0500 Subject: [PATCH 5/7] Unrelated test fix This method is sensitive to load, making it a bit less sensitive. --- tests/StackExchange.Redis.Tests/PubSub.cs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/tests/StackExchange.Redis.Tests/PubSub.cs b/tests/StackExchange.Redis.Tests/PubSub.cs index e4d22798d..0e4131913 100644 --- a/tests/StackExchange.Redis.Tests/PubSub.cs +++ b/tests/StackExchange.Redis.Tests/PubSub.cs @@ -643,7 +643,7 @@ public async Task Issue38() } } - internal static Task AllowReasonableTimeToPublishAndProcess() => Task.Delay(100); + internal static Task AllowReasonableTimeToPublishAndProcess() => Task.Delay(500); [Fact] public async Task TestPartialSubscriberGetMessage() From d8e15fa05eb4b5c45c04194646c6e4a7a293390f Mon Sep 17 00:00:00 2001 From: Nick Craver Date: Wed, 5 Jan 2022 11:47:12 -0500 Subject: [PATCH 6/7] Remove unused SharedThreadpoolMinVersion --- src/StackExchange.Redis/ConnectionMultiplexer.ReaderWriter.cs | 2 -- 1 file changed, 2 deletions(-) diff --git a/src/StackExchange.Redis/ConnectionMultiplexer.ReaderWriter.cs b/src/StackExchange.Redis/ConnectionMultiplexer.ReaderWriter.cs index 0781963fb..6a298b4f0 100644 --- a/src/StackExchange.Redis/ConnectionMultiplexer.ReaderWriter.cs +++ b/src/StackExchange.Redis/ConnectionMultiplexer.ReaderWriter.cs @@ -17,8 +17,6 @@ partial void OnCloseReaderWriter() } partial void OnWriterCreated(); - private static readonly Version SharedThreadpoolMinVersion = new(6, 0, 0); - /// /// .NET 6.0+ has changes to sync-over-async stalls in the .NET primary thread pool /// If we're in that environment, by default remove the overhead of our own threadpool From 3100a2a76cce8d69b21ea621cde0bf779932624a Mon Sep 17 00:00:00 2001 From: Nick Craver Date: Wed, 5 Jan 2022 11:53:26 -0500 Subject: [PATCH 7/7] LAST STRAW --- tests/StackExchange.Redis.Tests/Migrate.cs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/tests/StackExchange.Redis.Tests/Migrate.cs b/tests/StackExchange.Redis.Tests/Migrate.cs index 91bd373e3..73711129d 100644 --- a/tests/StackExchange.Redis.Tests/Migrate.cs +++ b/tests/StackExchange.Redis.Tests/Migrate.cs @@ -10,7 +10,7 @@ public class Migrate : TestBase { public Migrate(ITestOutputHelper output) : base (output) { } - [Fact] + [FactLongRunning] public async Task Basic() { var fromConfig = new ConfigurationOptions { EndPoints = { { TestConfig.Current.SecureServer, TestConfig.Current.SecurePort } }, Password = TestConfig.Current.SecurePassword, AllowAdmin = true };