Remove Gen2 GC based trigger for compacting memory cache #221

[JunTaoLuo]

#221

Will need approval from @Eilon and @DamianEdwards due to the breaking API change.

[Eilon]

Do we have any gauge as to whether anyone out there has implemented this API and would be broken?

[Eilon]

Can you update the doc comment in MemoryCache itself to be a proper doc comment? Right now there are some odd /// comments there but not an actual doc comment.

Caching/src/Microsoft.Extensions.Caching.Memory/MemoryCache.cs

Lines 275 to 281 in cd57aea

	/// Remove at least the given percentage (0.10 for 10%) of the total entries (or estimated memory?), according to the following policy:
	/// 1. Remove all expired items.
	/// 2. Bucket by CacheItemPriority.
	/// 3. Least recently used objects.
	/// ?. Items with the soonest absolute expiration.
	/// ?. Items with the soonest sliding expiration.
	/// ?. Larger objects - estimated by object graph size, inaccurate.

[Eilon]

Also, the implementation in MemoryCache doesn't do any argument validation, so that needs to be fixed as well.

[Tratcher]

should this return the number of elements removed? or a bool indicating if any elements were removed?

[JunTaoLuo]

Since the argument is in percentage, it would probably make more sense to return the percentage of elements removed?

[Tratcher]

That doesn't seem very informative. Also, percentages are problematic if you remove 0/0.

[JunTaoLuo]

Also, since we are exposing Compact, should we expose Count on the interface as well? It's not a good way to gauge cache size but I suspect users will downcast and rely on it to determine how much to compact like in #252.

[Tratcher]

The point of using percentages is that they don't need to know the size. Otherwise Compact would take a specific amount.

Or we shift it so Compact takes a specific number of elements to purge, returns the actual number removed, and expose Count.

[JunTaoLuo]

Then let's keep using percentages for both the amount to remove and amount removed. In the 0/0 case, just return 0. Returning the number of entry removed is rather strange considering a percentage was passed in. Returning a bool is also very uninformative.

[Tratcher]

@Eilon except we've punted the decide-when-to-compact duty to the developer, and the number of elements in the cache is one inputs you'd use to decide when and how much to purge.

[Tratcher]

@JunTaoLuo put this on hold until we can sync in person Monday.

[JunTaoLuo]

Will probably solicit feedback from @davidfowl and @DamianEdwards as well if possible.

[Eilon]

I don't think anyone can ever reason about how much to clear based on the number of items in the cache. Remember, the cache is generally a shared resource. Someone (including our own codes) might have stashed a thousand small objects there. Or 1 really ginormous one.

But yes, let's discuss.

[JunTaoLuo]

We're going with a different design. I will update the issue and open a new PR.

[Wallsmedia]

@JunTaoLuo Could have a look at my change proposal,
Add to MemoryCacheOptions the CustomCompactOnMemoryPressureDelegate. #252
It may contribute to design.
I have added example how it woks.
To be clear, I have been using this approach/solution in my web services.

[davidfowl]

We'll use it as an input but we're basically separating the concept of memory pressure detection out of the cache and adding a first class way to provide compaction logic via an interface. There will also be a way to trigger a compaction via an API exposed from the cache.

Similar so what you delegate can do but less specific.