Avoid allocating a delegate in OptionsMonitor.Get() when possible. (d…

…otnet#66688)

* Avoid allocating a delegate in OptionsMonitor.Get() when possible.

Fix dotnet#61086

* Address feedback from dotnet#66688 (comment)

* Use locals to avoid unnecessary closure allocations.

* Remove another closure allocation in OptionsMonitor and add test for dotnet#61086.

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<TOptions> createOptions!!)
         {
             name ??= Options.DefaultName;
-            Lazy<TOptions>? value;
+            Lazy<TOptions> value;
 
-#if NETSTANDARD2_1
+#if NET || NETSTANDARD2_1
             value = _cache.GetOrAdd(name, static (name, createOptions) => new Lazy<TOptions>(createOptions), createOptions);
 #else
             if (!_cache.TryGetValue(name, out value))
@@ -45,6 +45,28 @@ public virtual TOptions GetOrAdd(string? name, Func<TOptions> createOptions!!)
             return value.Value;
         }
 
+        internal TOptions GetOrAdd<TArg>(string? name, Func<string, TArg, TOptions> 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<TOptions>))
+#endif
+            {
+                // copying captured variables to locals avoids allocating a closure if we don't enter the if
+                string? localName = name;
+                Func<string, TArg, TOptions> localCreateOptions = createOptions;
+                TArg localFactoryArgument = factoryArgument;
+                return GetOrAdd(name, () => localCreateOptions(localName ?? Options.DefaultName, localFactoryArgument));
+            }
+
+#if NET || NETSTANDARD2_1
+            return _cache.GetOrAdd(
+                name ?? Options.DefaultName,
+                static (name, arg) => new Lazy<TOptions>(arg.createOptions(name, arg.factoryArgument)), (createOptions, factoryArgument)).Value;
+#endif
+        }
+
         /// <summary>
         /// Gets a named options instance, if available.
         /// </summary>
@@ -72,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<TOptions>(
-#if !NETSTANDARD2_1
+#if !(NET || NETSTANDARD2_1)
                 () =>
 #endif
                 options));

src/libraries/Microsoft.Extensions.Options/src/OptionsMonitor.cs

@@ -85,8 +85,17 @@ public TOptions CurrentValue
         /// </summary>
         public virtual TOptions Get(string? name)
         {
-            name = name ?? Options.DefaultName;
-            return _cache.GetOrAdd(name, () => _factory.Create(name));
+            if (_cache is not OptionsCache<TOptions> optionsCache)
+            {
+                // copying captured variables to locals avoids allocating a closure if we don't enter the if
+                string localName = name ?? Options.DefaultName;
+                IOptionsFactory<TOptions> localFactory = _factory;
+                return _cache.GetOrAdd(localName, () => localFactory.Create(localName));
+            }
+
+            // non-allocating fast path
+            return optionsCache.GetOrAdd(name, static (name, factory) => factory.Create(name), _factory);
+
         }
 
         /// <summary>

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,70 @@ 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<FakeOptions> CreateMonitor(IOptionsMonitorCache<FakeOptions> cache) =>
+                new OptionsMonitor<FakeOptions>(
+                    new OptionsFactory<FakeOptions>(Enumerable.Empty<IConfigureOptions<FakeOptions>>(), Enumerable.Empty<IPostConfigureOptions<FakeOptions>>()),
+                    Enumerable.Empty<IOptionsChangeTokenSource<FakeOptions>>(),
+                    cache);
+        }
+
+        private sealed class DerivedOptionsCache : OptionsCache<FakeOptions>
+        {
+            public int GetOrAddCalls { get; private set; }
+
+            public override FakeOptions GetOrAdd(string? name, Func<FakeOptions> createOptions)
+            {
+                GetOrAddCalls++;
+                return base.GetOrAdd(name, createOptions);
+            }
+        }
+
+        private sealed class ImplementedOptionsCache : IOptionsMonitorCache<FakeOptions>
+        {
+            public int GetOrAddCalls { get; private set; }
+
+            public void Clear() => throw new NotImplementedException();
+
+            public FakeOptions GetOrAdd(string? name, Func<FakeOptions> createOptions)
+            {
+                GetOrAddCalls++;
+                return createOptions();
+            }
+
+            public bool TryAdd(string? name, FakeOptions options) => throw new NotImplementedException();
+
+            public bool TryRemove(string? name) => throw new NotImplementedException();
+        }
+
+#if NET // need GC.GetAllocatedBytesForCurrentThread()
+        /// <summary>
+        /// Tests the fix for https://github.com/dotnet/runtime/issues/61086
+        /// </summary>
+        [Fact]
+        public void TestCurrentValueDoesNotAllocateOnceValueIsCached()
+        {
+            var monitor = new OptionsMonitor<FakeOptions>(
+                new OptionsFactory<FakeOptions>(Enumerable.Empty<IConfigureOptions<FakeOptions>>(), Enumerable.Empty<IPostConfigureOptions<FakeOptions>>()),
+                Enumerable.Empty<IOptionsChangeTokenSource<FakeOptions>>(),
+                new OptionsCache<FakeOptions>());
+            Assert.NotNull(monitor.CurrentValue); // populate the cache
+
+            long initialBytes = GC.GetAllocatedBytesForCurrentThread();
+            _ = monitor.CurrentValue;
+            Assert.Equal(0, GC.GetAllocatedBytesForCurrentThread() - initialBytes);
+        }
+#endif
     }
 }