On 05/16/2013 08:51 AM, Amos Jeffries wrote: > Attached is an update which goes straight to the end and make > pipeline_prefetch directive accept an integer count of pipeline length.
> + // if client_persistent_connections OFF, then we cannot service any > pipeline >1 Please replace "we cannot service any pipeline >1" with "we do not expect any pipelined requests" to avoid confusing "pipeline>1" math: Does pipeline==2 mean two concurrent requests or tree, since we are not counting the already read request? Your math is correct, but this phrasing is confusing IMO. > + const bool result = (conn->getConcurrentRequestCount() < 1); > + if (!result) > + debugs(33, 3, conn->clientConnection << " persistence disabled. > Concurrent requests discarded."); We do not know whether we discarded something or not. Drop the second sentence. If this code stays, please compute getConcurrentRequestCount() once, before all the if-statements and then uses the computed number as needed. > + if (!Config.onoff.client_pconns) { > + const bool result = (conn->getConcurrentRequestCount() < 1); > + if (!result) > + debugs(33, 3, conn->clientConnection << " persistence disabled. > Concurrent requests discarded."); > + return result; > + } The above addition to connOkToAddRequest() seems odd: If the caller does not check for persistency, then the above check is not sufficient -- there are other reasons the connection may not be persistent (and the old code was broken since it lacked any checks at all!). If the caller checks for persistency, the above check is not needed. Consider the case where Config.onoff.client_pconns is "on" (default) but the client sent "Connection:close" or Squid is already responding with "Connection:close". If the caller does not check, I do not think you have to be responsible for fixing this, but if you add the above code without adding other (far more relevant in default cases) checks, then please at least add a XXX documenting that other checks are missing here. > +LOC: Config.pipeline_max_prefetch How about "pipeline_prefetch_max" in case we need to add more pipeline_prefetch options later. > To boost the performance of pipelined requests to closer > match that of a non-proxied environment Squid can try to fetch > - up to two requests in parallel from a pipeline. > + several requests in parallel from a pipeline. This sets the > + maximum number of requests to be fetched in advance. I think this needs to be clarified to better explain the confusion between the pipeline "depth" and the number of concurrent requests: HTTP clients may send a pipeline of 1+N requests to Squid using a single connection, without waiting for Squid to respond to the first of those requests. This option limits the number of concurrent requests Squid will try to handle in parallel. If set to N, Squid will try to receive and process up to 1+N requests on the same connection concurrently. > + debugs(0, DBG_PARSE_NOTE(DBG_IMPORTANT), "WARNING: > 'pipeline_prefetch on' is deprecated. Please update to use a number."); Perhaps we should indicate what the replacement is, like we do in pipeline_prefetch off case? We could say "Please update to use 1 (or a higher number)." Thank you, Alex.