[squid-dev] [RFC] reduce MISS on transients collision

[Alex Rousskov]

On 07/17/2016 12:59 PM, Alex Rousskov wrote:
> On 07/17/2016 05:01 AM, Amos Jeffries wrote:
>> I've just been looking at the Store::Controller::find() implementation
>> and it struck me that if the transients lookup has an error the object
>> will fail to HIT on any existing cache entries.
> 
> If the transients table tells us that the transient object is in a "bad"
> state, then trying to load that same object from a store will fail at
> best or result in a stale/truncated/stuck response at worse.
> 
> 
>> Alex; am I missing something undocumented here ?
> 
> You might be missing something documented: The Transients definition or
> purpose. Transients is not one of the many "if not here than possibly
> there" cache stores. Transients is dedicated to cache entries in
> transient state. If a given entry can be correctly loaded from a regular
> cache store, then, by definition, that entry is not transient [any more]
> and would not be in Transients. Consequently, if the entry is in
> Transients, then it is impossible to load it correctly from a regular store.
> 
> It is possible that a lock contention or a similar SMP race condition
> inside Transients would result in a cache miss instead of reading from a
> Transients-controlled entry, but, bugs notwithstanding, that should not
> happen often.


And one more problem: The email subject implies that you are modifying
handling of Transient hash collisions whereas your patch modifies
handling of problematic Transient entries, not key collisions:

>      // Must search transients before caches because we must sync those
> we find.
>      if (transients) {
>          if (StoreEntry *e = transients->get(key)) {
>              debugs(20, 3, "got shared in-transit entry: " << *e);
>              bool inSync = false;
>              const bool found = anchorCollapsed(*e, inSync);
>              if (!found || inSync)
>                  return e;
>              assert(!e->locked()); // ensure release will
> destroyStoreEntry()
>              e->release(); // do not let others into the same trap
> -            return NULL;
> +            // continue on to maybe find it in cache
>          }
>      }

On hash collisions, transients->get() returns nil, bypassing your
modifications:

> StoreEntry *
> Transients::get(const cache_key *key)
> {
...
>     const Ipc::StoreMapAnchor *anchor = map->openForReading(key, index);
>     if (!anchor)
>         return NULL;
> 


HTH,

Alex.