IOptionsSnapshot is very slow

[NinoFloris]

This came up after a perf investigation on a recently refactored endpoint. Moving us back from IOptionsSnapshot to IOptionsMonitor knocked off ~100us, nothing else changed.

This option type of ours is fairly costly to create and looking into OptionsManager<TOptions> showed an obvious clue why we saw issues. OptionsManager does not just cache the instance for the duration of the DI scope but it also creates from scratch an instance through the factory per scope.

Looking through the docs this is vaguely alluded to (though there are contra-indicators) but after doing a search on SO and blogs on the subject most users seem to focus entirely on the 'you get a constant value during a scope/request' aspect of it.

As nobody (famous last words) seems to care whether it is entirely recreated my question would be:
Why doesn't OptionsManager<TOptions> cache the result of an IOptionsMonitor.Get call? (leaving out the backwards compatibility part for the sake of argument)

[ghost]

Tagging subscribers to this area: @maryamariyan
See info in area-owners.md if you want to be subscribed.

Issue Details

---

This came up after a perf investigation on a recently refactored endpoint. Moving us back from IOptionsSnapshot to IOptionsMonitor knocked off ~100us, nothing else changed.

This option type of ours is fairly costly to create, and looking into OptionsManager<TOptions> showed an obvious clue why we saw issues. OptionsManager does not just cache the instance for the duration of the DI scope but it also creates from scratch an instance through the factory per request.

Looking through the docs this is vaguely alluded to (though there are contra-indicators) but after doing a search on SO and blogs on the subject most users seem to focus entirely on the 'you get a constant value during a scope/request' aspect of it.

As nobody (famous last words) seems to care whether it is entirely recreated my question would be:
Why doesn't OptionsManager<TOptions> cache the result of an IOptionsMonitor.Get call? (leaving out the backwards compatibility part for the sake of argument)

Author:	NinoFloris
Assignees:	-
Labels:	area-Extensions-Options, tenet-performance, untriaged
Milestone:	-

[davidfowl]

Yes this is a known issue #42222. IOptionsSnapshot is currently a performance trap.

As nobody (famous last words) seems to care whether it is entirely recreated my question would be:
Why doesn't OptionsManager cache the result of an IOptionsMonitor.Get call? (leaving out the backwards compatibility part for the sake of argument)

