Stefan Fuhrmann wrote:
> Julian Foad wrote:
>> Stefan: First review comment: You can much more efficiently convert a
>> stringbuf to a string (when you own it, like here) using
>> svn_stringbuf__morph_into_string().
>
> We could only do that if the stringbuf was allocated in
> the result_pool. The original implementation allocates
> the read buffer in the scratch_pool and I wanted to
> preserve that characteristic.
I don't entirely follow your reasoning. What positive characteristic
you are preserving? The old version of svn_string_... did the
stringbuffer [re]allocation in the result pool and also used a read
buffer in the scratch pool. Your version eliminates the read buffer,
moves the stringbuffer [re]allocation to the scratch pool, eliminates
one copying of the data during the loop, and then adds back an extra
copying at the end. AFAICT that reduces the result pool usage (a good
thing in itself), changes scratch pool usage (which doesn't matter),
and keeps the original amount of copying in total. The alternative
mentioned above, using result pool only, would preserve the original
amount of result pool allocation, eliminate scratch pool usage
(doesn't matter), and eliminate one copying of the data.
So, if I did the analysis right, you chose to reduce the memory
(result pool) usage rather than to reduce the run time. That's not a
bad choice, but it wasn't at all obvious, and I don't know if you
intended it or if I have misunderstood it and made a wrong conclusion?
And if the caller benefits from use of a scratch pool, then surely you
will want callers of the _stringbuf_ variant to be able to get the
same benefit, won't you? If so, you'll want to rev that API to take a
scratch pool too.
What I don't want is two functions that look functionally identical
but have hidden subtle differences -- this one optimized for speed and
this one optimized for space. It's fine to have such variants, if we
want them, but not as hidden and subtle differences. If you want such
a difference, make it explicit.
> We might want to use
> SVN__STREAM_CHUNK_SIZE as LEN_HINT, though
> to prevent a few reallocations for larger streams.
>
>> Second review question: did you
>> consider whether the second implementation was in fact already better?
>> For example, is using _appendbytes better or worse than using
>> ensure(size x 2) followed by read()?
>
> The appendbytes approach needs a separate buffer to
> hold those bytes before they get appended. Ultimately,
> it implements the same x2 growth scheme but adds a
> copy and a temporary buffer.
OK, that's totally clear, and after your comment above about using
SVN__STREAM_CHUNK_SIZE as LEN_HINT, it sounds like you have considered
the differences. That was all I wanted to hear. Thanks.
- Julian
Received on 2015-10-07 17:11:19 CEST