Re: [RFC] Time to talk about StringNG merge again?

Resending due to MUA insisting on using HTML email..

On Thu, Jul 18, 2013 at 1:06 AM, Alex Rousskov
<rousskov_at_measurement-factory.com> wrote:
>
> On 07/04/2013 11:51 PM, Kinkie wrote:
> >>> void
> >>> >> SBuf::reserveCapacity(size_type minCapacity)
> >>> >> {
> >>> >> Must(0 <= minCapacity && minCapacity <= maxSize);
> >>> >> reserveSpace(minCapacity-length());
> >>> >> }
> >> >
> >> > This does not look right. For example, if minCapacity is smaller than
> >> > length(), we should do nothing, but will throw instead. I think this
> >> > method should look something like this:
> >> >
> >> > Must(0 <= minCapacity && minCapacity <= maxSize);
> >> > cow(minCapacity);
>
>
> >>> >> if (store_->canAppend(off_+len_, minSpace)) {
> >>> >> debugs(24, 7, "not growing");
> >>> >> return;
> >>> >> }
>
> >> > AFAICT, reserveSpace() should be implemented using
> >> > something like this:
> >> >
> >> > Must(0 <= minSpace && minSpace <= maxSize - length());
> >> > reserveCapacity(length() + minSpace);
>
>
> > The comment is badly worded but the code is correct:
>
> I am not sure which code you are referring to here, but I do not think
> it is correct for SBuf::reserveCapacity(minCapacity) to throw when
> minCapacity is smaller than length().

Decoupled the methods, but the other way around:
reserveCapacity(totalCapacity) ensures unique ownership,
reserveSpace(capacityToAdd) is an optimization to prime the SBuf to
receive data.
The upper bound in that Must() clause is actually redundant, as in
case of a needed reallocation
reserveSpace() -> cow() -> reAlloc() which throws a
SBufTooBigException if size is too big.
As a consequence, I'm removing the upper bound check.
Once we change size_type to be unsigned, the lower bound check will be
redundant too.

>
>
>
> > A decision needs to be made if these two methods have different
> > purposes or are to be simply different expressions of the same
> > underlying concept (as it is now).
>
> Sorry, this question is too abstract for me to answer it -- it feels
> like both statements are true: The methods have different purpose but
> are manipulating the same underlying concept of available buffer area.
> One method deals with the total size of the buffer area, while the other
> is concerned with the size of the yet unused buffer area.

As they are (were) one of the two is redundant, an in fact it was two
lines of code.
With the new semantics, reserveSpace is meant to announce to the SBuf
"I'm planning to append lots of data, please be prepared", while
reserveCapacity means "I'm going to meddle with your internal storage,
and I'm going to need this space. Get ready".

>
> > IMO, thinking about it, it does make sense that reserveSpace also
> > guarantees that after the call the MemBlob will be exclusively owned
> > by this SBuf (aka, blindly cow()), while reserveCapacity can mostly be
> > meant as an optimization, to hint the SBuf memory management system
> > that an append operation of a known size is about to happen, so that
> > only one cow is performed.
> > What do you think?
>
> I think that both methods should be adjusted, possibly as sketched
> above. They should not throw when the required space or capacity is
> available. The documentation should tell users that both reserve*()
> methods ensure single-ownership of the buffer by the caller.

If both methods have the exact same semantics, one of the two is
redundant. Do you agree to the semantics above?
If you do, we're ready for a next round of reviews.

The code is at lp:~squid/squid/stringng-cherrypick; if you make any
changes, please do them to the work-branch lp:~squid/squid/stringng.

Thanks!

-- 
    /kinkie