Change IOptionsSnapshot to reuse options instances across scopes #56271
Conversation
ghost
added
the
community-contribution
|
Tagging subscribers to this area: @maryamariyan Issue DetailsCloses #53793 I decided to keep the cache as a ConcurrentDictionary to head off problematic compatibility issues from switching out the underlying implementation. I'm not clear on the semantics of a DI scope regardless, maybe it's ok to use one in a concurrent fashion? For the cache I picked capacity 5 for the 99% case, apparently 31 is the default capacity of a ConcurrentDictionary and what was specified in OptionsCache. Capacity 31 sounds fairly wasteful to new up every request but on the flip side you'd get a tiny chance of resizes I guess 🙃 OptionsManager and OptionsCache are effectively dead code now. Something to mark as obsolete in a future version by the api review team?
|
|
As this type could be constructed per request I've pushed a commit that has a separate path for unnamed options, skipping allocation of the dictionary entirely if just unnamed options are used. |
NinoFloris
force-pushed
the
fix/ioptionssnapshot
branch
from
4733049
to
6fd7f10
Compare
src/libraries/Microsoft.Extensions.Options/src/OptionsSnapshot.cs
Outdated
Show resolved
Hide resolved
src/libraries/Microsoft.Extensions.Options/src/OptionsSnapshot.cs
Outdated
Show resolved
Hide resolved
...Microsoft.Extensions.Options/tests/Microsoft.Extensions.Options.Tests/OptionsSnapshotTest.cs
Show resolved
Hide resolved
|
@eerhardt thanks for your review, I've pushed a commit addressing it! |
NinoFloris
force-pushed
the
fix/ioptionssnapshot
branch
from
58eb65c
to
2345495
Compare
NinoFloris
force-pushed
the
fix/ioptionssnapshot
branch
from
2345495
to
0b4c719
Compare
|
@eerhardt is there anything on my end left before this can be merged? |
There was a problem hiding this comment.
I think this looks good.
@maryamariyan @davidfowl - any thoughts before this is merged?
src/libraries/Microsoft.Extensions.Options/src/OptionsSnapshot.cs
Outdated
Show resolved
Hide resolved
Co-authored-by: Kahbazi <akahbazi@gmail.com>
| return _unnamedOptionsValue = _optionsMonitor.Get(Options.DefaultName); | ||
| } | ||
|
|
||
| var cache = _cache ?? Interlocked.CompareExchange(ref _cache, new(concurrencyLevel: 1, capacity: 5, StringComparer.Ordinal), null) ?? _cache; |
There was a problem hiding this comment.
@stephentoub can you look here? I think it's copied from the other pattern we use in Options<T>
There was a problem hiding this comment.
The pattern looks fine. It's saying:
- Volatile read _cache: if it's not null, use it.
- It's null, so create a new one and atomically set _cache to a new instance iff _cache is still null.
- If it wasn't still null, use what it was instead.
- If it was still null, it's now instead the new instance we set it to, so use that.
There was a problem hiding this comment.
I'm waiting for your book to come out. Concurrency patterns with @stephentoub
|
runtime-staging failures are unrelated. |
|
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). |
…pes (dotnet#56271)" This reverts commit 8f5f9d0.
ghost
locked as resolved and limited conversation to collaborators
Closes #53793
I decided to keep the cache as a ConcurrentDictionary to head off problematic compatibility issues from switching out the underlying implementation. I'm not clear on the semantics of a DI scope regardless, maybe it's ok to use one in a concurrent fashion?
For the cache I picked capacity 5 for the 99% case, apparently 31 is the default capacity of a ConcurrentDictionary and what was specified in OptionsCache. Capacity 31 sounds fairly wasteful to new up every request but on the flip side you'd get a tiny chance of resizes I guess 🙃
OptionsManager and OptionsCache are effectively dead code now. Something to mark as obsolete in a future version by the api review team?