On 7 October 2015 at 18:10, Julian Foad <julianf...@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
