Re: [PATCH] Pipeline prefetch build-time control

On 05/25/2013 01:42 AM, Amos Jeffries wrote:

> So be it. This drops connOkToAddrequest() in favour of a
> ConnStateData::concurrentRequestQueueFilled() which simply checks the
> available queue space and replies true/false. All other state handling
> relies on the existing code to behave properly. Pity anyone who hits a bug.

Please note that I am not saying that the current code is correct or
that all of the proposed changes were useless. The earlier patch did
identify a couple of potentially problematic spots in the current code
and had good bits that could be massaged into making that code better.

While "this is the last request" changes would not be specific to
pipelining, and would not require a complex multi-state data member,
they would still be useful IMO. FWIW, I would welcome their resubmission
(as a separate patch), perhaps adding a new flag with a setter method
for debugging.

Now back to the pipelining changes:

> - /* Limit the number of concurrent requests to 2 */
> - if (!connOkToAddRequest(this)) {
> + /* Limit the number of concurrent requests */
> + if (!pipelineQueueFilled()) {
> break;
> }

The negation should now be removed, right?

> + debugs(33, 5, clientConnection << " defering new request until one is done");

Since you are changing these lines, let's fix spelling:
s/defering/deferring/

Both changes can be done during commit as far as I am concerned.

Thank you,

Alex.
Received on Sun May 26 2013 - 00:57:11 MDT