Re: [PATCH] Pipeline prefetch build-time control

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.

Thank you,

Alex.
Received on Wed May 15 2013 - 15:45:09 MDT