Re: [PATCH] Fixes to get more collapsed forwarding hits

On 06/24/2014 01:44 PM, Kinkie wrote:
> Amos already +1-ed the patch, I have no insights to add so unless
> someone speaks up real fast, we proceed.

Committed to trunk as r13476 and r13477.

Thank you,

Alex.

> On Tue, Jun 24, 2014 at 9:29 PM, Alex Rousskov
> <rousskov_at_measurement-factory.com> wrote:
>> On 06/24/2014 12:55 PM, Kinkie wrote:
>>
>>
>>> Did you run coadvisor and polygraph against it?
>>
>> Co-Advisor does not have collapsed forwarding cases (there are no
>> explicit RFC 2616 MUSTs that cover CF although some cases can be
>> certainly added to test some MUSTs in a CF context).
>>
>> Polygraph can be used to simulate CF-heavy environment, but all you will
>> get from this patch is an improved hit ratio. It is not going to tell
>> you which collapsing cases Squid have missed.
>>
>>
>>> If not, and if the branch is public, it's trivial tor un them against
>>> it now (thanks TMF, thanks Pavel!). It doesn't guarantee to catch all
>>> cases, but at least it should raise confidence.
>>
>> There is currently no branch for this -- just a trunk patch. A bigger
>> problem is that you will need a different Polygraph workload -- the one
>> we use for Squid regression testing via Jenkins is unlikely to produce a
>> lot of CF opportunities.
>>
>> Overall, while I would certainly welcome Co-Advisor and Polygraph
>> results, I decided to rely on existing trunk regression testing and not
>> spend more time on organizing custom pre-commit tests. If somebody wants
>> to volunteer to test this, please let me know.
>>
>>
>> Thank you,
>>
>> Alex.
>>
>>
>>> On Tue, Jun 24, 2014 at 7:37 PM, Alex Rousskov
>>> <rousskov_at_measurement-factory.com> wrote:
>>>> On 06/23/2014 11:58 PM, Amos Jeffries wrote:
>>>>> On 24/06/2014 4:07 p.m., Alex Rousskov wrote:
>>>>>> * Do not abandon writing a collapsed cache entry when we cannot cache
>>>>>> the entry in RAM if the entry can be cached on disk instead. Both shared
>>>>>> memory cache and the disk cache have to refuse to cache the entry for it
>>>>>> to become non-collapsible. This dual refusal is difficult to detect
>>>>>> because each cache may make the caching decision at different times.
>>>>>> Added StoreEntry methods to track those decisions and react to them.
>>>>
>>>>
>>>>> Hmm. FTR: IMHO the only reasons a response should be non-collapsible is
>>>>> if it is a) private, b) authenticated to another user, c) explicitly
>>>>> known non-cacheable, d) a non-matching variant of the URL, or e) we
>>>>> already discarded some body bytes.
>>>>
>>>> Yes, there are many "only" reasons why a we may not be able to collapse
>>>> what looked like a collapsible transaction at the beginning. The bullet
>>>> we are discussing fixes a case where the Squid shared memory caching
>>>> code said "if we cannot cache this entry in the shared memory cache,
>>>> then we cannot collapse onto this entry". That was wrong; we should
>>>> still be able to collapse if the disk cache stores the entry.
>>>>
>>>>
>>>>> The factor of memory/disk caches not having provided cacheable size
>>>>> allow/reject result yet is no reason to be non-collapsible. It is a
>>>>> reason to wait on that result to determine the cacheability condition (c).
>>>>
>>>> Yes, kind of. Today, Squid treats "HTTP cachable" and "storable in a
>>>> Squid cache" as two rather different things. The two conditions are
>>>> checked and managed in different code places and at different times
>>>> (with some overlap). And some of those decisions are unrelated to the
>>>> entry size. However, they are essentially the same from user-perceived
>>>> caching point of view and can be listed under a single "explicitly known
>>>> non-cacheable" bullet if one is careful to interpret that correctly.
>>>>
>>>>
>>>>> +1. Although I do not know enough about store yet to do much in-depth
>>>>> audit of the logic changes.
>>>>
>>>> FWIW, knowing "enough" about Store makes me even less confident about
>>>> changes like these ones. This is very tricky stuff not just because of
>>>> the complexity of the features involved (caching, collapsing, etc.) but
>>>> because there are very few simple invariants in Store. I rarely know for
>>>> sure what I am going to break when changing this stuff. The code is
>>>> getting better, and I am trying to document the invariants I find, but
>>>> it is a very slow process, and there will be many more bugs, possibly
>>>> from this patch as well.
>>>>
>>>>
>>>> Cheers,
>>>>
>>>> Alex.
>>>>
>>>
>>>
>>>
>>
>
>
>
Received on Wed Jun 25 2014 - 00:13:36 MDT