On 20 October 2015 at 12:23, Stefan Fuhrmann <stefan.fuhrm...@wandisco.com> wrote: > 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. > Thanks a lot for review!
> Maybe get rid of the scratch_pool because > we won't use it nor do any of the current > callers care. > I think it's better to leave it: maybe we use it future. It also makes upgrade to new API easier for callers. >> [[[ >> 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. > Yes, we resize buffer to final_size * 2 if final_size == LEN_HINT + 1. But I'm not sure that it could be fixed: we need to read more data from source stream until we got partial read. And we always increase read size by two after every read. -- Ivan Zhakov
