Re: [PATCH] Pipeline prefetch build-time control

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.
Received on Mon May 20 2013 - 21:47:30 MDT