On Tue, Oct 20, 2015 at 11:35 AM, Ivan Zhakov <i...@visualsvn.com> wrote:
> 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. > ok. > >> [[[ > >> 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. > I guess you are right. I was thinking of something like if (to_read ==0) resize(); but as it turns out, it will re-alloc under the same circumstances as you patch due to the way MIN_READ_SIZE is used. -- Stefan^2.
