Re: [PATCH v2] Do not release entries that may be kept in local memory cache.

On 07/05/2012 11:38 AM, Dmitry Kurochkin wrote:
> Alex Rousskov writes:
>> I suggest adding StoreController::keepInLocalMemory(e) that will
>> correctly check whether memory caching is enabled at all and then call
>> e.memoryCachable() to arrive at the final answer. The new method will be
>> called from StoreEntry::trimMemory(). Will that work better?
>>
>
> Yes, I think it will.
>
> But perhaps keepInLocalMemory() is not the best name

It is not a good one indeed, but I cannot think of a better one. I did try!

> given that it will check both local and shared memory cache?

That aspect is fine because the purpose of the method is to determine
whether the entry should be kept in _local_ memory for now because a
local or shared memory caches will need it there later.

> Also, keepInLocalMemory()
> sounds like it should make a final decision whether to cache an entry
> which would not be true in our case.

That is the aspect I could not fix without resorting to something very
long like mayBeNeededInLocalMemoryLater(). Feel free to use that one if
you prefer!

> How about adding StoreController::memoryCacheEnabled() method? It would
> have a clear meaning. StoreEntry::trimMemory() would then check for
> (StoreController::memoryCacheEnabled() && StoreEntry::memoryCachable()).
> What do you think?

I like my approach better because

a) These decisions should not be made by StoreEntry, in
StoreEntry::trimMemory() code. They should be made by Store [modules].
They will become increasingly complex and module-dependent if we start
optimizing this.

b) Even today, the decision should be more complex than "memory cache is
enabled". For shared memory cache, it should involve checking whether
the entry may fit. See MemStore::considerKeeping() and
MemStore::willFit() call in particular. That check (as is or adjusted)
should be applied here today by adding MemStore::keepInLocalMemory() and
calling that from StoreController::keepInLocalMemory().

(b) is necessary to avoid keeping huge entries in RAM when they have no
chance of being stored in a shared memory cache later. I should have
mentioned (b) earlier, sorry.

HTH,

Alex.
P.S. The comment next to the willFit() call in
MemStore::considerKeeping() is buggy. Do not let it confuse you. The
call is not about available or freed cache space.
Received on Thu Jul 05 2012 - 18:29:51 MDT