-
Notifications
You must be signed in to change notification settings - Fork 5.4k
Optimize IOptionsMonitor.CurrentValue with thread-safe field caching and generation-based invalidation #125650
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
base: main
Are you sure you want to change the base?
Changes from all commits
2f9595d
e28a825
61593cd
055c432
f8e2be6
10feca2
5fceb85
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -4,6 +4,8 @@ | |
| using System; | ||
| using System.Collections.Generic; | ||
| using System.Diagnostics.CodeAnalysis; | ||
| using System.Runtime.CompilerServices; | ||
| using System.Threading; | ||
| using Microsoft.Extensions.Primitives; | ||
|
|
||
| namespace Microsoft.Extensions.Options | ||
|
|
@@ -18,9 +20,12 @@ public class OptionsMonitor<[DynamicallyAccessedMembers(Options.DynamicallyAcces | |
| where TOptions : class | ||
| { | ||
| private readonly IOptionsMonitorCache<TOptions> _cache; | ||
| private readonly OptionsCache<TOptions>? _fastCache; | ||
| private readonly IOptionsFactory<TOptions> _factory; | ||
| private readonly List<IDisposable> _registrations = new List<IDisposable>(); | ||
| internal event Action<TOptions, string>? _onChange; | ||
| private TOptions? _currentValue; | ||
| private int _currentValueGeneration; | ||
|
|
||
| /// <summary> | ||
| /// Initializes a new instance of <see cref="OptionsMonitor{TOptions}"/> with the specified factory, sources, and cache. | ||
|
|
@@ -32,6 +37,7 @@ public OptionsMonitor(IOptionsFactory<TOptions> factory, IEnumerable<IOptionsCha | |
| { | ||
| _factory = factory; | ||
| _cache = cache; | ||
| _fastCache = cache as OptionsCache<TOptions>; | ||
|
|
||
| void RegisterSource(IOptionsChangeTokenSource<TOptions> source) | ||
| { | ||
|
|
@@ -76,7 +82,42 @@ private void InvokeChanged(string? name) | |
| /// <exception cref="MissingMethodException">The <typeparamref name="TOptions"/> does not have a public parameterless constructor or <typeparamref name="TOptions"/> is <see langword="abstract"/>.</exception> | ||
| public TOptions CurrentValue | ||
| { | ||
| get => Get(Options.DefaultName); | ||
| get | ||
| { | ||
| OptionsCache<TOptions>? fastCache = _fastCache; | ||
| if (fastCache is null) | ||
| { | ||
| // User-supplied IOptionsMonitorCache: no generation tracking, always go through Get. | ||
| return Get(Options.DefaultName); | ||
| } | ||
|
|
||
| int gen = fastCache.Generation; | ||
| // Read generation before value. RefreshCurrentValue writes _currentValue before | ||
| // _currentValueGeneration (release), so an acquire-load of _currentValueGeneration | ||
| // here guarantees the subsequent read of _currentValue sees the matching published | ||
| // value and not a stale one. | ||
| int cachedGen = Volatile.Read(ref _currentValueGeneration); | ||
| TOptions? value = Volatile.Read(ref _currentValue); | ||
| if (value is not null && cachedGen == gen) | ||
| { | ||
| return value; | ||
| } | ||
|
|
||
| return RefreshCurrentValue(gen); | ||
| } | ||
| } | ||
|
|
||
| [MethodImpl(MethodImplOptions.NoInlining)] | ||
| private TOptions RefreshCurrentValue(int gen) | ||
| { | ||
| TOptions value = Get(Options.DefaultName); | ||
| // Write value before generation: the generation acts as a release signal that | ||
| // _currentValue is ready. A reader that acquire-loads _currentValueGeneration and | ||
| // sees gen is then guaranteed (via the release/acquire pairing) to observe the | ||
| // _currentValue written here. | ||
| Volatile.Write(ref _currentValue, value); | ||
|
rosebyte marked this conversation as resolved.
|
||
| Volatile.Write(ref _currentValueGeneration, gen); | ||
|
Comment on lines
+118
to
+119
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. AFAICT, this code is correct. But isn't (But safety is of course more important than small performance improvement, so if we're not sure, an extra |
||
| return value; | ||
| } | ||
|
|
||
| /// <summary> | ||
|
|
@@ -88,7 +129,7 @@ public TOptions CurrentValue | |
| /// <exception cref="MissingMethodException">The <typeparamref name="TOptions"/> does not have a public parameterless constructor or <typeparamref name="TOptions"/> is <see langword="abstract"/>.</exception> | ||
| public virtual TOptions Get(string? name) | ||
| { | ||
| if (_cache is not OptionsCache<TOptions> optionsCache) | ||
| if (_fastCache is null) | ||
| { | ||
| // copying captured variables to locals avoids allocating a closure if we don't enter the if | ||
| string localName = name ?? Options.DefaultName; | ||
|
|
@@ -97,8 +138,7 @@ public virtual TOptions Get(string? name) | |
| } | ||
|
|
||
| // non-allocating fast path | ||
| return optionsCache.GetOrAdd(name, static (name, factory) => factory.Create(name), _factory); | ||
|
|
||
| return _fastCache.GetOrAdd(name, static (name, factory) => factory.Create(name), _factory); | ||
| } | ||
|
|
||
| /// <summary> | ||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nit: is it worth adding this field? Repeating a cast is probably cheaper than adding this field.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It was copilot's idea and by benchmarks it seems having a slight edge (judged by the flow where we get with default options name) but I'll add a direct benchmark to be sure.