Re: Sbuf review at r9331

On 02/20/2009 12:50 PM, Kinkie wrote:
>> 2) Confusion regarding pooling of SBuf objects is suspected (discussed
>> in a few bullets below).
>>
> Given your explanation it is now a certainty.
> On the other hand, SBufStore objects would probably benefit from it,
> wouldn't they?
>
Yes, they would.
>>>> * I doubt clear() should free the storage by default. Usually, we clear to
>>>> fill again. You even do that yourself in SBuf::assign(). Deallocating and
>>>> allocating would cost more than doing nothing. Let's not deallocate by
>>>> default, for now.
>>>>
>>> It is a possible optimization, but it's tricky and it involves
>>> checking refcounts and slicing history of the SBufStore, and the same
>>> practical result can be obtained by mempools (as it is now). In other
>>> words, I'd leave it as-is.
>>>
>> I do not foresee any new tricks and I believe one of us is confused
>> about MemPools in SBuf context (see above). AFAICT, SBuf::clear() can be
>> implemented as
>>
>> cow(); // noop if less than two references
>> offset = 0;
>> len = 0;
>>
> This doesn't free any memory back to the system, which for long-lived
> objects (think MemBuf-alikes) is not really a good thing IMVHO, for
> instance because it skewers the reallocs counter..
> What I currently do is implement a Prototype pattern, with a static
> zero-length MemBlob which gets assigned. Is this reasonable?
>
In most cases, clear() is used to efficiently reset the buffer for
immediate use, not to free it for long-term storage. Thus, it is a
_good_ thing that clear() does not free memory. That is the whole point
of my comment. We need a method that clears the content, leaving the
allocated memory intact.

I am not sure we need a method to free the memory since a regular
assignment would do:

    buf = Sbuf();

>>>> * See my earlier StringNg comments regarding truncateSelf, chopSelf,
>>>> import*, export*, substrSelf, and headUntil* naming and existence. Trim them
>>>> down to basics, use standard names, and remove "Self".
>>>>
>>> They're now all gone except for substr and substrSelf. The dualism is
>>> needed because the former creates a new String and the latter modifies
>>> in-place, which is not part of the std::string API. If you can suggest
>>> a better name, I'll gladly change it.
>>>
>> I bet std::string does not have substrSelf() because it can be easily
>> and cheaply written as
>>
>> s = s.substr(...);
>>
>> If you insist on leaving substrSelf in, then please call it leave(from,
>> to), focus(from, to), or zoom(from, to).
>>
>
> it can be substituted, but then it also doesn't have consume() which
> is a very handy tool for parsers for intance. The reworked Tokenizer
> is IMO very lean, partly as a result of in-place chopping of Buffers.
>
> zoom sounds fine. Otherwise, I'd dare suggest "slice"
>
Slicing produces slices. Here you chop the ends off without producing
any new substrings.

Trim() is a candidate although it is usually used to chop whitespace off.
>>>> * remove dumpStats. Just give access to Stats object(s) that can dump.
>>>>
>>> Are you sure? We're all friends and we're not supposed to be meddling
>>> with the contents of stats, but this makes it possible to enforce the
>>> policy.
>>>
>> I am sure. Give _const_ access to enforce the policy.
>>
>
> Ok. This means that the getter will remain (IIRC it was also a
> suggestion of yours to remove it).
>
I would remove it and trust the caller to only pass const-references to
stats objects, but it is not a big deal either way. Leaving it is fine.
If the stats object is static, make sure the getter is static.
> I will however object to anyone doing anything stupid such as
> append("") to trigger a cow().
>
Agreed.
>>> nreallocs tracks the history of a SBuf, not of its contents.
>>>
>> I think this is a bad approach because folks expect assignment to assign
>> everything, including history. They expect a copy constructor and an
>> assignment operator to have the same basic outcome.
>>
>> If you insist that your assignment should not be what an average
>> developer expects, add a comment explaining why some of the data members
>> are not assigned.
>>
> I'll ponder on this some more.
> I don't believe it to be a huge issue tho, as it's part of the
> heuristics that are mostly useful to estimateCapacity() - aka very
> much behind-the-scenes work, which will certainly require tweaking
> once StringNg is actuall used.
> Some of the questions intentionally left unresolved are:
> - what sizes and how many of them to use for MemBlobs' backing storage
> - how much headroom to leave
> - what algorithms to use to adapt the headroom left - if adapting is
> useful at all
>
> All of those estimates are concentrated in estimateCapacity. I do not
> expect to get it right on the first try.
>
I agree that we do not know the best answers to these questions or even
the stats we need to answer them. Since we do not know, let's keep it
simple and avoid surprises. We can add strange nreallocs exceptions such
as assignment avoidance later, if needed. I would remove that member
until then.
>> BTW, the whole idea of counting the number of reallocs in a
>> stack-allocated object that will be often copied to other
>> stack-allocated objects seems rather strange to me. You are not going to
>> get any usable stats from this, IMO.
>>
> I'm thinking in terms of data-members of other structs.
> producers/consumers are the main targets of this.
>
I still do not understand your motivation here so my advice may be wrong.

This is not a huge issue.

HTH,

Alex.
Received on Fri Feb 20 2009 - 20:56:11 MST