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
