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

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().

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.

Amos
Received on Fri Jul 06 2012 - 03:02:36 MDT