From 121124c520418ff1f82ca3bd3a53f36214204589 Mon Sep 17 00:00:00 2001 From: Michael Adelson Date: Tue, 15 Mar 2022 20:16:46 -0400 Subject: [PATCH 1/6] Avoid allocating a delegate in OptionsMonitor.Get() when possible. Fix #61086 --- .../src/OptionsCache.cs | 29 ++++++++++-- .../src/OptionsMonitor.cs | 5 +- .../OptionsMonitorTest.cs | 47 +++++++++++++++++++ 3 files changed, 76 insertions(+), 5 deletions(-) diff --git a/src/libraries/Microsoft.Extensions.Options/src/OptionsCache.cs b/src/libraries/Microsoft.Extensions.Options/src/OptionsCache.cs index 869fd75b4b6e2..55d6a7a97e62f 100644 --- a/src/libraries/Microsoft.Extensions.Options/src/OptionsCache.cs +++ b/src/libraries/Microsoft.Extensions.Options/src/OptionsCache.cs @@ -31,9 +31,9 @@ public class OptionsCache<[DynamicallyAccessedMembers(Options.DynamicallyAccesse public virtual TOptions GetOrAdd(string? name, Func createOptions!!) { name ??= Options.DefaultName; - Lazy? value; + Lazy value; -#if NETSTANDARD2_1 +#if NET || NETSTANDARD2_1 value = _cache.GetOrAdd(name, static (name, createOptions) => new Lazy(createOptions), createOptions); #else if (!_cache.TryGetValue(name, out value)) @@ -45,6 +45,29 @@ public virtual TOptions GetOrAdd(string? name, Func createOptions!!) return value.Value; } + internal TOptions GetOrAdd(string? name, Func createOptions, TArg factoryArgument) + { + // For compatibility, fall back to public GetOrAdd() if we're in a derived class. + if (GetType() != typeof(OptionsCache)) + { + return GetOrAdd(name, () => createOptions(name ?? Options.DefaultName, factoryArgument)); + } + + name ??= Options.DefaultName; + Lazy value; + +#if NET || NETSTANDARD2_1 + value = _cache.GetOrAdd(name, static (name, arg) => new Lazy(arg.createOptions(name, arg.factoryArgument)), (createOptions, factoryArgument)); +#else + if (!_cache.TryGetValue(name, out value)) + { + value = _cache.GetOrAdd(name, new Lazy(() => createOptions(name, factoryArgument))); + } +#endif + + return value.Value; + } + /// /// Gets a named options instance, if available. /// @@ -72,7 +95,7 @@ internal bool TryGetValue(string? name, [MaybeNullWhen(false)] out TOptions opti public virtual bool TryAdd(string? name, TOptions options!!) { return _cache.TryAdd(name ?? Options.DefaultName, new Lazy( -#if !NETSTANDARD2_1 +#if NETSTANDARD2_0 || NETFRAMEWORK () => #endif options)); diff --git a/src/libraries/Microsoft.Extensions.Options/src/OptionsMonitor.cs b/src/libraries/Microsoft.Extensions.Options/src/OptionsMonitor.cs index d75bc70b1e51e..c81b1841db21d 100644 --- a/src/libraries/Microsoft.Extensions.Options/src/OptionsMonitor.cs +++ b/src/libraries/Microsoft.Extensions.Options/src/OptionsMonitor.cs @@ -85,8 +85,9 @@ public TOptions CurrentValue /// public virtual TOptions Get(string? name) { - name = name ?? Options.DefaultName; - return _cache.GetOrAdd(name, () => _factory.Create(name)); + return _cache is OptionsCache optionsCache + ? optionsCache.GetOrAdd(name, (name, factory) => factory.Create(name), _factory) + : _cache.GetOrAdd(name ??= Options.DefaultName, () => _factory.Create(name)); } /// diff --git a/src/libraries/Microsoft.Extensions.Options/tests/Microsoft.Extensions.Options.Tests/OptionsMonitorTest.cs b/src/libraries/Microsoft.Extensions.Options/tests/Microsoft.Extensions.Options.Tests/OptionsMonitorTest.cs index 858b90dccd7ee..e23079d888d71 100644 --- a/src/libraries/Microsoft.Extensions.Options/tests/Microsoft.Extensions.Options.Tests/OptionsMonitorTest.cs +++ b/src/libraries/Microsoft.Extensions.Options/tests/Microsoft.Extensions.Options.Tests/OptionsMonitorTest.cs @@ -3,6 +3,7 @@ using System; using System.Collections.Generic; +using System.Linq; using System.Threading; using Microsoft.Extensions.Configuration; using Microsoft.Extensions.DependencyInjection; @@ -418,5 +419,51 @@ public ChangeTokenSource(IChangeToken changeToken) public IChangeToken GetChangeToken() => _changeToken; } + + [Fact] + public void CallsPublicGetOrAddForCustomOptionsCache() + { + DerivedOptionsCache derivedOptionsCache = new(); + CreateMonitor(derivedOptionsCache).Get(null); + Assert.Equal(1, derivedOptionsCache.GetOrAddCalls); + + ImplementedOptionsCache implementedOptionsCache = new(); + CreateMonitor(implementedOptionsCache).Get(null); + Assert.Equal(1, implementedOptionsCache.GetOrAddCalls); + + static OptionsMonitor CreateMonitor(IOptionsMonitorCache cache) => + new OptionsMonitor( + new OptionsFactory(Enumerable.Empty>(), Enumerable.Empty>()), + Enumerable.Empty>(), + cache); + } + + private class DerivedOptionsCache : OptionsCache + { + public int GetOrAddCalls { get; private set; } + + public override FakeOptions GetOrAdd(string? name, Func createOptions) + { + GetOrAddCalls++; + return base.GetOrAdd(name, createOptions); + } + } + + private class ImplementedOptionsCache : IOptionsMonitorCache + { + public int GetOrAddCalls { get; private set; } + + public void Clear() => throw new NotImplementedException(); + + public FakeOptions GetOrAdd(string? name, Func createOptions) + { + GetOrAddCalls++; + return createOptions(); + } + + public bool TryAdd(string? name, FakeOptions options) => throw new NotImplementedException(); + + public bool TryRemove(string? name) => throw new NotImplementedException(); + } } } From 9093586aa4eb0f194f363394f9e93a671c5b0d84 Mon Sep 17 00:00:00 2001 From: Michael Adelson Date: Tue, 15 Mar 2022 20:36:30 -0400 Subject: [PATCH 2/6] Address feedback from https://github.com/dotnet/runtime/pull/66688#discussion_r827519222 --- .../Microsoft.Extensions.Options/src/OptionsCache.cs | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-) diff --git a/src/libraries/Microsoft.Extensions.Options/src/OptionsCache.cs b/src/libraries/Microsoft.Extensions.Options/src/OptionsCache.cs index 55d6a7a97e62f..cb4dfb68e1f60 100644 --- a/src/libraries/Microsoft.Extensions.Options/src/OptionsCache.cs +++ b/src/libraries/Microsoft.Extensions.Options/src/OptionsCache.cs @@ -50,7 +50,7 @@ internal TOptions GetOrAdd(string? name, Func crea // For compatibility, fall back to public GetOrAdd() if we're in a derived class. if (GetType() != typeof(OptionsCache)) { - return GetOrAdd(name, () => createOptions(name ?? Options.DefaultName, factoryArgument)); + return GetOrAddHelper(name, createOptions, factoryArgument); } name ??= Options.DefaultName; @@ -68,6 +68,10 @@ internal TOptions GetOrAdd(string? name, Func crea return value.Value; } + // provides indirection to avoid the caller needing to allocate a closure + private TOptions GetOrAddHelper(string? name, Func createOptions, TArg factoryArgument) => + GetOrAdd(name, () => createOptions(name ?? Options.DefaultName, factoryArgument)); + /// /// Gets a named options instance, if available. /// From 82ed15333af78d4ca23383488994a427470a246d Mon Sep 17 00:00:00 2001 From: Michael Adelson Date: Fri, 18 Mar 2022 13:56:17 -0400 Subject: [PATCH 3/6] Use locals to avoid unnecessary closure allocations. --- .../src/OptionsCache.cs | 17 ++++++++++------- 1 file changed, 10 insertions(+), 7 deletions(-) diff --git a/src/libraries/Microsoft.Extensions.Options/src/OptionsCache.cs b/src/libraries/Microsoft.Extensions.Options/src/OptionsCache.cs index cb4dfb68e1f60..1d313b239cc28 100644 --- a/src/libraries/Microsoft.Extensions.Options/src/OptionsCache.cs +++ b/src/libraries/Microsoft.Extensions.Options/src/OptionsCache.cs @@ -38,7 +38,8 @@ public virtual TOptions GetOrAdd(string? name, Func createOptions!!) #else if (!_cache.TryGetValue(name, out value)) { - value = _cache.GetOrAdd(name, new Lazy(createOptions)); + Func localCreateOptions = createOptions; + value = _cache.GetOrAdd(name, new Lazy(localCreateOptions)); } #endif @@ -50,7 +51,10 @@ internal TOptions GetOrAdd(string? name, Func crea // For compatibility, fall back to public GetOrAdd() if we're in a derived class. if (GetType() != typeof(OptionsCache)) { - return GetOrAddHelper(name, createOptions, factoryArgument); + string? localName = name; + Func localCreateOptions = createOptions; + TArg localFactoryArgument = factoryArgument; + return GetOrAdd(name, () => localCreateOptions(localName ?? Options.DefaultName, localFactoryArgument)); } name ??= Options.DefaultName; @@ -61,17 +65,16 @@ internal TOptions GetOrAdd(string? name, Func crea #else if (!_cache.TryGetValue(name, out value)) { - value = _cache.GetOrAdd(name, new Lazy(() => createOptions(name, factoryArgument))); + string? localName = name; + Func localCreateOptions = createOptions; + TArg localFactoryArgument = factoryArgument; + value = _cache.GetOrAdd(name, new Lazy(() => localCreateOptions(localName, localFactoryArgument))); } #endif return value.Value; } - // provides indirection to avoid the caller needing to allocate a closure - private TOptions GetOrAddHelper(string? name, Func createOptions, TArg factoryArgument) => - GetOrAdd(name, () => createOptions(name ?? Options.DefaultName, factoryArgument)); - /// /// Gets a named options instance, if available. /// From 83a98a9bfcfbcfc884a334b6cea137acff0ed6f9 Mon Sep 17 00:00:00 2001 From: Michael Adelson Date: Sat, 19 Mar 2022 14:18:44 -0400 Subject: [PATCH 4/6] Implement feedback from https://github.com/dotnet/runtime/pull/66688 --- .../Microsoft.Extensions.Options/src/OptionsCache.cs | 3 +++ .../Microsoft.Extensions.Options.Tests/OptionsMonitorTest.cs | 4 ++-- 2 files changed, 5 insertions(+), 2 deletions(-) diff --git a/src/libraries/Microsoft.Extensions.Options/src/OptionsCache.cs b/src/libraries/Microsoft.Extensions.Options/src/OptionsCache.cs index 1d313b239cc28..acad6fb6e6043 100644 --- a/src/libraries/Microsoft.Extensions.Options/src/OptionsCache.cs +++ b/src/libraries/Microsoft.Extensions.Options/src/OptionsCache.cs @@ -38,6 +38,7 @@ public virtual TOptions GetOrAdd(string? name, Func createOptions!!) #else if (!_cache.TryGetValue(name, out value)) { + // copying captured variables to locals avoids allocating a closure if we don't enter the if Func localCreateOptions = createOptions; value = _cache.GetOrAdd(name, new Lazy(localCreateOptions)); } @@ -51,6 +52,7 @@ internal TOptions GetOrAdd(string? name, Func crea // For compatibility, fall back to public GetOrAdd() if we're in a derived class. if (GetType() != typeof(OptionsCache)) { + // copying captured variables to locals avoids allocating a closure if we don't enter the if string? localName = name; Func localCreateOptions = createOptions; TArg localFactoryArgument = factoryArgument; @@ -65,6 +67,7 @@ internal TOptions GetOrAdd(string? name, Func crea #else if (!_cache.TryGetValue(name, out value)) { + // copying captured variables to locals avoids allocating a closure if we don't enter the if string? localName = name; Func localCreateOptions = createOptions; TArg localFactoryArgument = factoryArgument; diff --git a/src/libraries/Microsoft.Extensions.Options/tests/Microsoft.Extensions.Options.Tests/OptionsMonitorTest.cs b/src/libraries/Microsoft.Extensions.Options/tests/Microsoft.Extensions.Options.Tests/OptionsMonitorTest.cs index e23079d888d71..1831fbb3f4899 100644 --- a/src/libraries/Microsoft.Extensions.Options/tests/Microsoft.Extensions.Options.Tests/OptionsMonitorTest.cs +++ b/src/libraries/Microsoft.Extensions.Options/tests/Microsoft.Extensions.Options.Tests/OptionsMonitorTest.cs @@ -438,7 +438,7 @@ static OptionsMonitor CreateMonitor(IOptionsMonitorCache + private sealed class DerivedOptionsCache : OptionsCache { public int GetOrAddCalls { get; private set; } @@ -449,7 +449,7 @@ public override FakeOptions GetOrAdd(string? name, Func createOptio } } - private class ImplementedOptionsCache : IOptionsMonitorCache + private sealed class ImplementedOptionsCache : IOptionsMonitorCache { public int GetOrAddCalls { get; private set; } From e0e46bb809b861d95b3435fa96d0887feb236a79 Mon Sep 17 00:00:00 2001 From: Michael Adelson Date: Sat, 19 Mar 2022 16:01:22 -0400 Subject: [PATCH 5/6] Remove another closure allocation in OptionsMonitor and add test for #61086. --- .../src/OptionsMonitor.cs | 14 +++++++++++--- .../OptionsMonitorTest.cs | 19 +++++++++++++++++++ 2 files changed, 30 insertions(+), 3 deletions(-) diff --git a/src/libraries/Microsoft.Extensions.Options/src/OptionsMonitor.cs b/src/libraries/Microsoft.Extensions.Options/src/OptionsMonitor.cs index c81b1841db21d..3f1c02900f629 100644 --- a/src/libraries/Microsoft.Extensions.Options/src/OptionsMonitor.cs +++ b/src/libraries/Microsoft.Extensions.Options/src/OptionsMonitor.cs @@ -85,9 +85,17 @@ public TOptions CurrentValue /// public virtual TOptions Get(string? name) { - return _cache is OptionsCache optionsCache - ? optionsCache.GetOrAdd(name, (name, factory) => factory.Create(name), _factory) - : _cache.GetOrAdd(name ??= Options.DefaultName, () => _factory.Create(name)); + if (_cache is not OptionsCache optionsCache) + { + // copying captured variables to locals avoids allocating a closure if we don't enter the if + string localName = name ?? Options.DefaultName; + IOptionsFactory localFactory = _factory; + return _cache.GetOrAdd(localName, () => localFactory.Create(localName)); + } + + // non-allocating fast path + return optionsCache.GetOrAdd(name, static (name, factory) => factory.Create(name), _factory); + } /// diff --git a/src/libraries/Microsoft.Extensions.Options/tests/Microsoft.Extensions.Options.Tests/OptionsMonitorTest.cs b/src/libraries/Microsoft.Extensions.Options/tests/Microsoft.Extensions.Options.Tests/OptionsMonitorTest.cs index 1831fbb3f4899..88c032a133077 100644 --- a/src/libraries/Microsoft.Extensions.Options/tests/Microsoft.Extensions.Options.Tests/OptionsMonitorTest.cs +++ b/src/libraries/Microsoft.Extensions.Options/tests/Microsoft.Extensions.Options.Tests/OptionsMonitorTest.cs @@ -465,5 +465,24 @@ public FakeOptions GetOrAdd(string? name, Func createOptions) public bool TryRemove(string? name) => throw new NotImplementedException(); } + +#if NET // need GC.GetAllocatedBytesForCurrentThread() + /// + /// Tests the fix for https://github.com/dotnet/runtime/issues/61086 + /// + [Fact] + public void TestCurrentValueDoesNotAllocateOnceValueIsCached() + { + var monitor = new OptionsMonitor( + new OptionsFactory(Enumerable.Empty>(), Enumerable.Empty>()), + Enumerable.Empty>(), + new OptionsCache()); + Assert.NotNull(monitor.CurrentValue); // populate the cache + + long initialBytes = GC.GetAllocatedBytesForCurrentThread(); + _ = monitor.CurrentValue; + Assert.Equal(0, GC.GetAllocatedBytesForCurrentThread() - initialBytes); + } +#endif } } From 32820fb2988350c26a55ec6ff257549fa656799d Mon Sep 17 00:00:00 2001 From: Michael Adelson Date: Mon, 21 Mar 2022 20:40:55 -0400 Subject: [PATCH 6/6] Address feedback from https://github.com/dotnet/runtime/pull/66688 --- .../src/OptionsCache.cs | 27 ++++++------------- 1 file changed, 8 insertions(+), 19 deletions(-) diff --git a/src/libraries/Microsoft.Extensions.Options/src/OptionsCache.cs b/src/libraries/Microsoft.Extensions.Options/src/OptionsCache.cs index acad6fb6e6043..902f314bf8124 100644 --- a/src/libraries/Microsoft.Extensions.Options/src/OptionsCache.cs +++ b/src/libraries/Microsoft.Extensions.Options/src/OptionsCache.cs @@ -38,9 +38,7 @@ public virtual TOptions GetOrAdd(string? name, Func createOptions!!) #else if (!_cache.TryGetValue(name, out value)) { - // copying captured variables to locals avoids allocating a closure if we don't enter the if - Func localCreateOptions = createOptions; - value = _cache.GetOrAdd(name, new Lazy(localCreateOptions)); + value = _cache.GetOrAdd(name, new Lazy(createOptions)); } #endif @@ -50,7 +48,10 @@ public virtual TOptions GetOrAdd(string? name, Func createOptions!!) internal TOptions GetOrAdd(string? name, Func createOptions, TArg factoryArgument) { // For compatibility, fall back to public GetOrAdd() if we're in a derived class. + // For simplicity, we do the same for older frameworks that don't support the factoryArgument overload of GetOrAdd(). +#if NET || NETSTANDARD2_1 if (GetType() != typeof(OptionsCache)) +#endif { // copying captured variables to locals avoids allocating a closure if we don't enter the if string? localName = name; @@ -59,23 +60,11 @@ internal TOptions GetOrAdd(string? name, Func crea return GetOrAdd(name, () => localCreateOptions(localName ?? Options.DefaultName, localFactoryArgument)); } - name ??= Options.DefaultName; - Lazy value; - #if NET || NETSTANDARD2_1 - value = _cache.GetOrAdd(name, static (name, arg) => new Lazy(arg.createOptions(name, arg.factoryArgument)), (createOptions, factoryArgument)); -#else - if (!_cache.TryGetValue(name, out value)) - { - // copying captured variables to locals avoids allocating a closure if we don't enter the if - string? localName = name; - Func localCreateOptions = createOptions; - TArg localFactoryArgument = factoryArgument; - value = _cache.GetOrAdd(name, new Lazy(() => localCreateOptions(localName, localFactoryArgument))); - } + return _cache.GetOrAdd( + name ?? Options.DefaultName, + static (name, arg) => new Lazy(arg.createOptions(name, arg.factoryArgument)), (createOptions, factoryArgument)).Value; #endif - - return value.Value; } /// @@ -105,7 +94,7 @@ internal bool TryGetValue(string? name, [MaybeNullWhen(false)] out TOptions opti public virtual bool TryAdd(string? name, TOptions options!!) { return _cache.TryAdd(name ?? Options.DefaultName, new Lazy( -#if NETSTANDARD2_0 || NETFRAMEWORK +#if !(NET || NETSTANDARD2_1) () => #endif options));