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

madelson · 2022-03-16T00:17:22Z

ghost · 2022-03-16T00:17:30Z

Tagging subscribers to this area: @dotnet/area-extensions-options
See info in area-owners.md if you want to be subscribed.

Issue Details

Fix #61086

Author:	madelson
Assignees:	-
Labels:	`area-Extensions-Options`
Milestone:	-

madelson · 2022-03-16T00:20:40Z

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


-#if NETSTANDARD2_1
+#if NET || NETSTANDARD2_1


Given that we could use TryGetValue in all frameworks, I assume that it is desirable to use GetOrAdd where we can. This way we'll use it for the newer .NET targets as well.

madelson · 2022-03-16T00:23:24Z

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

+            Lazy<TOptions> value;
+
+#if NET || NETSTANDARD2_1
+            value = _cache.GetOrAdd(name, static (name, arg) => new Lazy<TOptions>(arg.createOptions(name, arg.factoryArgument)), (createOptions, factoryArgument));


This would be less awkward if we weren't using Lazy. From what I can tell the benefit we're getting from that is ensuring that we never create an options instance that doesn't end up getting used. Is that worth it?

madelson · 2022-03-16T00:24:06Z

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

@@ -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<TOptions>(
-#if !NETSTANDARD2_1
+#if NETSTANDARD2_0 || NETFRAMEWORK


Using the non-delegate constructor whenever we can feels like a win.

stephentoub · 2022-03-16T00:26:10Z

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

+            // For compatibility, fall back to public GetOrAdd() if we're in a derived class.
+            if (GetType() != typeof(OptionsCache<TOptions>))
+            {
+                return GetOrAdd(name, () => createOptions(name ?? Options.DefaultName, factoryArgument));


Note that even though this is guarded behind an if, it's still going to force a closure allocation at the beginning of the method for all uses due to the createOptions parameter being captured.

I added a method to create indirection. Is that sufficient to fix?

You can do that, or you can just create a local inside the scope where you want the closure allocated and capture that local, e.g.

if (GetType() != typeof(OptionsCache<TOptions>)) { string? localName = name; Func<TOptions> localCreateOptions = createOptions; TArg localFactoryArgument = factoryArgument; return GetOrAdd(name, () => localCreateOptions(localName ?? Options.DefaultName, localFactoryArgument)); }

Makes sense. Which pattern do you prefer here?

I don't have a strong preference. We've typically done the locals thing, though it's easier to "get it right" with a separate method.

It looks like the legacy framework branches should have the same issue because of the delegate passed to the Lazy constructor in the cache miss case. Do you think it is worth fixing for those as well?

madelson · 2022-03-16T00:27:57Z

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

@@ -85,8 +85,9 @@ public TOptions CurrentValue
        /// </summary>
        public virtual TOptions Get(string? name)
        {
-            name = name ?? Options.DefaultName;


This technically isn't necessary because the cache API accepts null. For compat, I left this logic on the branch where we are dealing with an unknown cache implementation.

madelson · 2022-03-16T00:40:36Z

.../Microsoft.Extensions.Options/tests/Microsoft.Extensions.Options.Tests/OptionsMonitorTest.cs

+            public bool TryAdd(string? name, FakeOptions options) => throw new NotImplementedException();
+
+            public bool TryRemove(string? name) => throw new NotImplementedException();
+        }


Are there standard patterns for testing the behavior that a cached Get() doesn't allocate?

I doubt there is a commonly used behavior for this.
But if this is important I would suggest that within a NoGC region you measure the allocated bytes before the get call and after the get call (

runtime/src/coreclr/System.Private.CoreLib/src/System/GC.cs

Lines 356 to 357 in 59b5585

[MethodImpl(MethodImplOptions.InternalCall)]

public static extern long GetAllocatedBytesForCurrentThread();

).

Example

void TestMethod() { Assert.True(GC.TryStartNoGCRegion()); long allocatedBytesBefore = GC.GetAllocatedBytesForCurrentThread(); // my cool operation (Get) long allocatedBytesAfter = GC.GetAllocatedBytesForCurrentThread(); Assert.Equal(allocatedBytesBefore, allocatedBytesAfter); GC.EndNoGCRegion(); }

How reliable this allocated bytes count is I can't tell you though. Probably someone from the GC area would have to tell you.

/cc @Maoni0

This seems like a good approach assuming that the allocated byte count is reliable.

I could be wrong but I don't think we actually need the NoGC region because GetAllocatedBytesForCurrentThread() does not go down after a garbage collection (it returns the number of bytes that have ever been allocated). Does that sound right?

Actually, wouldn't it make sense to put an AssertExtension for the allocations into the TestUtilities?
Is there anything against it? I could imagine that this will be a test pattern more often in the future.

Assert.NoAllocations(() => _ = monitor.CurrentValue);

@deeprobin this seems straightforward to add to https://github.com/dotnet/runtime/blob/main/src/libraries/Common/tests/TestUtilities/System/AssertExtensions.cs if you think that's useful. I don't know what the guidelines are for adding new methods there.

In the snippet you show I don't think we could use static because you're capturing monitor in the closure. However, I don't think that matters since the method would check for allocations when calling the provided Action, not from creating it.

Anyway, let me know if you'd like me to move this to be a TestExtensions helper!

In the snippet you show I don't think we could use static because you're capturing monitor in the closure. However, I don't think that matters since the method would check for allocations when calling the provided Action, not from creating it.

You're right.

this seems straightforward to add to https://github.com/dotnet/runtime/blob/main/src/libraries/Common/tests/TestUtilities/System/AssertExtensions.cs if you think that's useful. I don't know what the guidelines are for adding new methods there.

I think there are hardly any guidelines (except the style guidelines).

Anyway, let me know if you'd like me to move this to be a TestExtensions helper!

In my opinion, of course, that would make sense but I might wait for an okay from a maintainer of the repository.
@stephentoub What do you think?

I think it would be OK to add an AssertExtension for this functionality. It would be useful in other places. It doesn't have to happen in this PR though. It could happen the next time we need it.

It could happen the next time we need it.

Makes sense. Rule of three and all that.

deeprobin · 2022-03-19T00:33:47Z

.../Microsoft.Extensions.Options/tests/Microsoft.Extensions.Options.Tests/OptionsMonitorTest.cs

+            public bool TryAdd(string? name, FakeOptions options) => throw new NotImplementedException();
+
+            public bool TryRemove(string? name) => throw new NotImplementedException();
+        }


I doubt there is a commonly used behavior for this.
But if this is important I would suggest that within a NoGC region you measure the allocated bytes before the get call and after the get call (

runtime/src/coreclr/System.Private.CoreLib/src/System/GC.cs

Lines 356 to 357 in 59b5585

[MethodImpl(MethodImplOptions.InternalCall)]

public static extern long GetAllocatedBytesForCurrentThread();

).

Example

void TestMethod() { Assert.True(GC.TryStartNoGCRegion()); long allocatedBytesBefore = GC.GetAllocatedBytesForCurrentThread(); // my cool operation (Get) long allocatedBytesAfter = GC.GetAllocatedBytesForCurrentThread(); Assert.Equal(allocatedBytesBefore, allocatedBytesAfter); GC.EndNoGCRegion(); }

How reliable this allocated bytes count is I can't tell you though. Probably someone from the GC area would have to tell you.

/cc @Maoni0

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

.../Microsoft.Extensions.Options/tests/Microsoft.Extensions.Options.Tests/OptionsMonitorTest.cs

…otnet#61086.

madelson · 2022-03-20T16:38:46Z

.../Microsoft.Extensions.Options/tests/Microsoft.Extensions.Options.Tests/OptionsMonitorTest.cs

+            long initialBytes = GC.GetAllocatedBytesForCurrentThread();
+            _ = monitor.CurrentValue;
+            Assert.Equal(0, GC.GetAllocatedBytesForCurrentThread() - initialBytes);
+        }


I think having this test is pretty helpful. Writing it helped me catch one more rogue closure allocation, and this should help prevent regression.

eerhardt · 2022-03-21T15:43:49Z

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

@@ -72,7 +105,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 NETSTANDARD2_0 || NETFRAMEWORK


(nit) can we keep the #if checks consistent in this file?

This could be

#if !(NET || NETSTANDARD2_1)

eerhardt · 2022-03-21T16:01:59Z

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

            value = _cache.GetOrAdd(name, static (name, createOptions) => new Lazy<TOptions>(createOptions), createOptions);
 #else
            if (!_cache.TryGetValue(name, out value))
            {
-                value = _cache.GetOrAdd(name, new Lazy<TOptions>(createOptions));
+                // copying captured variables to locals avoids allocating a closure if we don't enter the if
+                Func<TOptions> localCreateOptions = createOptions;


I don't understand what is allocating a closure here. new Lazy<TOptions>(createOptions) will always create the Lazy object. But what is creating the closure?

There's no closure here. The workaround used elsewhere isn't needed here.

Yep I misread that this was not using the factory overload of GetOrAdd. Will fix.

eerhardt · 2022-03-21T16:09:38Z

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

+#if NET || NETSTANDARD2_1
+            value = _cache.GetOrAdd(name, static (name, arg) => new Lazy<TOptions>(arg.createOptions(name, arg.factoryArgument)), (createOptions, factoryArgument));
+#else
+            if (!_cache.TryGetValue(name, out value))


On .NET Framework and netstandard2.0, why don't we just fall back to the public GetOrAdd() as well? I don't think we need to try making a fast-path outside of .NETCoreApp. Especially considering how complicated this method is getting.

Makes sense. Wasn't sure how much we should care about this sort of thing on the older frameworks.

eerhardt

I think this looks good. Thanks for the contribution, @madelson.

Any other feedback?

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

121124c

Fix dotnet#61086

dotnet-issue-labeler bot added the area-Extensions-Options label Mar 16, 2022

ghost added the community-contribution Indicates that the PR has been added by a community member label Mar 16, 2022

madelson commented Mar 16, 2022

View reviewed changes

stephentoub reviewed Mar 16, 2022

View reviewed changes

madelson commented Mar 16, 2022

View reviewed changes

Address feedback from dotnet#66688 (comment)

9093586

madelson commented Mar 16, 2022

View reviewed changes

madelson marked this pull request as ready for review March 16, 2022 12:15

runfoapp bot mentioned this pull request Mar 16, 2022

ThreadPoolTests.CooperativeBlockingCanCreateThreadsFaster timed out #64964

Closed

Merge remote-tracking branch 'origin/main' into issue-61086

780fe23

madelson requested a review from stephentoub March 18, 2022 17:30

madelson marked this pull request as draft March 18, 2022 17:31

Use locals to avoid unnecessary closure allocations.

82ed153

madelson marked this pull request as ready for review March 18, 2022 17:58

runfoapp bot mentioned this pull request Mar 18, 2022

Threadpool test CooperativeBlockingCanCreateThreadsFaster failing on Mac #66852

Closed

deeprobin reviewed Mar 19, 2022

View reviewed changes

madelson added 2 commits March 19, 2022 14:18

Implement feedback from dotnet#66688

83a98a9

Remove another closure allocation in OptionsMonitor and add test for d…

e0e46bb

…otnet#61086.

madelson commented Mar 20, 2022

View reviewed changes

eerhardt reviewed Mar 21, 2022

View reviewed changes

Address feedback from dotnet#66688

32820fb

madelson requested a review from eerhardt March 22, 2022 22:59

eerhardt approved these changes Mar 28, 2022

View reviewed changes

eerhardt merged commit 670262a into dotnet:main Mar 29, 2022

ghost locked as resolved and limited conversation to collaborators Apr 28, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

madelson commented Mar 16, 2022

ghost commented Mar 16, 2022

madelson Mar 16, 2022

madelson Mar 16, 2022

madelson Mar 16, 2022

stephentoub Mar 16, 2022

madelson Mar 16, 2022

stephentoub Mar 16, 2022 •

edited

Loading

madelson Mar 16, 2022

stephentoub Mar 16, 2022

madelson Mar 16, 2022

madelson Mar 16, 2022

madelson Mar 16, 2022

deeprobin Mar 19, 2022

madelson Mar 19, 2022 •

edited

Loading

deeprobin Mar 20, 2022 •

edited

Loading

madelson Mar 21, 2022

deeprobin Mar 21, 2022

eerhardt Mar 21, 2022

madelson Mar 22, 2022

deeprobin Mar 19, 2022

madelson Mar 20, 2022

eerhardt Mar 21, 2022

eerhardt Mar 21, 2022

stephentoub Mar 21, 2022

madelson Mar 21, 2022 •

edited

Loading

eerhardt Mar 21, 2022

madelson Mar 22, 2022

eerhardt left a comment

	[MethodImpl(MethodImplOptions.InternalCall)]
	public static extern long GetAllocatedBytesForCurrentThread();

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

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

Conversation

madelson commented Mar 16, 2022

ghost commented Mar 16, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

stephentoub Mar 16, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Example

madelson Mar 19, 2022 • edited Loading

Choose a reason for hiding this comment

deeprobin Mar 20, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Example

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

madelson Mar 21, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

eerhardt left a comment

Choose a reason for hiding this comment

stephentoub Mar 16, 2022 •

edited

Loading

madelson Mar 19, 2022 •

edited

Loading

deeprobin Mar 20, 2022 •

edited

Loading

madelson Mar 21, 2022 •

edited

Loading