On 19 October 2015 at 19:22, 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 > [[[ > 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. > ]]] > Committed in r1710065.
> 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. > ]]] > Committed in r1710066. -- Ivan Zhakov