Agreed that this implementation would have been more efficient. My only guess as to why it was done this way was because we had unified IOptions and IOptionsSnapsot implementations via the OptionsManager (though that's now changed in .NET 6).

The sad truth is that it would have been fixed if our own components were using it.

I'd be willing to take this change and see if there was any real impact not re-running factories per request. There are some very subtle differences but maybe worth the break.

cc @HaoK to see if he remembers why it was done this way.

[HaoK]

The original idea was:
IOptions is singleton and cached forever
IOptionsSnapshot is supposed to be scoped and recomputed per request (which is fine for some things, that don't involve config) (mostly exists for backcompat)
IOptionsMonitor is supposed to combine the ability to have options updates with caching semantics, change notifications and cache eviction

[HaoK]

If we are talking specifically about IOptionsSnapshot, that was intended to be really the no-caching you get a really new fresh instance on every scope, so it literally was intended to recompute everything on every request

[davidfowl]

Right. We should consider changing the implementation so that it caches the currently cached value so scoped services are a consistent view of the data but there's no need to recompute if nothing changes

[NinoFloris]

@davidfowl I see it's been placed in 7.0 is a PR for 6.0 still welcome?

[davidfowl]

Yes, if you send a PR, it can be done without adding public API (I believe).

[NinoFloris]

I agree, alright will be cooking it up.

[NinoFloris]

@davidfowl started work on it just now, practically the change is extremely simple * if * OptionsManager can take IOptionsMonitor instead of IOptionsFactory. This does mean an api compat error for the constructor, what's the policy on those? Alternatively I could duplicate the OptionsMonitor code into OptionsManager as that's as good as it gets with access to just IOptionsFactory.

runtime/src/libraries/Microsoft.Extensions.Options/src/OptionsManager.cs

Lines 17 to 27 in 57bfe47

	private readonly IOptionsFactory<TOptions> _factory;
	private readonly OptionsCache<TOptions> _cache = new OptionsCache<TOptions>(); // Note: this is a private cache
	/// <summary>
	/// Initializes a new instance with the specified options configurations.
	/// </summary>
	/// <param name="factory">The factory to use to create options.</param>
	public OptionsManager(IOptionsFactory<TOptions> factory)
	{
	_factory = factory;
	}

Ideally I'd also inline that silly cache (which is only used in OptionsManager) but it's a public type so I guess that's a no ;)

[davidfowl]

Or just make a new internal type so we don't need to change the public surface here. Call it OptionsSnapshot<T>

[pentp]

This should be re-opened since it was reverted in #57570?

[eerhardt]

Yes - good call, I had forgotten.

[davidfowl]

So the performance issue is by design but the thing that can be cached here is the parsing of configuration. We should be able to only read that once until it changes, VS reading it once per request. The scoped IConfigureOptions<T> need to run per call, that's by design and can use arbitrary scoped services to change the options value.

[NinoFloris]

Without doing some hacky diffing how would you propose to request just the IConfigureOptions that have a scoped registration?

[NinoFloris]

@davidfowl that was an honest question, I would like to fix it up but suggestions are welcome.

[dmitriyse]

IMHO the root problem of the interface IOptionsSnapshot<T> that it's suitable for multiple scenarios:

1. It allows to fix configuration state at the beginning of request and be consistent among scoped services.
2. It allows to write clean code, if it enough for us to get options only time per lifetime scope and don't track changes during the scope lifetime, we can prefer IOptionsShapshot<T> over IOptionsMonitor<T> to express it.
3. It allows to recalculate options always in each scope. Only this scenario creates the performance problem as we unable to cache the value between scopes.

Documentation states that IOptionsSnapshot<T> at once suitable for all scenarios,
We can keep all this without changes.

if developer know that each subsequent recalculation will give the same result (in between of configuration change events), then he can enable caching and it will be an optimization.

We can at the time of services registration explicitly allow caching of the IOptionsSnapshot<T> among scopes.

Implementation idea:
The registration logic could be extended.

        public static IServiceCollection Configure<TOptions>(this IServiceCollection services, string name, IConfiguration config, Action<BinderOptions> configureBinder, 
           bool allowCaching // New argument
)
            where TOptions : class
        {
            if (services == null)
            {
                throw new ArgumentNullException(nameof(services));
            }

            if (config == null)
            {
                throw new ArgumentNullException(nameof(config));
            }

            services.AddOptions()
            services.AddSingleton<IOptionsChangeTokenSource<TOptions>>(new ConfigurationChangeTokenSource<TOptions>(name, config));
            
            if (allowCaching)
            {
                 // Hope DI supports overriding of open generics by specific registrations.
                 services.AddScoped<IOptionsSnapshot<T>, OptionsSnapshotCached<T>>();
            }
            return services.AddSingleton<IConfigureOptions<TOptions>>(new NamedConfigureFromConfigurationOptions<TOptions>(name, config, configureBinder));
        }
        
    // Just an idea, final code should be well implemented with various optimizations
    public class OptionsSnapshotCached<T>: IOptionsSnapshot<T>
    {
        private readonly ConcurrentDictionary<string, T> _scopeCache = new (); 
        private readonly IOptionsMonitor<T> _optionsMonitor;
        public OptionsSnapshotCached(IOptionsMonitor<T> optionsMonitor)
        {
             _optionsMonitor = optionsMonitor;
        }
        public T Value => _scopeCache.GetOrAdd(string.Empty, _ =>_optionsMonitor.Value);
        public T Get(string name) => _scopeCache.GetOrAdd(name, key => _optionsMonitor.Get(key));
    }

[tompazourek]

So the performance issue is by design but the thing that can be cached here is the parsing of configuration. We should be able to only read that once until it changes, VS reading it once per request.

However, there's a possible issue. Parsing of the configuration applies the configuration values to an existing TOptions instance. And it's possible that the existing TOptions instance is different each time depending on which scoped IConfigureOptions services were executed before the configuration-binding IConfigureOptions. I'm not sure which part can actually be cached given how the configuration binding currently works. Maybe if the entire configuration binding would be rewritten (something like #36130) so it's blazing fast and has some sort of caching inside of it, then this whole issue with IOptionsSnapshot being slow would disappear. But as the configuration binding is currently written, I'm not sure what's there to cache...

The scoped IConfigureOptions need to run per call, that's by design and can use arbitrary scoped services to change the options value.

To be honest, I don't like that IOptionsSnapshot<T> has this hidden by-design difference from IOptionsMonitor<T> very much. I think it would be much simpler to understand if it was just a more limited version of IOptionsMonitor<T>. Just something that you use if you don't need the full monitoring features, not something that sometimes applies changes that even the supposedly 'more powerful" IOptionsMonitor<T> doesn't detect. But on the other hand, I cannot tell if you would be willing to accept a breaking change like that (based on the reverted PR, probably not?).

If it's really by design, it should at least be documented (maybe we can contribute with a documentation update PR?) as this behavior is not obvious.

[deeprobin]

Have you profiled where the performance bottle-neck is?

[deeprobin]

Some micro-optimization/workaround would be not using anonymous methods, because they are currently not optimized by jit ¹.

runtime/src/libraries/Microsoft.Extensions.Options/src/OptionsManager.cs

Line 46 in 57bfe47

options = _cache.GetOrAdd(name, () => localFactory.Create(localName));

Footnotes

1. Also some case not using delegates: https://github.com/dotnet/runtime/issues/61086 ↩

[deeprobin]

OptionsManager.Get seems fine...

The bottleneck is the ConcurrentDictionary

[tompazourek]

@deeprobin I think the bottleneck is this: #33954. And if we use IOptionsSnapshot, it runs on every request. If we use IOptionsMonitor, it runs once, and then it's cached until the configuration is changed (and reloaded). I encourage you to try this with IConfiguration that contains several thousand key-value pairs (and nesting, etc.)

[davidfowl]

@tompazourek is right. The problem is the rebinding per request.

[Blackclaws]

There is also a difference in behavior if your code doesn't properly track what causes configuration to change.

You can write something like this:

class TestOptionsConfigure : IConfigureOptions<TestOptions>
{
    private IConfiguration _root;
    private ILogger<TestOptionsConfigure> _logger;
    public TestOptionsConfigure(IConfiguration root, ILogger<TestOptionsConfigure> logger)
    {
        _root = root;
        _logger = logger;
        _logger.LogError("Constructed");
    }
    
    public void Configure(TestOptions options)
    {
        var sec = _root.GetSection("Test");
        if (sec != null)
        {
            options.Val = sec["Value"];
        }
        _logger.LogError("Configured");
    }
}

If you add this to the service collection, it will get called each time for IOptionsSnapshot but only once for IOptionsMonitor, even if you change the configuration underneath.

IOptionsMonitor only gets notified of changes if you actually bind configuration on adding options, not if you just use IConfigureOptions classes.

[profet23]

Has there been any movement on this?

This is a significant pain point.

My naïve take is that whatever is building the IOptionSnapshots should just use IOptionsMonitor internally and only re-bind when a change event occurs. So at least then MOST requests wouldn't have to rebind.

[dmitriyse]

@profet23, IOptionsSnapshot also is used for the scenario when each request (scope) can have different version (state) of the options. That why it was decided to be always re-populated from IConfiguration on each request. So the mapping logic can respect the context of the request (scope).

[dmitriyse]

Yes it would be great to have the official support of both: per-context state + caching (probably with custom key calculated from the request). And yest the cache should be invalidated once the configuration changes.

[davidfowl]

IOptionsSnapshot can't use IOptionsMonitor. In fact, we tried to do that in .NET 7 and it is a major breaking change. IOptionsSnapshot supports injecting scoped IConfigureOptions<T>. That's the problem.

The combination of IOptionsSnapshot with uncached configuration binding is a performance pit of failure worth investigating. Specifically, the problem is that calling:

[davidfowl]

To do anything useful here we'd need something like the API I proposed in #36130 (comment). This is essentially what the new configuration source generator does today so it's possible that is a reasonable mitigation for this.

[tompazourek]

To do anything useful here we'd need something like the API I proposed in #36130 (comment). This is essentially what the new configuration source generator does today so it's possible that is a reasonable mitigation for this.

This is a similar solution that I implemented as a workaround for issue in our code. I ended up writing an extension to provide a custom binder, and wrote some binders by hand that were written for the exact types used. It performed very well.

A source generator that will generate similar code as the one I wrote by hand sounds like a very good solution to me.

[profet23]

@davidfowl I understand that this isn't a one size all solution and that this does not behave 1:1 with the current implementation of IOptionsSnapshot, but I am curious how much would break with this:

	public static IServiceCollection AddFastOptions(this IServiceCollection services)
	{
		var descriptor = services.Single(descriptor => descriptor.ServiceType == typeof(IOptionsSnapshot<>));
		services.Remove(descriptor);

		services.AddScoped(typeof(IOptionsSnapshot<>), typeof(FastOptionsSnapshot<>));

		return services;
	}

	public class FastOptionsSnapshot<TOptions> : IOptionsSnapshot<TOptions> where TOptions : class
	{
		private readonly IOptionsMonitor<TOptions> monitor;
		private readonly ConcurrentDictionary<string, TOptions> namedValuesDictionary = new ConcurrentDictionary<string, TOptions>();

		public FastOptionsSnapshot(IOptionsMonitor<TOptions> monitor)
		{
			this.monitor = monitor;
			this.Value = monitor.CurrentValue;
		}

		public TOptions Value { get; }

	        public TOptions Get(string name)
	        {
		        name ??= Options.DefaultName;
        
		        namedValuesDictionary.TryAdd(name, monitor.Get(name));
        
		        return namedValuesDictionary[name];
	        }
	}

Again, understanding that this maintains the same TOptions for each scope lifetime. But does not maintain that the entire configuration is the same for each scope.

That said, this is a significant performance boost. Between 200 and 600ms per request (for my use case).

EDIT: Updated the non relevant Dictionary snippet because people keep talking about it, and not the actual issue.

[deeprobin]

@profet23 Your FastOptionsSnapshot uses Dictionary instead of ConcurrentDictionary. However, you are locking the dictionary and therefore I don't think there should be a problem from a concurrency point of view.

I just wonder if the tide turns with the total number of options. This should be analyzed.

[davidfowl]

#56271

[KalleOlaviNiemitalo]

@profet23, your code lets a thread read the dictionary without locking, while another thread is writing. That combination is not safe for System.Collections.Generic.Dictionary<TKey, TValue>, although it is safe for System.Collections.Hashtable.

[profet23]

#56271

@davidfowl Are there any linkable examples of the following?

I think we're going to have to revert this PR. It breaks scoped services contributing to the value of options snapshots (I now see the tests were changed here to account for those changes).

[davidfowl]

This deleted line needs to continue working.

[profet23]

This isn't a fix, but more of a workaround for the current "performance pit of failure".
This implementation is aware that using IOptionsMonitor is not possible for scoped IConfiguredOptions.

public static IServiceCollection AddFastOptions(this IServiceCollection services)
{
	var descriptor = services.Single(descriptor => descriptor.ServiceType == typeof(IOptionsSnapshot<>));
	services.Remove(descriptor); 
	services.Add(new ServiceDescriptor(descriptor.ImplementationType!, descriptor.ImplementationType!, descriptor.Lifetime));
  
	services.AddScoped(typeof(IOptionsSnapshot<>), typeof(FastOptionsSnapshot<>));
  
	return services;
}

public class FastOptionsSnapshot<TOptions> : IOptionsSnapshot<TOptions> where TOptions : class
{
	private readonly IServiceProvider serviceProvider;
	private readonly IOptionsMonitor<TOptions>? monitor;
	private readonly ConcurrentDictionary<string, TOptions> namedValuesDictionary = new ConcurrentDictionary<string, TOptions>();

	public FastOptionsSnapshot(IServiceProvider serviceProvider)
	{
		this.serviceProvider = serviceProvider;

		try
		{
			monitor = serviceProvider.GetService(typeof(IOptionsMonitor<TOptions>)) as IOptionsMonitor<TOptions>;
		}
		catch (InvalidOperationException)
		{
			// Swallow the exception and continue without the monitor.
			// This means that the type contains at least one scoped option and we'll need to fall back to OptionsManager (slow) later.
		}
	}

	public TOptions Value => Get(null);

	public TOptions Get(string? name)
	{
		name ??= Options.DefaultName;

		var value =  monitor?.Get(name) ?? ((OptionsManager<TOptions>) serviceProvider.GetRequiredService(typeof(OptionsManager<TOptions>))).Get(name);

		namedValuesDictionary.TryAdd(name, value);

		return namedValuesDictionary[name];
	}
}

Idea being, use the IOptionsMonitor value when possible, but fallback to the existing OptionsManager (slow) when a scoped option exists.

Again, this is a bit hacky, but the performance of the default OptionsManager is such that I think it is worth the added complexity.

[davidfowl]

Yes it’s a workaround that we can’t ship but you can use in your apps if they don’t hit this scenario

[profet23]

Yes it’s a workaround that we can’t ship but you can use in your apps if they don’t hit this scenario

Understood.

But if others find it useful, I've published my workaround:

https://github.com/sermo/FastOptions

[deeprobin]

Is there anything we can optimize in ConcurrentDictionary?

The code looks quite complex.
But probably complex enough to support access from multiple threads.

[davidfowl]

@deeprobin what is it that you’re attempting to fix?

[renrutsirhc]

@NinoFloris @davidfowl How slow is very slow? I'm currently troubleshooting a performance issue in an application that seems to point to IOptionsSnapshot being passed into the InvokeAsync method in a custom middleware component. We are seeing Waiting times of 30 seconds or more for Framework/Library Lazy`1.CreateValue in application insights profiler on some requests while others sail through just fine. Could this performance issue be attributed to this issue with IOptionsSnapshot or is this orders of magnitude slower than what has been discussed here previously?