On 19 October 2015 at 19:22, 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
> [[[
> 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
Received on 2015-10-22 19:11:16 CEST