On Tue, Oct 20, 2015 at 11:35 AM, Ivan Zhakov <ivan_at_visualsvn.com> wrote:
> On 20 October 2015 at 12:23, Stefan Fuhrmann
> <stefan.fuhrmann_at_wandisco.com> wrote:
> > 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.
> >
> 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.
Received on 2015-10-20 11:48:22 CEST