[squid-dev] [PATCH] Fix ICAP transactions that read a lot of data

[Alex Rousskov]

On 07/24/2015 07:46 PM, Amos Jeffries wrote:
> On 25/07/2015 10:51 a.m., Alex Rousskov wrote:
>> Affected transactions fail with mayReadMore() exceptions because their
>> readBuf.spaceSize() is zero while they need to read more data.
>>
>> Any append,parse,consume;append,parse,consume;... user of SBuf cannot
>> rely on SBuf::spaceSize() to be meaningful because even consuming the
>> entire SBuf contents may leave spaceSize() at zero! Instead such code
>> has to use SBuf::length() to keep buffer from growing too big and
>> SBuf::rawSpace(1) to ensure some space is available for reading when the
>> buffer is not too big.


> If you were using rawSpace(1) to avoid the possible reallocation by
> reserveSpace() 

I use rawSpace() instead of reserveSpace() because reserveSpace()
guarantees single ownership, and we do not need that in the context of
an immediate ReadNow() call.


> it was not worth it. The reserveCapacity() used above it
> is what reserveSpace() uses internally and causes that reallocation. So
> its happening anyway.

Yes, I agree that the earlier reserveCapacity() call essentially blocks
the desired rawSpace() optimization, but I did not want to make
out-of-scope changes or even mark them with TODO/XXX comments, given the
tendency of those comments to be used against me by reviewers.

AFAICT, the rawSpace() call is in the right place. The reserveCapacity()
call may not be, but it does not cause the bug I am fixing. Somebody
should investigate whether [re]moving the reserveCapacity() call is
feasible. It probably is!



> It is probably better to do:
>    rawSpace(SQUID_TCP_SO_RCVBUF - readBuf.length());


(*)I do not think so: The above "greedy" parameters will "work", but we
are not trying to maximize space here because *always* getting the
maximum allowed space is often more expensive than using the available
space first.


> Which guarantees the requested size, without triggering as many
> reallocates as reserveCapacity().

The greedy call guarantees more space than we need to make progress.
With the misplaced reserveCapacity() call above, it probably does not
have any [negative or positive] effect, but if somebody [re]moves that
reserveCapacity() call, the greedy parameters will start working against us.


> client_side.cc/http.cc both also use reserveSpace(N) instead of
> rawSpace(N). Which may reallocate a lot more. Seems like a mistake to
> avoid the SBuf API marked "EXTREME caution". At least for I/O.

The SBuf space-related API is broken IMO. Nearly all of the related
methods need an "EXTREME caution" mark, for various context-dependent
reasons. This is a side effect of using SBuf for both "raw I/O buffer"
and "meaningful string" roles. I regret not insisting on role separation
at the time when SBuf was being added.


> So lots more reading into many smaller buffers. That was one of the
> goals right ? :-P

Unknown to me: IIRC, the attempts to actually establish a clear
performance model for SBuf I/O code have failed. Most of the work was
focusing on "meaningful string" manipulations. The "How are we going to
implement and integrate read/decode and encode/write loops efficiently?"
question was raised a few times, but I do not remember it getting any
serious traction.


Thank you for checking other spaceSize() uses.


> +1, Please apply ASAP after considering the above mentioned alteration
> of rawSpace(1).


Committed to trunk (r14180). Please see above for the rawSpace()
parameter value discussion at (*).


Thank you,

Alex.