On 07/05/2012 09:02 PM, Amos Jeffries wrote: > It sounds like another analysis of the places calling trimMemory() is > needed.
StoreEntry::trimMemory() is only called from StoreEntry::swapOut(), before we are starting to swap data out (or skipping swap because the entry is not swappable). Ideally, it should also be called from places where we unlock entry's memory as well, but that is the way it was written. Changing that is outside this bug scope and may be difficult. > 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(). First of all, I would ignore the "maybe" aspect. The caller does not rely on any trimming actually happening and some computations of how much to trim (if anything!) happen deeper inside MemObject. We can add "maybe" to a bunch of methods so that their names reflect the possibility that nothing would be trimmed. Those old methods are present in all Squid versions. I think renaming them just to polish the names would cause more porting headaches than it would solve problems. If I get your primary idea correctly, then instead of StoreEntry::swapOut() calling e.trimMemory() it will call new StoreController::trimMemory(e) and the controller would decide whether to proceed with trimming and, if yes, will call e.trimMemory(). StoreEntry::trimMemory() will not have any new conditions added then. If that is what you are suggesting, it sounds good to me, and I can do that during commit of v3 patch (it would not change the functionality or the order of the checks; just move a couple of checks to a different class/method). > 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. They were allowed into local memory because Squid cannot do I/O without using local memory :-). Every byte read is placed in local memory first. The issue here is whether those local memory bytes can be forgotten (i.e., the entry memory can be trimmed). The answer depends on the entry state and on the memory caching module state/functionality. The latter aspect was ignored for a while, until this patch. HTH, Alex.
