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
