Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Merged
merged 7 commits into from
Mar 29, 2022
Merged

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

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
121124c
Avoid allocating a delegate in OptionsMonitor.Get() when possible.
madelson Mar 16, 2022
9093586
Address feedback from https://github.com/dotnet/runtime/pull/66688#di…
madelson Mar 16, 2022
780fe23
Merge remote-tracking branch 'origin/main' into issue-61086
madelson Mar 18, 2022
82ed153
Use locals to avoid unnecessary closure allocations.
madelson Mar 18, 2022
83a98a9
Implement feedback from https://github.com/dotnet/runtime/pull/66688
madelson Mar 19, 2022
e0e46bb
Remove another closure allocation in OptionsMonitor and add test for …
madelson Mar 19, 2022
32820fb
Address feedback from https://github.com/dotnet/runtime/pull/66688
madelson Mar 22, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
28 changes: 25 additions & 3 deletions src/libraries/Microsoft.Extensions.Options/src/OptionsCache.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

value = _cache.GetOrAdd(name, static (name, createOptions) => new Lazy<TOptions>(createOptions), createOptions);
#else
if (!_cache.TryGetValue(name, out value))
Expand All @@ -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));
madelson marked this conversation as resolved.
Show resolved Hide resolved
}

#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>
Expand Down Expand Up @@ -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));
Expand Down
13 changes: 11 additions & 2 deletions src/libraries/Microsoft.Extensions.Options/src/OptionsMonitor.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -85,8 +85,17 @@ public TOptions CurrentValue
/// </summary>
public virtual TOptions Get(string? name)
{
name = name ?? Options.DefaultName;
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

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>
Expand Down
66 changes: 66 additions & 0 deletions ...crosoft.Extensions.Options/tests/Microsoft.Extensions.Options.Tests/OptionsMonitorTest.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,7 @@

using System;
using System.Collections.Generic;
using System.Linq;
using System.Threading;
using Microsoft.Extensions.Configuration;
using Microsoft.Extensions.DependencyInjection;
Expand Down Expand Up @@ -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();
}
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

@madelson madelson Mar 19, 2022 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Contributor

@deeprobin deeprobin Mar 20, 2022 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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);

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@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!

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It could happen the next time we need it.

Makes sense. Rule of three and all that.


#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);
}
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

#endif
}
}