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

Hi Amos.

Amos Jeffries <squid3_at_treenet.co.nz> writes:

> On 6/07/2012 6:29 a.m., Alex Rousskov wrote:
>> 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.
>
> It sounds like another analysis of the places calling trimMemory() is
> needed. So that trimMemory is isolated to just trim like its name
> suggests. The maybeTrim() decisions being outside functionality (in
> StoreController?) and which is the only place calling e.trimMemory().
>

What is the benefit given that maybeTrim() is not used separately?

> Looking at this v2 patch the Bug comment and IN_MEMORY test looks like
> it should be inside the keepInLocalMemory() instead. Since things
> already *in* local memory are obviously allowed there somehow. That is
> probably wrong, but is clearly implied by the symbol names around it.
>

IN_MEMORY indicates that entry is cached in local memory, so it probably
belongs to SoreControlle::keepInLocalMemoryCache(). On the other hand,
I am not sure this check is needed at all. I am ok with moving it. But
keeping it where it is now with the comment makes it clear where it came
from in case we want to remove it later.

Regards,
  Dmitry

> Amos
Received on Fri Jul 06 2012 - 09:48:03 MDT