Hi Amos. Amos Jeffries <squ...@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