Re: [PATCH] Pipeline prefetch build-time control

On 16/05/2013 3:45 a.m., Alex Rousskov wrote:
> On 05/15/2013 04:45 AM, Amos Jeffries wrote:
>> On 15/05/2013 10:20 a.m., Alex Rousskov wrote:
>>> On 05/11/2013 10:00 PM, Amos Jeffries wrote:
>>>
>>>> The attached patch adds a built-time setting SQUID_PIPELINE_MAX to
>>>> replace the magic numbers previously used for limiting the amount of
>>>> prefetch Squid does when pipeline_prefeth config directive is enabled.
>>>> The default remains at 2 unless altered by using -DSQUID_PIPELINE_MAX=n
>>>> in CXXFLAGS and this adds some documentation about why, and what can be
>>>> done to improve pipelining in future.
>>> I do not understand why we need a hard-coded maximum.
>> I'm not completely sure myself. I think I've checked about half the code
>> involved and most of it it nicely using linked-list pointer checks. Just
>> these two input points using magic 2's.
>>
>> I know that there are protocol use cases where things can happen which
>> cause the pipeline to needs discarding and that causes all sorts of edge
>> cases. I'm gettign the idea these 2's were there solely to avoid dealing
>> with that mess.
>>
>> PS. When we are certain its safe it will be easy to change
>> pipeline_prefetch into a numeric directive setting a max limit and some
>> smart code doing dynamic adjusting. But for now I'm simply not sure
>> enough about what the rest of the code is doing to go go further than
>> removing the magic numbers - as we audit and find other code relying on
>> the magics we can alter them to rely on this macro easily enough.
> FWIW, I think we should make the limit configurable because I do not
> think there are any valid reasons to restrict _all_ pipelining
> deployments to 2 pipelined requests.
>
> If the plan is to make the limit configurable, I do not see the point of
> replacing a magic number if a #define (which will, in turn, be removed
> when the limit is configurable). However, if you are sure this change is
> needed, feel free to commit.

Very well. Attached is an update which goes straight to the end and make
pipeline_prefetch directive accept an integer count of pipeline length.

There are some additional polishing that I would like to do to the
pipeline API of ConnStateData, but that will come in a followup patch.

Amos