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

On Sun, Jul 28, 2013 at 1:03 AM, Alex Rousskov
<rousskov_at_measurement-factory.com> wrote:
> On 07/27/2013 02:57 PM, Kinkie wrote:
>> On Sat, Jul 27, 2013 at 10:32 PM, Alex Rousskov
>> <rousskov_at_measurement-factory.com> wrote:
>>> On 07/27/2013 01:03 PM, Kinkie wrote:
>>>> On Sat, Jul 27, 2013 at 8:31 PM, Alex Rousskov wrote:
>>>>> On 07/27/2013 12:00 PM, Kinkie wrote:
>>>>>>>> 1a. Reserve total buffer capacity. Ensure exclusive buffer ownership.
>>>>>>>>
>>>>>>>> 1b. Reserve buffer space. Ensure exclusive buffer ownership.
>>>>>>>>
>>>>>>>> 2. Reserve N space bytes for the caller to append to. No guarantees
>>>>>>>> regarding buffer ownership are provided.
>>>
>>>>> // 1a.
>>>>> void reserveCapacity(size_type minCap) {
>>>>> cow(minCap);
>>>>> }
>>>>>
>>>>> // 1b.
>>>>> void reserveSpace(size_type minSpace) {
>>>>> // check that the sum below does not exceed size_type
>>>>> Must(size() <= size_type's limit - minSpace);
>>>>> reserveCapacity(size() + minSpace);
>>>>> }
>>>>>
>>>>> // 2.
>>>>> char *rawSpace(size_type minSpace) {
>>>>> ... your optimization goes here ...
>>>>> return pointer to space;
>>>>> }
>
>> Current call chain (and functions for this purpose, outlined):
>> append (various flavors) calls
>> lowAppend which calls
>> reserveSpace which
>> checks if we're at the tail of the used portion of the store
>> (via store->canAppend())
>> reAlloc() if needed // reAlloc is the low-level, unconditional
>> part of cow()
>> (back to lowAppend): tell the MemBlob to append the user-supplied
>> buffer and adjust internal data
>
> Yes, I know. The current code is inconsistent/confusing because
> reserveSpace does not guarantee exclusive buffer ownership but
> reserveCapacity does. In the fixed design, that inconsistency is removed.

I agree with you that it is clearer that both reserve* methods have
the same semantics. Let's consider it done: they do the same thing and
one is an inline wrapper of the other.

The way I envisioned the key guarantee difference between reserve* and
cow() is that: reserve* guarantees that it is safe to APPEND after the
currently-used area and up to the requested size/space. cow() slso
guarantees that it is safe to WRITE over the currently-used area, up
to the requested size.

The former is a weaker guarantee, and a possible optimization. What I
understand about your request is that you want the stronger guarantee
to be always used. It's not a big deal, as the cost is at most some
unneeded copies here and there, and code-wise it's very simple. It'd
just alias reserveCapacity to cow, and reserveSpace to wrap on top of
that.
Did I understand correctly? If so, I can do that right away.

The other size is rawSpace(). For that, I'd like to adopt Amos's
suggestion and enforce that the requester specify how much storage he
needs. If he wants raw access to the data, he better know what he's
doing.

I don't like the idea of using rawSpace for append; MemBlob::append
also does some housekeeping within MemBlob and I'm not keen on the
idea shuffling that outside the class itself. It is however immaterial
to this discussion, as it's an implementation detail from the SBuf
user's point of view.

> The current design is inconsistent (similar-sounding methods behave very
> differently) and more dangerous (reserveSpace() does not return volatile
> raw space to the caller, increasing the changes that the caller will do
> something bad before extracting that [now stale] space from the SBuf).
> The proposed design solves both problems.

It may be useful to document that reserve* methods are meant to be
used only in preparation for other SBuf methods (assign, append, setAt
etc., not for direct manipulation by the caller.
This is implied by the documentation of rawSpace: ("\warning Use with
EXTREME caution, this is a dangerous operation.").

To wrap it up, if you agre I will:
- alias reserveCapacity(size) to cow(size)
- reserveSpace(size) -> reserveCapacity(size+length())
- rawSpace(size_type size=npos) -> rawSpace(size_type size)

And leave everything else as is.
Do you agree?

-- 
    /kinkie