dotnet / runtime

.NET is a cross-platform runtime for cloud, mobile, desktop, and IoT apps.
https://docs.microsoft.com/dotnet/core/
MIT License
15.12k stars 4.7k forks source link

New memory cache implementation #48567

Open davidfowl opened 3 years ago

davidfowl commented 3 years ago

Background and Motivation

The MemoryCache implementation and interface leaves much to be desired. At the core of it, we ideally want to expose something more akin to ConcurrentDictionary<TKey, TValue> that supports expiration and can handle memory pressure. What we have right now has issues:

Proposed API

The APIs are still TBD but I'm thinking a generic memory cache.

namespace Microsoft.Extensons.Caching
{
    public class MemoryCache<TKey, TValue>
    {
        public TValue this[TKey key] { get; set; }
        public bool IsEmpty { get; }
        public int Count { get; }
        public ICollection<TKey> Keys { get; }
        public ICollection<TValue> Values { get; }
        public void Clear();
        public bool ContainsKey(TKey key);
        public IEnumerator<KeyValuePair<TKey, TValue>> GetEnumerator();
        public KeyValuePair<TKey, TValue>[] ToArray();

        public bool TryAdd(TKey key, CacheEntry<TValue> value);
        public bool TryGetValue(TKey key, [MaybeNullWhen(false)] out TValue value);
        public bool TryRemove(TKey key, [MaybeNullWhen(false)] out TValue value);
    }

    public class CacheEntry<TValue>
    {
        TValue Value { get; set; }
        DateTimeOffset? AbsoluteExpiration { get; set; }
        TimeSpan? AbsoluteExpirationRelativeToNow { get; set; }
        TimeSpan? SlidingExpiration { get; set; }
        IList<IChangeToken> ExpirationTokens { get; }
        IList<PostEvictionCallbackRegistration> PostEvictionCallbacks { get; }
        CacheItemPriority Priority { get; set; }
        long? Size { get; set; }
    }
}

I'm convinced now that this shouldn't be an interface or an abstract class but I'm open to discussion.

Usage Examples

TBD

Alternative Designs

TBD

Risks

Having 3 implementations.

cc @Tratcher @JunTaoLuo @maryamariyan @eerhardt

ghost commented 3 years ago

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

Issue Details
## Background and Motivation The MemoryCache implementation and interface leaves much to be desired. At the core of it, we ideally want to expose something more akin to `ConcurrentDictionary` that supports expiration and can handle memory pressure. What we have right now has issues: - ICacheEntry uses an unintuitive pattern for adding new entries (https://github.com/dotnet/runtime/issues/36556) - It's non generic which means it boxes value types (keys and values) - Since there's no TryAdd, it's hard to implement GetOrAdd/Async in an extension method in a way that prevents the "cache stampede problem" (https://github.com/dotnet/runtime/issues/36499) - It's impossible to enumerate keys or entries (https://github.com/dotnet/runtime/issues/36026) - There's no way to clear the cache (https://github.com/dotnet/runtime/issues/45593) ## Proposed API The APIs are still TBD but I'm thinking a generic memory cache. ```C# namespace Microsoft.Extensons.Caching { public class IMemoryCache { // TBD } } ``` ## Usage Examples TBD ## Alternative Designs TBD ## Risks Having 2 implementations.
Author: davidfowl
Assignees: -
Labels: `api-suggestion`, `area-Extensions-Caching`, `untriaged`
Milestone: -
adamsitnik commented 3 years ago

It's non generic which means it boxes value types (keys and values)

How is it being typically used by the end-users: to cache multiple instances of the same type or different types? If we make it generic but users end up having more cache instances we might get a memory overhead bigger from just the boxing in the non-generic version.

adamsitnik commented 3 years ago
        DateTimeOffset? AbsoluteExpiration { get; set; }
        TimeSpan? AbsoluteExpirationRelativeToNow { get; set; }
        TimeSpan? SlidingExpiration { get; set; }

Would it be possible to make these properties have setter only, to keep the internal representation of these fields an implementation detail? So we could for example translate AbsoluteExpiration and AbsoluteExpirationRelativeToNow to ticks like StackOverflow did in their implementation

        private long _absoluteExpirationTicks;
        private readonly uint _slidingSeconds;

