Reduce unnecessary allocation of SeldomUsedFields in MemoryCache. #51761
Conversation
ghost
added
the
src/libraries/System.Runtime.Caching/src/System/Runtime/Caching/MemoryCacheEntry.cs
Outdated
Show resolved
Hide resolved
| // is never null. So check that the collection of dependencies is not empty before allocating | ||
| // the 'seldom' used fields. | ||
| if (dependencies != null && dependencies.Count > 0) | ||
| { |
There was a problem hiding this comment.
I think there's a bug here. The dependencies param passed originally comes from CacheItemPolicy.ChangeMonitors. The docs for that property says:
If you change the set of change monitors on a CacheItemPolicy object after the CacheItemPolicy object has been passed to an ObjectCache implementation, the changes have no effect.
This implies that the dependencies are expected to be copied when passed in. If a caller passes a CacheItemPolicy to MemoryCache.Set, and then modifies the CacheItemPolicy.ChangeMonitors collection afterwards, that change will be reflected in the collection stored here. It's also not thread safe. For example, CallNotifyOnChanged gets an iterator which will throw if the collection is modified while iterating.
There was a problem hiding this comment.
Yes, for the scope of this fix, it looks good.
Co-authored-by: Christopher Watford <[email protected]>
4 participants
Reduce unnecessary allocation of SeldomUsedFields in MemoryCache.
Checking for the existence of a 'seldom used field' would trigger
the allocation of this object, even if none of the fields were being
used. And this check/trigger happens on just about every item
inserted in the cache, meaning this perf optimization wasn't actually
saving anything. The fix is to not allocate when simply checking
for the existence of these fields.
Fix #1425