On Mon, Oct 19, 2015 at 6:22 PM, Ivan Zhakov <ivan_at_visualsvn.com> wrote:
> On 7 October 2015 at 18:10, Julian Foad <julianfoad_at_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.
Received on 2015-10-20 11:23:19 CEST