We could also take @edevoogd experiment under the consideration: https://github.com/dotnet/runtime/pull/45842#issuecomment-767931390

adamsitnik commented 3 years ago

The StackOverflow implementation also introduced an AccessCount field to the cache entry: https://github.com/dotnet/runtime/issues/45592#issuecomment-741994471

adamsitnik commented 3 years ago

cc @NickCraver @mgravell

davidfowl commented 3 years ago

How is it being typically used by the end-users: to cache multiple instances of the same type or different types? If we make it generic but users end up having more cache instances we might get a memory overhead bigger from just the boxing in the non-generic version.

Libraries end up with their own private caches. Apps usually have a single cache.

AndersMad commented 3 years ago

Wish: Pre- vs. PostEvictionCallbacks (instead of or additional)

Short: Called before eviction with an option to skip eviction / keep the cache entry.

Long: Skip could be:

And all topic main 👍 !.. That could reduce my code removing reflection to get the main collection etc. + make cache a lot faster with this suggestion. Think the hardest part of cache is estimating the memory. Came here to clone - hope above will be a thing ~:)

davidfowl commented 3 years ago

Short: Called before eviction with an option to skip eviction / keep the cache entry.

Can you file an issue for this? With details and usage patterns etc.

alastairtree commented 3 years ago

There is a long running discussion on why I have not implemented Clear() in LazyCache over at https://github.com/alastairtree/LazyCache/issues/67 which largely is due to the use of MemoryCache and it's current API. If this went ahead I suspect I would rewrite LazyCache to use MemoryCache<TKey, TValue> as David's proposal would make that much easier.

Worth considering usage scenarios - are you suggesting one cache per item-type, or one cache per app? Having TValue in the definition would encourage me to have one cache per item-type, which makes doing Clear on all cached data (a common use case) more difficult as there are many caches. Typically up to now apps usually have one cache, with the TValue know at time of Get, but that does cause the boxing issue.

davidfowl commented 3 years ago

I think having lots of caches is fine (we basically have this today all over the framework). It's should be treated like a dictionary with expiration.

JoeShook commented 3 years ago

I would like to see cache hit and cache miss EventCounters per cache. I want to be able to capture metrics in something like InfluxDb. And filter the cache name as a Tag and the cache-hit/cache-miss as a field.

Today I am experimenting with LazyCache and adding metrics. I found with my strategy of collecting counters by cache name it becomes a dynamic process of adding counters as they are called rather than knowing the cache names ahead of time and then having to create a specific EventSource for every application. At the moment I have not reached my destination with LazyCache and many named caches, but the point is I would hope some thought would be put towards metrics or at least hooks to allow metrics to be plugged in.

Looking around the .NET ecosystem in relation to resiliency frameworks, hooks for metrics don't seem to be a consideration. Like looking at Polly, metrics are absent and not easy to add.

jodydonetti commented 3 years ago

My 2 cents:

One question: what would be the rationale behind the implementation of the GetOrSet/Add via an ext method instead of being a functionality baked in?

roji commented 3 years ago

I've never seen a cache used with only one cache-wide specific TValue. I understand the boxing concerns, but the fixed TValue to me does not have a real-world usage.

