On 04/29/2014 05:29 AM, Amos Jeffries wrote: > +1. All good apart form one minor nit: > > src/tests/stub_store.cc > * the new checkCacheable() needs to be STUB_RETVAL(false). > > That can be added on merge.
Finally merged as trunk r13475. Please note that another, unaudited fix for a frequent shared memory cache assertion was included in this merge: > Avoid store_client.cc "entry->swap_filen > -1 || entry->swappingOut()" > asserts. > > A client may hit on an incomplete shared memory cache entry. Such entry is > fully backed by the shared memory cache, but the copy of its data in local RAM > may be trimmed. When that trimMemory() happens, StoreEntry::storeClientType() > assumes DISK_CLIENT due to positive inmem_lo, and the store_client constructor > asserts upon discovering that there is no disk backing. > > To improve shared cache effectiveness for "being cached" entries, we need to > prevent local memory trimming while the shared cache entry is being filled > (possibly by another worker, so this is far from trivial!) or, better, stop > using the local memory for entries feeding off the shared memory cache. The > latter would also require revising DISK_CLIENT designation to include entries > backed by a shared memory cache. This production-tested fix essentially reorders two checks in StoreEntry::validToSend(): > if (!mem_obj) // not backed by a memory cache and not collapsed > return 0; > > - if (mem_obj->memCache.index >= 0) // backed by a shared memory cache > - return 1; > - > // StoreEntry::storeClientType() assumes DISK_CLIENT here, but there is > no > - // disk cache backing so we should not rely on the store cache at all. > This > - // is wrong for range requests that could feed off nibbled memory (XXX). > - if (mem_obj->inmem_lo) // in local memory cache, but got nibbled at > + // disk cache backing that store_client constructor will assert. XXX: > This > + // is wrong for range requests (that could feed off nibbled memory) and > for > + // entries backed by the shared memory cache (that could, in theory, get > + // nibbled bytes from that cache, but there is no such "memoryIn" code). > + if (mem_obj->inmem_lo) // in memory cache, but got nibbled at > return 0; > > + // The following check is correct but useless at this position. TODO: > Move > + // it up when the shared memory cache can either replenish locally > nibbled > + // bytes or, better, does not use local RAM copy at all. > + // if (mem_obj->memCache.index >= 0) // backed by a shared memory cache > + // return 1; > + > return 1; > } I can adjust or revert this if needed, of course. Thank you, Alex.
