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.
