Re: [PATCH] SBufList

On 02 Dec 2013, at 21:36, Alex Rousskov <rousskov_at_measurement-factory.com> wrote:

> On 12/02/2013 11:34 AM, Kinkie wrote:
>
>> + SBuf operator() (SBuf accumulated, const SBuf &item) {
>> + if (first) {
>> + first = false;
>> + accumulated.append(item);
>> + return accumulated;
>> + }
>> + accumulated.append(separator_).append(item);
>> + return accumulated;
>> + }
>
>> + SBuf rv;
>> + rv.reserveSpace(sz);
>> + rv = std::accumulate(items.begin(), items.end(), rv, SBufAddContent(separator));
>
>
> The above combination feels wrong. Would not the above result in the
> pre-allocated accumulation buffer (rv) disappearing after the first
> operation, and every consecutive operation allocating a new, bigger buffer?

The only way to be sure is tracing, but it shouldn't, as each append operation is at the tail of the backing MemBlob's used portion. After some testing, I can confirm that:

Before: SBuf51: id @0x16a4d50mem:0x16a8f00,capacity:16388,size:0,refs:1; , offset:0, len:0) : ''
After: SBuf51: id @0x16a4d50mem:0x16a8f00,capacity:16388,size:44,refs:1; , offset:0, len:44) : 'The quick brown fox jumped over the lazy dog'

(Before is dumped immediately before the std::accumulate, and after immediately after)

> If my suspicion is correct, then I think the solution is to move away
> from std::accumulate for content accumulation. That algorithm is from
> <numerics> so perhaps it was never meant for this kind of in-place
> buffering optimization. We can use std::for_each instead (placing the
> accumulation buffer inside the AddContent function class, just like we
> placed the separator) but perhaps you can find a more appropriate
> algorithm at http://www.cplusplus.com/reference/algorithm/

I thought about that as well; this design however seemed quite clear and workable, and I could find no reason not to use it :)

> The first std::accumulate use (to calculate the size) seems OK.
>
> And yes, I know I used std::accumulate twice in my sketch. This just
> proves that STL is not my strong suit (and illustrates why I hate
> sketching things and effectively ending up taking responsibility for any
> problems).

Are you kidding? This sketch is great IMO, no problems to solve. PLUS I was able to learn something :)
Computation cost went down from somewhere between 4*N and 6*N depending on implementation to 2*N. I'd call it a win.

>>>> plus a specialized SBufListJoin which is simply an instantiation of
>>>> SBufContainerJoin<SBufList>?
>>>
>>> What will that specialization do differently? I do not think we need it
>>> but perhaps I am missing something.
>>
>> As above: just convenience,
>
> Calling SBufContainerJoin(l, s) is as convenient as calling
> SBufListJoin(l, s), right? I do not see the added convenience here.
> IsMember() adds a little bit of convenience, but SBufListJoin() does not
> seem to do that.

True.
Removing it.

>> +/** convert a wordlist to a SBufList
>> + */
>
> Using /// would save you one line and would prevent the comment from
> looking so awkward :-)

Done

>> +SBufList wordlistToSbufList(wordlist *);
>
> This one can be called ToSbufList(). The compiler will know you are
> converting wordlist based on the parameter type. There may be more
> functions like that in the future...

It should actually be called ToSBufList. Corrected.

Please let me know if there's any other questions, or if it's OK to merge now :)

  Thanks!
  Kinkie
Received on Mon Dec 02 2013 - 21:04:10 MST