On Mon, Oct 19, 2015 at 6:22 PM, Ivan Zhakov <i...@visualsvn.com> wrote:
> 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. > > > [...] > >> 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. > > > Hi Julian, > > I propose attached two patches as follow-up to r1707196: > svn_string_from_stream2-v2.patch.txt > Not-Julian here. The patches looks good. Maybe get rid of the scratch_pool because we won't use it nor do any of the current callers care. [[[ > Revv svn_string_from_stream() function and share implementation with > svn_stringbuf_from_stream(). > > Suggested by: julianf > > * subversion/include/svn_io.h > (svn_string_from_stream2): New. > (svn_string_from_stream): Deprecate. > > * subversion/libsvn_subr/stream.c > (svn_string_from_stream2): Revv from svn_string_from_stream(): add > LEN_HINT > argument. Use svn_stringbuf_from_stream() as implementation. > > * subversion/libsvn_subr/deprecated.c > (svn_string_from_stream): Call svn_string_from_stream2() with LEN_HINT=0. > > * subversion/libsvn_fs_x/reps.c > * subversion/libsvn_wc/old-and-busted.c > * tools/dev/x509-parser.c > (svn_fs_x__reps_add_base, svn_wc__read_entries_old, > get_der_cert_from_stream): Use svn_string_from_stream2() with > LEN_HINT=SVN__STREAM_CHUNK_SIZE. It doesn't increase memory usage > because > we use same pool for SCRATCH and RESULT pool. > ]]] > > And svn_stringbuf_from_stream-v1.patch.txt > [[[ > * subversion/libsvn_subr/stream.c > (svn_stringbuf_from_stream): Optimize memory usage a bit and avoid > svn_stream_read_full() call once we got partial read. > ]]] > Minor nitpick: If LEN_HINT < final size, we may resize the buffer unnecessarily, e.g. for final_size==LEN_HINT+1. -- Stefan^2.
