From 76d9389d8531b26cd7c37fef16955befdb9bd1e4 Mon Sep 17 00:00:00 2001 From: James Newton-King Date: Sun, 16 Feb 2025 22:28:50 +0800 Subject: [PATCH 1/7] Avoid using ConcurrentDictionary for channels with few methods --- src/Grpc.Net.Client/GrpcChannel.cs | 5 +- .../Internal/ThreadSafeLookup.cs | 82 +++++++++++++++++++ 2 files changed, 84 insertions(+), 3 deletions(-) create mode 100644 src/Grpc.Net.Client/Internal/ThreadSafeLookup.cs diff --git a/src/Grpc.Net.Client/GrpcChannel.cs b/src/Grpc.Net.Client/GrpcChannel.cs index 05e170e04..1ee1d41a6 100644 --- a/src/Grpc.Net.Client/GrpcChannel.cs +++ b/src/Grpc.Net.Client/GrpcChannel.cs @@ -16,7 +16,6 @@ #endregion -using System.Collections.Concurrent; using System.Diagnostics; using Grpc.Core; #if SUPPORT_LOAD_BALANCING @@ -51,7 +50,7 @@ public sealed partial class GrpcChannel : ChannelBase, IDisposable internal const long DefaultMaxRetryBufferPerCallSize = 1024 * 1024; // 1 MB private readonly object _lock; - private readonly ConcurrentDictionary _methodInfoCache; + private readonly ThreadSafeLookup _methodInfoCache; private readonly Func _createMethodInfoFunc; private readonly Dictionary? _serviceConfigMethods; private readonly bool _isSecure; @@ -109,7 +108,7 @@ public sealed partial class GrpcChannel : ChannelBase, IDisposable internal GrpcChannel(Uri address, GrpcChannelOptions channelOptions) : base(address.Authority) { _lock = new object(); - _methodInfoCache = new ConcurrentDictionary(); + _methodInfoCache = new ThreadSafeLookup(); // Dispose the HTTP client/handler if... // 1. No client/handler was specified and so the channel created the client itself diff --git a/src/Grpc.Net.Client/Internal/ThreadSafeLookup.cs b/src/Grpc.Net.Client/Internal/ThreadSafeLookup.cs new file mode 100644 index 000000000..2fae63675 --- /dev/null +++ b/src/Grpc.Net.Client/Internal/ThreadSafeLookup.cs @@ -0,0 +1,82 @@ +#region Copyright notice and license + +// Copyright 2019 The gRPC Authors +// +// Licensed under the Apache License, Version 2.0 (the "License"); +// you may not use this file except in compliance with the License. +// You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, software +// distributed under the License is distributed on an "AS IS" BASIS, +// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +// See the License for the specific language governing permissions and +// limitations under the License. + +#endregion + +using System.Collections.Concurrent; + +internal sealed class ThreadSafeLookup where TKey : notnull +{ + // Avoid allocating ConcurrentDictionary until the threshold is reached. + // Looking up a key in an array is as fast as a dictionary for small collections and uses much less memory. + private const int Threshold = 10; + + private KeyValuePair[] _array = Array.Empty>(); + private ConcurrentDictionary? _dictionary; + + public TValue GetOrAdd(TKey key, Func valueFactory) + { + if (_dictionary != null) + { + return _dictionary.GetOrAdd(key, valueFactory); + } + + var snapshot = _array; + foreach (var kvp in snapshot) + { + if (EqualityComparer.Default.Equals(kvp.Key, key)) + { + return kvp.Value; + } + } + + var newValue = valueFactory(key); + + if (snapshot.Length + 1 > Threshold) + { + // Lock here to ensure that only one thread will create the initial dictionary. + lock (this) + { + if (_dictionary != null) + { + _dictionary.TryAdd(key, newValue); + } + else + { + var newDict = new ConcurrentDictionary(); + foreach (var kvp in snapshot) + { + newDict.TryAdd(kvp.Key, kvp.Value); + } + newDict.TryAdd(key, newValue); + + _dictionary = newDict; + _array = Array.Empty>(); + } + } + } + else + { + var newArray = new KeyValuePair[snapshot.Length + 1]; + Array.Copy(snapshot, newArray, snapshot.Length); + newArray[newArray.Length - 1] = new KeyValuePair(key, newValue); + + _array = newArray; + } + + return newValue; + } +} From 739e67fc0be3aad81e31d5d85c05321539d78f14 Mon Sep 17 00:00:00 2001 From: James Newton-King Date: Sun, 16 Feb 2025 22:34:20 +0800 Subject: [PATCH 2/7] Update --- .../Internal/ThreadSafeLookup.cs | 4 +- .../ThreadSafeLookupTests.cs | 69 +++++++++++++++++++ 2 files changed, 72 insertions(+), 1 deletion(-) create mode 100644 test/Grpc.Net.Client.Tests/ThreadSafeLookupTests.cs diff --git a/src/Grpc.Net.Client/Internal/ThreadSafeLookup.cs b/src/Grpc.Net.Client/Internal/ThreadSafeLookup.cs index 2fae63675..9f73e633b 100644 --- a/src/Grpc.Net.Client/Internal/ThreadSafeLookup.cs +++ b/src/Grpc.Net.Client/Internal/ThreadSafeLookup.cs @@ -22,7 +22,7 @@ internal sealed class ThreadSafeLookup where TKey : notnull { // Avoid allocating ConcurrentDictionary until the threshold is reached. // Looking up a key in an array is as fast as a dictionary for small collections and uses much less memory. - private const int Threshold = 10; + internal const int Threshold = 10; private KeyValuePair[] _array = Array.Empty>(); private ConcurrentDictionary? _dictionary; @@ -70,6 +70,8 @@ public TValue GetOrAdd(TKey key, Func valueFactory) } else { + // Add new value by creating a new array with old plus new value. + // This allows for lookups without locks and is more memory efficient than a dictionary. var newArray = new KeyValuePair[snapshot.Length + 1]; Array.Copy(snapshot, newArray, snapshot.Length); newArray[newArray.Length - 1] = new KeyValuePair(key, newValue); diff --git a/test/Grpc.Net.Client.Tests/ThreadSafeLookupTests.cs b/test/Grpc.Net.Client.Tests/ThreadSafeLookupTests.cs new file mode 100644 index 000000000..57d073c61 --- /dev/null +++ b/test/Grpc.Net.Client.Tests/ThreadSafeLookupTests.cs @@ -0,0 +1,69 @@ +#region Copyright notice and license + +// Copyright 2019 The gRPC Authors +// +// Licensed under the Apache License, Version 2.0 (the "License"); +// you may not use this file except in compliance with the License. +// You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, software +// distributed under the License is distributed on an "AS IS" BASIS, +// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +// See the License for the specific language governing permissions and +// limitations under the License. + +#endregion + +namespace Grpc.Net.Client.Tests; + +[TestFixture] +public class ThreadSafeLookupTests +{ + [Test] + public void GetOrAdd_ReturnsCorrectValueForNewKey() + { + var lookup = new ThreadSafeLookup(); + var result = lookup.GetOrAdd(1, k => "Value-1"); + + Assert.AreEqual("Value-1", result); + } + + [Test] + public void GetOrAdd_ReturnsExistingValueForExistingKey() + { + var lookup = new ThreadSafeLookup(); + lookup.GetOrAdd(1, k => "InitialValue"); + var result = lookup.GetOrAdd(1, k => "NewValue"); + + Assert.AreEqual("InitialValue", result); + } + + [Test] + public void GetOrAdd_SwitchesToDictionaryAfterThreshold() + { + var addCount = (ThreadSafeLookup.Threshold * 2); + var lookup = new ThreadSafeLookup(); + + for (var i = 0; i <= addCount; i++) + { + lookup.GetOrAdd(i, k => $"Value-{k}"); + } + + var result = lookup.GetOrAdd(addCount, k => $"NewValue-{addCount}"); + + Assert.AreEqual($"Value-{addCount}", result); + } + + [Test] + public void GetOrAdd_HandlesConcurrentAccess() + { + var lookup = new ThreadSafeLookup(); + Parallel.For(0, 1000, i => + { + var value = lookup.GetOrAdd(i, k => $"Value-{k}"); + Assert.AreEqual($"Value-{i}", value); + }); + } +} From d9b4a3a9b39abe59d3317ef321c680273a9a532a Mon Sep 17 00:00:00 2001 From: James Newton-King Date: Sun, 16 Feb 2025 22:59:55 +0800 Subject: [PATCH 3/7] Fix build? --- .github/workflows/test.yml | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/.github/workflows/test.yml b/.github/workflows/test.yml index 8dace7b8f..b9e727bb2 100644 --- a/.github/workflows/test.yml +++ b/.github/workflows/test.yml @@ -15,7 +15,7 @@ jobs: build: name: Basic Tests - runs-on: ubuntu-latest + runs-on: ubuntu-22.04 steps: - name: Check out code @@ -36,7 +36,7 @@ jobs: grpc_web: name: gRPC-Web Tests - runs-on: ubuntu-latest + runs-on: ubuntu-22.04 steps: - name: Check out code From 8ceb615a7eb496d64d0152b9a0a5c2ab34cde5ee Mon Sep 17 00:00:00 2001 From: James Newton-King Date: Mon, 17 Feb 2025 20:50:16 +0800 Subject: [PATCH 4/7] Update src/Grpc.Net.Client/Internal/ThreadSafeLookup.cs MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Co-authored-by: Günther Foidl --- src/Grpc.Net.Client/Internal/ThreadSafeLookup.cs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/Grpc.Net.Client/Internal/ThreadSafeLookup.cs b/src/Grpc.Net.Client/Internal/ThreadSafeLookup.cs index 9f73e633b..5d28a21d1 100644 --- a/src/Grpc.Net.Client/Internal/ThreadSafeLookup.cs +++ b/src/Grpc.Net.Client/Internal/ThreadSafeLookup.cs @@ -45,7 +45,7 @@ public TValue GetOrAdd(TKey key, Func valueFactory) var newValue = valueFactory(key); - if (snapshot.Length + 1 > Threshold) + if (snapshot.Length > Threshold - 1) { // Lock here to ensure that only one thread will create the initial dictionary. lock (this) From a3e4a0553a9d0c763c2e15696394ebae20f863d6 Mon Sep 17 00:00:00 2001 From: James Newton-King Date: Tue, 18 Feb 2025 09:24:01 +0800 Subject: [PATCH 5/7] PR feedback --- .../Internal/ThreadSafeLookup.cs | 66 +++++++++++++------ 1 file changed, 45 insertions(+), 21 deletions(-) diff --git a/src/Grpc.Net.Client/Internal/ThreadSafeLookup.cs b/src/Grpc.Net.Client/Internal/ThreadSafeLookup.cs index 5d28a21d1..1b38704bd 100644 --- a/src/Grpc.Net.Client/Internal/ThreadSafeLookup.cs +++ b/src/Grpc.Net.Client/Internal/ThreadSafeLookup.cs @@ -27,6 +27,10 @@ internal sealed class ThreadSafeLookup where TKey : notnull private KeyValuePair[] _array = Array.Empty>(); private ConcurrentDictionary? _dictionary; + /// + /// Gets the value for the key if it exists. If the key does not exist then the value is created using the valueFactory. + /// The value is created outside of a lock and there is no guarentee which value will be stored or returned. + /// public TValue GetOrAdd(TKey key, Func valueFactory) { if (_dictionary != null) @@ -34,30 +38,36 @@ public TValue GetOrAdd(TKey key, Func valueFactory) return _dictionary.GetOrAdd(key, valueFactory); } - var snapshot = _array; - foreach (var kvp in snapshot) + if (TryGetValue(_array, key, out var value)) { - if (EqualityComparer.Default.Equals(kvp.Key, key)) - { - return kvp.Value; - } + return value; } var newValue = valueFactory(key); +<<<<<<< Updated upstream if (snapshot.Length > Threshold - 1) +======= + lock (this) +>>>>>>> Stashed changes { - // Lock here to ensure that only one thread will create the initial dictionary. - lock (this) + if (_dictionary != null) { - if (_dictionary != null) + _dictionary.TryAdd(key, newValue); + } + else + { + // Double check inside lock if the key was added to the array by another thread. + if (TryGetValue(_array, key, out value)) { - _dictionary.TryAdd(key, newValue); + return value; } - else + + if (_array.Length + 1 > Threshold) { + // Array length exceeds threshold so switch to dictionary. var newDict = new ConcurrentDictionary(); - foreach (var kvp in snapshot) + foreach (var kvp in _array) { newDict.TryAdd(kvp.Key, kvp.Value); } @@ -66,19 +76,33 @@ public TValue GetOrAdd(TKey key, Func valueFactory) _dictionary = newDict; _array = Array.Empty>(); } + else + { + // Add new value by creating a new array with old plus new value. + var newArray = new KeyValuePair[_array.Length + 1]; + Array.Copy(_array, newArray, _array.Length); + newArray[newArray.Length - 1] = new KeyValuePair(key, newValue); + + _array = newArray; + } } } - else - { - // Add new value by creating a new array with old plus new value. - // This allows for lookups without locks and is more memory efficient than a dictionary. - var newArray = new KeyValuePair[snapshot.Length + 1]; - Array.Copy(snapshot, newArray, snapshot.Length); - newArray[newArray.Length - 1] = new KeyValuePair(key, newValue); - _array = newArray; + return newValue; + } + + private static bool TryGetValue(KeyValuePair[] array, TKey key, out TValue value) + { + foreach (var kvp in array) + { + if (EqualityComparer.Default.Equals(kvp.Key, key)) + { + value = kvp.Value; + return true; + } } - return newValue; + value = default!; + return false; } } From 485dbda03068fe254acea838d149b39a6cd6c3cc Mon Sep 17 00:00:00 2001 From: James Newton-King Date: Tue, 18 Feb 2025 09:26:49 +0800 Subject: [PATCH 6/7] Fix merge --- src/Grpc.Net.Client/Internal/ThreadSafeLookup.cs | 4 ---- 1 file changed, 4 deletions(-) diff --git a/src/Grpc.Net.Client/Internal/ThreadSafeLookup.cs b/src/Grpc.Net.Client/Internal/ThreadSafeLookup.cs index 1b38704bd..c1a0d1f9c 100644 --- a/src/Grpc.Net.Client/Internal/ThreadSafeLookup.cs +++ b/src/Grpc.Net.Client/Internal/ThreadSafeLookup.cs @@ -45,11 +45,7 @@ public TValue GetOrAdd(TKey key, Func valueFactory) var newValue = valueFactory(key); -<<<<<<< Updated upstream - if (snapshot.Length > Threshold - 1) -======= lock (this) ->>>>>>> Stashed changes { if (_dictionary != null) { From 15a34094983a5c8f2759501ec2f8a778b58dcc58 Mon Sep 17 00:00:00 2001 From: James Newton-King Date: Tue, 18 Feb 2025 17:00:37 +0800 Subject: [PATCH 7/7] Update src/Grpc.Net.Client/Internal/ThreadSafeLookup.cs MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Co-authored-by: Günther Foidl --- src/Grpc.Net.Client/Internal/ThreadSafeLookup.cs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/Grpc.Net.Client/Internal/ThreadSafeLookup.cs b/src/Grpc.Net.Client/Internal/ThreadSafeLookup.cs index c1a0d1f9c..b7a37b8d1 100644 --- a/src/Grpc.Net.Client/Internal/ThreadSafeLookup.cs +++ b/src/Grpc.Net.Client/Internal/ThreadSafeLookup.cs @@ -59,7 +59,7 @@ public TValue GetOrAdd(TKey key, Func valueFactory) return value; } - if (_array.Length + 1 > Threshold) + if (_array.Length > Threshold - 1) { // Array length exceeds threshold so switch to dictionary. var newDict = new ConcurrentDictionary();