Skip to content
29 changes: 26 additions & 3 deletions src/libraries/Microsoft.Extensions.Options/src/OptionsCache.cs
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,29 @@ 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.
if (GetType() != typeof(OptionsCache<TOptions>))
{
return GetOrAdd(name, () => createOptions(name ?? Options.DefaultName, factoryArgument));
Copy link
Member

Choose a reason for hiding this comment

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

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.

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 added a method to create indirection. Is that sufficient to fix?

Copy link
Member

@stephentoub stephentoub Mar 16, 2022

Choose a reason for hiding this comment

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

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

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. Which pattern do you prefer here?

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 have a strong preference. We've typically done the locals thing, though it's easier to "get it right" with a separate method.

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 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?

}

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.

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

return value.Value;
}

/// <summary>
/// Gets a named options instance, if available.
/// </summary>
Expand Down Expand Up @@ -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
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>
{
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>
{
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.

}
}