There are ample cases where people cache objects with specific key/value types (similarly to Dictionary). EF Core uses this internally in multiple places (see #48455).

jodydonetti commented 3 years ago

There are ample cases where people cache objects with specific key/value types (similarly to Dictionary). EF Core uses this internally in multiple places (see #48455).

I see what you are saying, but I think there is a big difference between what could be described as "a dictionary with an optional expiration logic" and "an application cache", and maybe we are trying to squeeze 2 different concepts into the same abstraction, which may be problematic.

I can see a partial convergence in the feature set, but imho they should remain separate.

Maybe the right thing would be to have 2 different types?

davidfowl commented 3 years ago

I agree with @adamsitnik with having only one actual field with the expiration, but have a different design proposal: instead of having set-only props (which would make reading the expiration prob impossible) we may have a DateTimeOffset? AbsoluteExpiration { get; set; } and 2 set methods SetAbsoluteExpiration(DateTimeOffset) and SetAbsoluteExpiration(TimeSpan) which would both write to the same place

Seems fine.

in my experience the long? Size prop have created a lot of issues: if a SizeLimit is specified for the cache and a size is not specified for each entry an exception is thrown, which is frequently unexpected. I would either make it not throw in that case or make the Size prop non-nullable, aka just a long (also less pressure on the stack), with a default value of 1. This would make any entry without a specific size be worth 1 generic unit, which seems reasonable even when specifying a SizeLimit

We may remove it on this generic cache and instead expose a specialized cache for strings and bytes where the count actually works.

One question: what would be the rationale behind the implementation of the GetOrSet/Add via an ext method instead of being a functionality baked in?

GetOrAdd can be built in but right now our implementation doesn't work well because we're missing the right primitives on the IMemoryCache to implement it.

NickCraver commented 3 years ago

Would we consider an AccessCount on CacheEntry<TValue> here? This has been essential in all our use cases in eliminating a lot of very low (and often zero) usage cache members resulting in less GC work and memory usage peaks. IMO, it's something fantastic to have at scale and the overhead wouldn't matter if you weren't at scale (esp. relative to the current interface-based cache primitives).

Overall, we found that there is a huge barrier to taking and analyzing a memory dump at scale. However, if we can expose this information in the app with a few tweaks like this it becomes immensely more useful and actionable by far more people.

Fram a practical standpoint, we went from doing memory dumps and using ClrMd in LINQPad or other bits to having views like this live in production any time:

Screen Shot 2021-04-04 at 8 22 45 AM Screen Shot 2021-04-04 at 8 23 12 AM Screen Shot 2021-04-04 at 8 23 37 AM Screen Shot 2021-04-04 at 8 24 04 AM Screen Shot 2021-04-04 at 8 24 27 AM

(apologies for the bad developer UX, but you can see the info!)

If anyone's curious about the last one - it's the death by a thousand papercuts, we've found it very useful to just crawl the cache and dump some random entries to see any surprises. The * are also configured patterns in code of anything common we know about, basically collapsing anything with a list of known prefixes there but really any name translation and collapse could happen far outside the primitives as long as we could enumerate the cache.

Ideally, whatever primitives we move to here would allow such enumeration without that high "take a memory dump" bar. At current, it doesn't seem possible because:

  1. GetEnumerator() returns the value directly and so in a full enumeration the CacheEntry<TValue> and any properties wouldn't be accessible.
  2. CacheEntry<TValue> doesn't have an access/usage count.
danmoseley commented 3 years ago

How would users make sense of which of the many memory caches they should use? Are neither of the existing cache APIs redeemable?

davidfowl commented 3 years ago

How would users make sense of which of the many memory caches they should use?

I don't think this matters. The whole point of this implementation is to be a lightweight concurrent dictionary with expiration that doesn't try to respond to memory pressure and start kicking out entries. That's the only reason I see to be concerned about having a single cache. The for it to have a global process wide view of the state. That is already not the case today with .NET Core. Applications have different caches with different policies. I think this is no different.

Are neither of the existing cache APIs redeemable?

I don't think so, but I haven't given it a fair chance.

jodydonetti commented 3 years ago

The whole point of this implementation is to be a lightweight concurrent dictionary with expiration that doesn't try to respond to memory pressure and start kicking out entries.

That is what I was talking about: being for a single TKey + TValue type I see this new type as an addition to our collections toolbelt - just like the new PriorityQueue - and not as a new "cache", where cache is typically meant imho as an application-wide one, with a non-fixed TValue. Also, not kicking out entries automatically maybe means no need for both the Priority and Size + SizeLimit part.

The use case is surely interesting, and I for one would like to have that at my disposal, but calling it the "new memory cache implementation" would be potentially misleading: maybe putting it in the System.Collections.Concurrent namespace and naming it something like ConcurrentExpirableDictionary or ConcurrentDictionaryWithExpiration or simply ExpirableDictionary would be more aligned with what is already there and users expectations?

davidfowl commented 3 years ago

FYI the current IMemoryCache does the same. To be clear, we already end up with multiple in the app that each have they own policies for eviction.

I hear you in the size limit and generics though. The generics especially encourage more caches in the app itself and might cause confusion in some cases.

roji commented 3 years ago

FWIW in the EF Core usage, query compilation artifacts are being cached (rather than e.g. remote data). So expiration definitely isn't what we're looking for (artifacts remain valid forever), but rather a decent LRU which guarantees that memory consumption is capped in case there's an explosion of queries. Not saying this has to be the same type/problem being discussed here, though.

jodydonetti commented 3 years ago

Yes absolutely: on top of a date-based expiration dictionary, an LRU dictionary is another data structure missing in the BCL that would be really useful and requested a lot by the community (see on stackoverflow & co).

edevoogd commented 3 years ago

@adamsitnik, FWIW, I finally found time to write down some notes and performance measurements that I collected in the experiment.

bklooste commented 3 years ago

No convenience interface ?

I agree but there will be a whole round of how do we mock it questions ? And to the obvious answer dont , you get but how do we get 100% coverage.

geeknoid commented 3 years ago
  • I've very rarely seen a cache used with only one cache-wide specific TValue. I understand the boxing concerns, but the fixed TValue to me does not have a real-world usage. On the contrary, to then support that scenario I can imagine people coming up with higher level abstractions with separate lower level caches per type, but that would make the perf concerns about boxing basically evaporate VS having multiple caches, on top of having potential problems coordinating the different caches for high level operations (like the Clear() above)

You can always make TValue of type object, right?

geeknoid commented 3 years ago

Couple thoughts:

jodydonetti commented 3 years ago

You can always make TValue of type object, right?

From a technical point of view yes, sure, it would work. But the developer experience of using it would be bad imho, because you would then have something like Get(key) which would always return an object that you would need to manually convert to your needed type.

Obviously you may solve that with some extension methods on the type "cache of object" type but it should also take into account the generic part of TKey so I don't know if it would be such a good solution 🤷

My take is that this new type should be seen as a low level "concurrent dictionary + expiration" thing, and it would be a useful new tool on our belt (which would be even usable as a substitute for this in the MemoryCache impl).

I just don't see it as the new MemoryCache which natively supports multiple types and is mostly (in my exp at least) used as an application-level cache.

bklooste commented 3 years ago

I dont think its good to build polymorphism into the cache itself developer experience like always can be done via some extention methods .

davidfowl commented 3 years ago

OK so lets say the polymorphism isn't important because of the overhead of the cache entry itself it bigger than small value types. @roji what sort of value types are you storing in the cache today?

bklooste commented 3 years ago

Not Roji but mulitple Value tuple type support would be really nice and suite the newer more functional style microservices...kind of anoying from an implimentation point of view and there is always the option of multiple caches.

I sometimes store tagged unions in caches eg

TUnion (T) :TUnion { string type T value; }

esp other languages but C# AFAIK doesnt have a nice way of handling multiple T types without going to polymorphism or (box) reference type with type to value code.

davidfowl commented 3 years ago

I'm not sure how that would work with a generic without preserving that generic argument in the CacheEntry<T>. The only thing you could do beyond that is hardcode a fixed set of types e.g. https://github.com/dotnet/runtime/issues/28882.

geeknoid commented 3 years ago

I really don't see the need for anything beyond a class generic abstraction. With the types defined above, you can do:

MemoryCache<string, MyType> - strongly typed MemoryCache<string, IMyInterface> - strongly-typed with polymorphism MemoryCache<string, object> - untyped, fully polymorphic

For most of the scenarios I'm familiar with, a strongly typed model is what I want. I can get different cache hit stats for different types of data, I can control cache sizing independently, etc.

NickCraver commented 3 years ago

Do we think that a cache per type is something that scales well? I know where I'm at we have hundreds of types in cache, generally we're using .Get<T>, or .Set<T> and object underneath the covers. Managing n caches in pretty much every form seems like a very onerous thing for users to take on. How do you handle eviction when low on memory? Max capacity of any of them? Do we have expiration timers and iterators for each?

I'm overall wondering what the use cases are for a cache-per-type in general. In my experience thus far, that's a very niche thing and not worth the overhead (of managing many) vs. generic accessors...but maybe there are big use cases in specific areas where this makes a lot of sense. Can anyone expound on some of the cases this aims to cover? I don't doubt they exist, only very curious what they are, and if their volume justifies adding a cache with that base type structure.

davidfowl commented 3 years ago

How do you handle eviction when low on memory? Max capacity of any of them?

This isn't even supported in the memory cache today.

NickCraver commented 3 years ago

This isn't even supported in the memory cache today.

It's not built-in, but it is doable with the surface area it exposes. The same is true here with the enumerator, but the design encourages managing n of them. Whenever you're adding a new cache of whatever, you'd be in the position of making and managing a new cache. This may work great in niche scenarios where you're dealing with 1 or a few types, but for most apps I've seen that's not the case - there are many types to cache. If (I'm asking here) that is the majority use case, making TValue effectively object seems to be an odd design choice.

Tangential to this, one of the major things we do to avoid lock contention at scale is the concept of a stale serve, which may be something to consider either as a base or extension method here. What that looks like (simplified) is:

public static T GetSet<T>(this MemoryCache cache, string key, Func<T, T> lookup, TimeSpan duration, TimeSpan serveStaleDuration)

What this does is extend a cache (our internal in this case), and for a key, takes a function. This function gets the old value (or default, if it doesn't exist) and is tasked with getting the result (e.g. from a database or API or whatever, and in the case of failure, you could return the old value). The duration argument is how long this cache is fresh for. The serveStaleDuration is how long the cache is additionally served for, past freshness.

Let's say we cache for 5 seconds, and serve stale another minute, what would happen on a timeline is:

The tradeoff here is that we're serving a cache that might be a tad stale to some users, but we don't fall off a cliff and have many threads trying to get a value or block of getting a value when the cache expires. Instead, for that duration we serve the value a bit past freshness while exactly one instance of lookup goes off and gets a fresher value.

Behind the scenes, what's actually cached is a GetSetWrapper<T> which contains a DateTime StaleAfter and a T Data. That object is cached for the fresh + stale duration combined since that's the total efficacy.

We have a similar version that's .GetSetAsync<T> as well.

I'm not sure if that's germane to this design conversation, but I thought I'd bring it up because it's something we use a lot at Stack Overflow - it's our go-to for hot paths so that we don't have a task/thread pile-up when critical caches expire in the app, nor do we go beat the crap out of the data source at the same time. It would be awesome, if this is desirable, to support such an extension method at least on whatever data structure is designed here. In our current world, it's ultimately using object and converting on the fetch. If you were to re-use a key with a different type...nothing good would happen.

Apologies if that's way off the goals here, just thought I'd bring it up somewhere possibly relevant since it's been a great approach to us internally.

bklooste commented 3 years ago

"it is doable with the surface area it exposes"

Its the interface here that is key .. and a KISS memory cache now is better than a promised one with memory pressure that never arrives. You also quickly get into IDistributeCache

You can have a per type interface on a more complex cache with memory pressure in the future.

kieranbenton commented 3 years ago

I've always had this pattern described to me as "dog piling protection".

We do something very similar, but in a slightly different manner as we tier caches (per request, in memory and redis) depending on a policy for individual keys.

Instead of extending the timeout when serving the stale request, we actually extend the true cache timeout at SET time and store a wrapper data structure within the cache value that has the 'soft' expiry and 'is regenerating' flags that are checked on GET.

Probably not that memory efficient as it adds two extra fields to the entry and requires an additional comparison on each get, but it massively simplifies the tiering logic.

roji commented 3 years ago

@davidfowl

@roji what sort of value types are you storing in the cache today?

EF Core has various places where a struct key is used, pointing to various other heavy objects which are traversed as part of the lookup (e.g. here, here). Note that again that we're not interested in expiration-based eviction (entries stay valid for over), only for capping memory usage if a lot of values are getting inserted into the cache (because tons of different queries are being executed).

@NickCraver

If (I'm asking here) that is the majority use case, making TValue effectively object seems to be an odd design choice.

What are we concretely losing by making this generic? If a single, application-wide heterogeneous cache is what you want, you can do that (with TValue being an object). People who want separate caches for whatever reasons can do their thing by specifying a specific TValue. It basically feels like there's no downside to it, no?

jodydonetti commented 3 years ago

In my oss lib FusionCache (shameless plug) I do kind of the same thing as @NickCraver or @kieranbenton, that is serving stale data in case bad things happen (a refresh fails, takes too much, etc): it is a very useful approach and saves a ton of errors in the face of end users when the infrastructure is having a bad day. But honestly I don't know if putting something like that into a core api is the way to go.

ps: sorry if the shameless plug is undesired, if that is the case please remove my comment.

jodydonetti commented 3 years ago

In general I think we're circling around this: there are imho 2 different use cases, and trying to crunch them into 1 api design may not be the best thing.

2 different use cases

The first is “I need to cache some objs of a certain specific type and I will access it” and these caches are typically created locally, via new, and nobody except my code will access it. It's more of a low level thing.

The second is more of an app-wide, shared, typically singleton (think DI registration) cache that is used across an application by a lot of different pieces of code, not "owned" by someone in particular.

Today the first is typically done with a [concurrent] dict or similar, but is lacking some sort of expiration/lru logic. The second is done via the old ObjectCache or the new MemoryCache, and is more heavyweight.

If you try to use the MemoryCache for the first case, you are worried about boxing and some useless extra features. If you use dict for the second case, well, that is not really doable because it's too low level and is missing a lot of functionality.

It may seem like they are the same thing but they are actually not, and I think the confusion comes from the word itself, "cache", because it is both a general practice that can be even implemented with a simple static class and some init code w/ a lock, no expiration and extra bells and whistles (think "I save this piece of data here for faster reuse later") and a type of service tyipically shared between different parts of an application.

Lifetime and instance creation

I'll add an additional thing which may be helpful to reason about. As we all know DI is the official "way to go" right? And how is MemoryCache registered? Yep, as a singleton, meaning it is expected to be used in a shared way accross the entire application, and that falls into the second use case above. But when we look at how a "local cache" is used, that is typically not requested globally to the system via DI but is created and managed locally.

Opinions?

paulomorgado commented 3 years ago

@NickCraver, doesn't having a single cache, usually with a string key, cause the creation of lots of strings just for indexing the cache?

bklooste commented 3 years ago

@jodydonetti I think using a cache in the first form is really rare , normally you would just use a dictionary (ToDictionary) or hashset. sometimes a concurrent dictiionary. esp since the add normally require long lived life times and external IO.

The 2nd case is nearly all use cases and in this case whethere its IOC ( note NOT DI ) or not is irrelevant a console app , azure / lamda function or micro service with a global cache is the same as a larger app managed by an IOC container. The key thing both are app lifetime scoped .

bklooste commented 3 years ago

@paulomorgado strings are not normally big compared to value and if you want tiny objects as a consumer your better of using the hash as a key .

I have some implimentations use the hash internally but its best done by the caller.

jodydonetti commented 3 years ago

@jodydonetti I think using a cache in the first form is really rare , normally you would just use a dictionary (ToDictionary) or hashset. sometimes a concurrent dictiionary. esp since the add normally require long lived life times and external IO.

Exactly, you would use something lower level (like a dict, etc) "to cache some data" and not "as a cache service", that was my point. But as it turns out people who need a "dict + date-based expiration" or "dict + size-based compaction" end up using the MemoryCache because it has those features, like @roji in EFCore (if I understood correctly). This new type may fit that space, so that would be in fact not a "cache service" but a pumped up dict.

The 2nd case is nearly all use cases and in this case whethere its IOC ( note NOT DI ) or not is irrelevant a console app , azure / lamda function or micro service with a global cache is the same as a larger app managed by an IOC container. The key thing both are app lifetime scoped .

Yep, I agree. And since in those use cases we are talking about something that is app-lifetime-scoped and shared (eg: typically singleton), that would be a "cache service" shared accross the entire app, and that means a single TValue is probably not what's needed, at least to me.

Again, I think the main point of confusion and the big difference is between "a low level component to cache some data that can expire/be evicted in some way" (eg: a smarter dict or similar) and "a shared app-wide cache service". Both are useful, just different in scope and design.

If I misunderstood you let me know.

paulomorgado commented 3 years ago

@bklooste,

strings are not normally big compared to value and if you want tiny objects as a consumer your better of using the hash as a key .

It's mostly not about the length of the strings but the number of times you have to compute them and allocate them. That's CPU comsumtion to compute the key and memory and GC work to allocate/deallocate them.

I have some implimentations use the hash internally but its best done by the caller.

Beware that different strings might have the same hash code.

vitek-karas commented 3 years ago

Given that memory pressure based eviction seems to be an explicit non-goal here, I guess unloading is also a non-goal. But I wanted to make sure it's considered. It's another example of an eviction policy, but one which should be typically combined with some other eviction policy and almost never used on its own.

What I mean by unloading is making sure that all entries which have references to an AssemblyLoadContext which is being unloaded are evicted (the trigger would be the AssemblyLoadContext.Unloading event in this case).

I'm sure it's possible to build this on top, but then it becomes a question of how to make all the libraries in the app implement these additional eviction policies so that the app works as a whole (and for example can successfully unload a plugin).

NickCraver commented 3 years ago

@NickCraver, doesn't having a single cache, usually with a string key, cause the creation of lots of strings just for indexing the cache?

Yep, for lack of a better option generally. It's also very fast though, for example you could cache based on the hash of a tuple or something, but since that would be on fetch there is a computational cost to it. Is that cheaper than the GC cost? I'm not sure, but it's an interesting experiment to try at scale. Most alternatives I'm aware of have similar risks of hash collision if that's your lookup model, though. I'm all ears for better options - strings are simple, easy to understand, and fairly cheap to compute (though you deal with cost later) - that's not an argument for them over other things, it just places them high on the usability tradeoff scale today.

NickCraver commented 3 years ago

For what it's worth, I always thought a model that allow caching via a tuple with minimum allocations might be interesting, but I'm not sure how we address the collision issue or exactly what the interface for such could look like in the current type system. If you had n caches that approach works, but becomes a n caches management problem (at least, we'd have hundreds of caches at a minimum).

If we could have a cache that internally was exposed like (these arguments being a completely arbitrary example):

public MyCachedType Get(int userId, int categoryId)
{
    var key = ("SomeUniqueKey", userId, category); // Tuple here for cache key
    // ...something...
}

Today, we'd do something like:

public MyCachedType Get(int userId, int categoryId)
{
    var key = $"SomeUniquePattern:{userId.ToString()}:{categoryId.ToString()}";
    // ...something...
}

(so that it's using string.Concat in the end)

...anyway, I think that'd potentially be useful, but would want a shared cache for such a variant key pattern. The return type from cache would likely be object and casted still, as we do today.

ericsampson commented 3 years ago

It seems to me like there are two quite different usecases that people are looking for, and that a) trying to address both in one design might lead to a compromise, and b) the current discussion/proposal seems to lean more heavily towards the "ConcurrentDictionary with expiration, caching objects with key/value types, N caches" scenario. The other scenario being more the "application cache, one per app, key is often a string currently but maybe a tuple could work well, often used to cache things like API bearer tokens"

Should there be a separate proposal/interface for the latter application-cache usecase? To prevent the design from getting muddled between the two different needs. For instance, in this case as David points out by referring to [https://github.com/dotnet/runtime/issues/36499], people get bit by not realizing the fact that the factory in GetOrCreate* is not re-entrant-proof, which matters when you're calling an external API or token endpoint that takes N seconds to respond. Or another angle to consider for this usecase is if it would be worthwhile to build in the "serve-stale" functionality that Nick Craver mentioned, as an extension if desired.

I just feel like the two usecases are sufficiently different that trying to address both in one API might make things messy.

Cheers!

Turnerj commented 3 years ago

One thing to consider is how much should be in the runtime as default. There are several caching libraries in the .NET ecosystem (FusionCache, CacheManager, Cache Tower, EasyCaching, LazyCache, MonkeyCache, and probably a bunch of others) which can handle the more complicated and feature rich scenarios.

I am biased as the creator of one of those caching libraries but my view is that it is the simple/common scenario that the MemoryCache implementation should aim for - the ConcurrentDictionary with expiration scenario. It should be fast, it should be straight forward and a lot of people should use it, it just shouldn't be all-encompassing.