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.
>> [[[
>> 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
Received on 2015-10-20 11:36:22 CEST