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

On 07/26/2013 03:27 PM, Kinkie wrote:
> 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".

I do not see the difference because "lots of data" is going to be
appended to "your internal storage" so both are meddling with that
storage. To me, the difference was simply in argument math: Sometimes it
is easier to specify the total capacity, sometimes the available space.

The way the two functions are implemented now the difference is more
than just math. One does not guarantee exclusive ownership and the other
one does. Your semantics description above does not seem to convey that
key difference.

Here is the use case I am thinking about:

    sbuf.reserveSpace(ioSize);
    bytesRead = read(sbuf.rawSpace(), ioSize);
    sbuf.forceSize(sbuf.size() + bytesRead);

The above case is already handled by rawSpace(), but now I am confused
why rawSpace() is implemented using unconditional cow() while
reserveSpace() appears to be optimizing something. That optimization
seems to be the key here. Perhaps rawSpace() should be deleted and
reserveCapacity() adjusted to use the same optimization?

  reserveSpace(n) ought to be reserveCapacity(content size + n)

To summarize,

1) We need to agree whether the optimization currently residing in
reserveSpace() should apply to reserveCapacity(). This is the key
difference between the two methods. Documentation and code should be
adjusted accordingly.

2) We probably should not have both reserveSpace() and rawSpace().

HTH,

Alex.
Received on Fri Jul 26 2013 - 23:38:22 MDT