From 31ac591376d55e030ed7a77aa57b4a742d21d0b1 Mon Sep 17 00:00:00 2001 From: Nino Floris Date: Sat, 24 Jul 2021 18:46:11 +0200 Subject: [PATCH 1/4] Change IOptionsSnapshot to reuse options instances across scopes --- .../src/OptionsServiceCollectionExtensions.cs | 2 +- .../src/OptionsSnapshot.cs | 53 +++++++++++++++++++ .../OptionsSnapshotTest.cs | 23 ++++++-- 3 files changed, 72 insertions(+), 6 deletions(-) create mode 100644 src/libraries/Microsoft.Extensions.Options/src/OptionsSnapshot.cs diff --git a/src/libraries/Microsoft.Extensions.Options/src/OptionsServiceCollectionExtensions.cs b/src/libraries/Microsoft.Extensions.Options/src/OptionsServiceCollectionExtensions.cs index a84cbe19e1fa6f..3d89c86e2c90ac 100644 --- a/src/libraries/Microsoft.Extensions.Options/src/OptionsServiceCollectionExtensions.cs +++ b/src/libraries/Microsoft.Extensions.Options/src/OptionsServiceCollectionExtensions.cs @@ -27,7 +27,7 @@ public static IServiceCollection AddOptions(this IServiceCollection services) } services.TryAdd(ServiceDescriptor.Singleton(typeof(IOptions<>), typeof(UnnamedOptionsManager<>))); - services.TryAdd(ServiceDescriptor.Scoped(typeof(IOptionsSnapshot<>), typeof(OptionsManager<>))); + services.TryAdd(ServiceDescriptor.Scoped(typeof(IOptionsSnapshot<>), typeof(OptionsSnapshot<>))); services.TryAdd(ServiceDescriptor.Singleton(typeof(IOptionsMonitor<>), typeof(OptionsMonitor<>))); services.TryAdd(ServiceDescriptor.Transient(typeof(IOptionsFactory<>), typeof(OptionsFactory<>))); services.TryAdd(ServiceDescriptor.Singleton(typeof(IOptionsMonitorCache<>), typeof(OptionsCache<>))); diff --git a/src/libraries/Microsoft.Extensions.Options/src/OptionsSnapshot.cs b/src/libraries/Microsoft.Extensions.Options/src/OptionsSnapshot.cs new file mode 100644 index 00000000000000..b6d0ebcbef4915 --- /dev/null +++ b/src/libraries/Microsoft.Extensions.Options/src/OptionsSnapshot.cs @@ -0,0 +1,53 @@ +// Licensed to the .NET Foundation under one or more agreements. +// The .NET Foundation licenses this file to you under the MIT license. + +using System; +using System.Collections.Concurrent; +using System.Diagnostics.CodeAnalysis; + +namespace Microsoft.Extensions.Options +{ + /// + /// Implementation of and . + /// + /// Options type. + internal class OptionsSnapshot<[DynamicallyAccessedMembers(Options.DynamicallyAccessedMembers)] TOptions> : + IOptionsSnapshot + where TOptions : class + { + private readonly IOptionsMonitor _factory; + private readonly ConcurrentDictionary _cache = new(concurrencyLevel: 1, capacity: 5, StringComparer.Ordinal); + + /// + /// Initializes a new instance with the specified options configurations. + /// + /// The factory to use to create options. + public OptionsSnapshot(IOptionsMonitor factory) + { + _factory = factory; + } + + /// + /// The default configured instance, equivalent to Get(Options.DefaultName). + /// + public TOptions Value => Get(Options.DefaultName); + + /// + /// Returns a configured instance with the given . + /// + public TOptions Get(string name) + { + name ??= Options.DefaultName; + +#if NETSTANDARD2_1 + TOptions options = _cache.GetOrAdd(name, static (name, factory) => factory.Get(name), _factory); +#else + if (!_cache.TryGetValue(name, out TOptions options)) + { + options = _cache.GetOrAdd(name, _factory.Get(name)); + } +#endif + return options; + } + } +} diff --git a/src/libraries/Microsoft.Extensions.Options/tests/Microsoft.Extensions.Options.Tests/OptionsSnapshotTest.cs b/src/libraries/Microsoft.Extensions.Options/tests/Microsoft.Extensions.Options.Tests/OptionsSnapshotTest.cs index adb63a5c6913c9..870b3d00c51e95 100644 --- a/src/libraries/Microsoft.Extensions.Options/tests/Microsoft.Extensions.Options.Tests/OptionsSnapshotTest.cs +++ b/src/libraries/Microsoft.Extensions.Options/tests/Microsoft.Extensions.Options.Tests/OptionsSnapshotTest.cs @@ -131,12 +131,14 @@ public void Configure(string name, FakeOptions options) [Fact] public void SnapshotOptionsAreCachedPerScope() { + var config = new ConfigurationBuilder().AddInMemoryCollection().Build(); var services = new ServiceCollection() .AddOptions() - .AddScoped, TestConfigure>() + .AddSingleton, TestConfigure>() + .AddSingleton>(new ConfigurationChangeTokenSource(Options.DefaultName, config)) + .AddSingleton>(new ConfigurationChangeTokenSource("1", config)) .BuildServiceProvider(); - var cache = services.GetRequiredService>(); var factory = services.GetRequiredService(); FakeOptions options = null; FakeOptions namedOne = null; @@ -148,18 +150,29 @@ public void SnapshotOptionsAreCachedPerScope() namedOne = scope.ServiceProvider.GetRequiredService>().Get("1"); Assert.Equal(namedOne, scope.ServiceProvider.GetRequiredService>().Get("1")); Assert.Equal(2, TestConfigure.ConfigureCount); + + // Reload triggers Configure for the two registered change tokens. + config.Reload(); + Assert.Equal(4, TestConfigure.ConfigureCount); + + // Reload should not affect current scope. + var options2 = scope.ServiceProvider.GetRequiredService>().Value; + Assert.Equal(options, options2); + var namedOne2 = scope.ServiceProvider.GetRequiredService>().Get("1"); + Assert.Equal(namedOne, namedOne2); } Assert.Equal(1, TestConfigure.CtorCount); + + // Reload should be reflected in a fresh scope. using (var scope = factory.CreateScope()) { var options2 = scope.ServiceProvider.GetRequiredService>().Value; Assert.NotEqual(options, options2); - Assert.Equal(3, TestConfigure.ConfigureCount); var namedOne2 = scope.ServiceProvider.GetRequiredService>().Get("1"); - Assert.NotEqual(namedOne2, namedOne); + Assert.NotEqual(namedOne, namedOne2); Assert.Equal(4, TestConfigure.ConfigureCount); } - Assert.Equal(2, TestConfigure.CtorCount); + Assert.Equal(1, TestConfigure.CtorCount); } [Fact] From 6fd7f10269d76bd76e7f366df5676fb13ff85abc Mon Sep 17 00:00:00 2001 From: Nino Floris Date: Mon, 26 Jul 2021 20:17:16 +0200 Subject: [PATCH 2/4] Improve snapshot perf for unnamed option and delay dictionary alloc --- .../src/OptionsSnapshot.cs | 23 +++++++++++++++---- 1 file changed, 18 insertions(+), 5 deletions(-) diff --git a/src/libraries/Microsoft.Extensions.Options/src/OptionsSnapshot.cs b/src/libraries/Microsoft.Extensions.Options/src/OptionsSnapshot.cs index b6d0ebcbef4915..a4bbe9bfc2151c 100644 --- a/src/libraries/Microsoft.Extensions.Options/src/OptionsSnapshot.cs +++ b/src/libraries/Microsoft.Extensions.Options/src/OptionsSnapshot.cs @@ -4,6 +4,7 @@ using System; using System.Collections.Concurrent; using System.Diagnostics.CodeAnalysis; +using System.Threading; namespace Microsoft.Extensions.Options { @@ -16,7 +17,9 @@ internal class OptionsSnapshot<[DynamicallyAccessedMembers(Options.DynamicallyAc where TOptions : class { private readonly IOptionsMonitor _factory; - private readonly ConcurrentDictionary _cache = new(concurrencyLevel: 1, capacity: 5, StringComparer.Ordinal); + + private volatile ConcurrentDictionary _cache; + private volatile TOptions _unnamedOptionsValue; /// /// Initializes a new instance with the specified options configurations. @@ -37,14 +40,24 @@ public OptionsSnapshot(IOptionsMonitor factory) /// public TOptions Get(string name) { - name ??= Options.DefaultName; + if (name == null || name == Options.DefaultName) + { + if (_unnamedOptionsValue is TOptions value) + { + return value; + } + + return _unnamedOptionsValue = _factory.Get(Options.DefaultName); + } + + var cache = _cache ?? Interlocked.CompareExchange(ref _cache, new(concurrencyLevel: 1, capacity: 5, StringComparer.Ordinal), null) ?? _cache; #if NETSTANDARD2_1 - TOptions options = _cache.GetOrAdd(name, static (name, factory) => factory.Get(name), _factory); + TOptions options = cache.GetOrAdd(name, static (name, factory) => factory.Get(name), _factory); #else - if (!_cache.TryGetValue(name, out TOptions options)) + if (!cache.TryGetValue(name, out TOptions options)) { - options = _cache.GetOrAdd(name, _factory.Get(name)); + options = cache.GetOrAdd(name, _factory.Get(name)); } #endif return options; From 0b4c719d231a6669e0471511fdca5df7646ac8e7 Mon Sep 17 00:00:00 2001 From: Nino Floris Date: Wed, 28 Jul 2021 15:46:08 +0200 Subject: [PATCH 3/4] Address review feedback --- .../src/OptionsSnapshot.cs | 16 ++++++++-------- .../OptionsSnapshotTest.cs | 1 + 2 files changed, 9 insertions(+), 8 deletions(-) diff --git a/src/libraries/Microsoft.Extensions.Options/src/OptionsSnapshot.cs b/src/libraries/Microsoft.Extensions.Options/src/OptionsSnapshot.cs index a4bbe9bfc2151c..56937ab2b081fb 100644 --- a/src/libraries/Microsoft.Extensions.Options/src/OptionsSnapshot.cs +++ b/src/libraries/Microsoft.Extensions.Options/src/OptionsSnapshot.cs @@ -12,11 +12,11 @@ namespace Microsoft.Extensions.Options /// Implementation of and . /// /// Options type. - internal class OptionsSnapshot<[DynamicallyAccessedMembers(Options.DynamicallyAccessedMembers)] TOptions> : + internal sealed class OptionsSnapshot<[DynamicallyAccessedMembers(Options.DynamicallyAccessedMembers)] TOptions> : IOptionsSnapshot where TOptions : class { - private readonly IOptionsMonitor _factory; + private readonly IOptionsMonitor _optionsMonitor; private volatile ConcurrentDictionary _cache; private volatile TOptions _unnamedOptionsValue; @@ -24,10 +24,10 @@ internal class OptionsSnapshot<[DynamicallyAccessedMembers(Options.DynamicallyAc /// /// Initializes a new instance with the specified options configurations. /// - /// The factory to use to create options. - public OptionsSnapshot(IOptionsMonitor factory) + /// The options monitor to use to provide options. + public OptionsSnapshot(IOptionsMonitor optionsMonitor) { - _factory = factory; + _optionsMonitor = optionsMonitor; } /// @@ -47,17 +47,17 @@ public TOptions Get(string name) return value; } - return _unnamedOptionsValue = _factory.Get(Options.DefaultName); + return _unnamedOptionsValue = _optionsMonitor.Get(Options.DefaultName); } var cache = _cache ?? Interlocked.CompareExchange(ref _cache, new(concurrencyLevel: 1, capacity: 5, StringComparer.Ordinal), null) ?? _cache; #if NETSTANDARD2_1 - TOptions options = cache.GetOrAdd(name, static (name, factory) => factory.Get(name), _factory); + TOptions options = cache.GetOrAdd(name, static (name, optionsMonitor) => optionsMonitor.Get(name), _optionsMonitor); #else if (!cache.TryGetValue(name, out TOptions options)) { - options = cache.GetOrAdd(name, _factory.Get(name)); + options = cache.GetOrAdd(name, _optionsMonitor.Get(name)); } #endif return options; diff --git a/src/libraries/Microsoft.Extensions.Options/tests/Microsoft.Extensions.Options.Tests/OptionsSnapshotTest.cs b/src/libraries/Microsoft.Extensions.Options/tests/Microsoft.Extensions.Options.Tests/OptionsSnapshotTest.cs index 870b3d00c51e95..28390691c6903e 100644 --- a/src/libraries/Microsoft.Extensions.Options/tests/Microsoft.Extensions.Options.Tests/OptionsSnapshotTest.cs +++ b/src/libraries/Microsoft.Extensions.Options/tests/Microsoft.Extensions.Options.Tests/OptionsSnapshotTest.cs @@ -168,6 +168,7 @@ public void SnapshotOptionsAreCachedPerScope() { var options2 = scope.ServiceProvider.GetRequiredService>().Value; Assert.NotEqual(options, options2); + Assert.Equal(4, TestConfigure.ConfigureCount); var namedOne2 = scope.ServiceProvider.GetRequiredService>().Get("1"); Assert.NotEqual(namedOne, namedOne2); Assert.Equal(4, TestConfigure.ConfigureCount); From 48fc34b3445b74208066ba4531bf45d9f0b96b82 Mon Sep 17 00:00:00 2001 From: Nino Floris Date: Fri, 6 Aug 2021 20:29:31 +0200 Subject: [PATCH 4/4] Update src/libraries/Microsoft.Extensions.Options/src/OptionsSnapshot.cs Co-authored-by: Kahbazi --- .../Microsoft.Extensions.Options/src/OptionsSnapshot.cs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/libraries/Microsoft.Extensions.Options/src/OptionsSnapshot.cs b/src/libraries/Microsoft.Extensions.Options/src/OptionsSnapshot.cs index 56937ab2b081fb..197d97c19d7e7d 100644 --- a/src/libraries/Microsoft.Extensions.Options/src/OptionsSnapshot.cs +++ b/src/libraries/Microsoft.Extensions.Options/src/OptionsSnapshot.cs @@ -9,7 +9,7 @@ namespace Microsoft.Extensions.Options { /// - /// Implementation of and . + /// Implementation of . /// /// Options type. internal sealed class OptionsSnapshot<[DynamicallyAccessedMembers(Options.DynamicallyAccessedMembers)] TOptions> :