subversion-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Ivan Zhakov <i...@visualsvn.com>
Subject Re: svn commit: r1707196 - /subversion/trunk/subversion/libsvn_subr/stream.c
Date Mon, 19 Oct 2015 16:22:57 GMT
On 7 October 2015 at 18:10, Julian Foad <julianfoad@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.
]]]

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.
]]]



-- 
Ivan Zhakov

Mime
View raw message