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.

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
38 changes: 34 additions & 4 deletions src/libraries/Microsoft.Extensions.Options/src/OptionsCache.cs
Original file line number Diff line number Diff line change
Expand Up @@ -31,14 +31,44 @@ 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))
{
value = _cache.GetOrAdd(name, new Lazy<TOptions>(createOptions));
Func<TOptions> localCreateOptions = createOptions;
Copy link
Member

Choose a reason for hiding this comment

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

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?

Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

@madelson madelson Mar 21, 2022

Choose a reason for hiding this comment

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

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

value = _cache.GetOrAdd(name, new Lazy<TOptions>(localCreateOptions));
}
#endif

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.
if (GetType() != typeof(OptionsCache<TOptions>))
{
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
}

name ??= Options.DefaultName;
Lazy<TOptions> value;

#if NET || NETSTANDARD2_1
value = _cache.GetOrAdd(name, static (name, arg) => new Lazy<TOptions>(arg.createOptions(name, arg.factoryArgument)), (createOptions, factoryArgument));
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 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?

#else
if (!_cache.TryGetValue(name, out value))
Copy link
Member

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

{
string? localName = name;
Func<string, TArg, TOptions> localCreateOptions = createOptions;
TArg localFactoryArgument = factoryArgument;
value = _cache.GetOrAdd(name, new Lazy<TOptions>(() => localCreateOptions(localName, localFactoryArgument)));
}
#endif

Expand Down Expand Up @@ -72,7 +102,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
Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

Copy link
Member

Choose a reason for hiding this comment

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

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

This could be

#if !(NET || NETSTANDARD2_1)

() =>
#endif
options));
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -85,8 +85,9 @@ 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));
return _cache is OptionsCache<TOptions> optionsCache
? optionsCache.GetOrAdd(name, (name, factory) => factory.Create(name), _factory)
: _cache.GetOrAdd(name ??= Options.DefaultName, () => _factory.Create(name));
}

/// <summary>
Expand Down
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,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<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 class DerivedOptionsCache : OptionsCache<FakeOptions>
madelson marked this conversation as resolved.
Show resolved Hide resolved
{
public int GetOrAddCalls { get; private set; }

public override FakeOptions GetOrAdd(string? name, Func<FakeOptions> createOptions)
{
GetOrAddCalls++;
return base.GetOrAdd(name, createOptions);
}
}

private class ImplementedOptionsCache : IOptionsMonitorCache<FakeOptions>
madelson marked this conversation as resolved.
Show resolved Hide resolved
{
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 (

[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

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

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.

}
}