On 7 October 2015 at 18:10, Julian Foad <julianfoad_at_gmail.com> wrote:
> 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.
>
FWIW all current callers of svn_string_from_stream() uses the pool for
SCRATCH and RESULT pool. In this case proposed implementation will
waste twice memory, still perform more re-allocations and memcpy().
> 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.
>
+1.
--
Ivan Zhakov
Received on 2015-10-07 17:20:56 CEST