You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@subversion.apache.org by Bert Huijben <be...@qqmail.nl> on 2015/10/07 13:08:48 UTC
RE: svn commit: r1707196 - /subversion/trunk/subversion/libsvn_subr/stream.c
> -----Original Message-----
> From: ivan@apache.org [mailto:ivan@apache.org]
> Sent: woensdag 7 oktober 2015 09:57
> To: commits@subversion.apache.org
> Subject: svn commit: r1707196 -
> /subversion/trunk/subversion/libsvn_subr/stream.c
>
> Author: ivan
> Date: Wed Oct 7 07:57:05 2015
> New Revision: 1707196
>
> URL: http://svn.apache.org/viewvc?rev=1707196&view=rev
> Log:
> Slightly optimize svn_stringbuf_from_stream() to avoid allocating twice
> more memory and unnecessary memcpy() when LEN_HINT is equal to final
> stringbuf
> length.
>
> * subversion/libsvn_subr/stream.c
> (svn_stringbuf_from_stream): Always preallocate LEN_HINT +
> MIN_READ_SIZE
> bytes to be able perform final read without stringbuf reallocation.
Can you explain why hint + MIN_READ_SIZE instead of something like
MAX(len_hint+1, MIN_READ_SIZE)
I don't know what MIN_READ_SIZE is from just this patch, but it could easily be something like 16 Kbyte, while len_hint could be something like 16 for a file like 'format' that was just statted to obtain the actual size
16 Kbyte + 16 bytes for a 16 bytes file looks a bit large... And allocating 16 + 32 bytes is far more efficient than allocating that huge chunk at once.
Bert Huijben
>
> Modified:
> subversion/trunk/subversion/libsvn_subr/stream.c
>
> Modified: subversion/trunk/subversion/libsvn_subr/stream.c
> URL:
> http://svn.apache.org/viewvc/subversion/trunk/subversion/libsvn_subr/str
> eam.c?rev=1707196&r1=1707195&r2=1707196&view=diff
> ==========================================================
> ====================
> --- subversion/trunk/subversion/libsvn_subr/stream.c (original)
> +++ subversion/trunk/subversion/libsvn_subr/stream.c Wed Oct 7 07:57:05
> 2015
> @@ -1490,7 +1490,7 @@ svn_stringbuf_from_stream(svn_stringbuf_
>
> apr_size_t to_read = 0;
> svn_stringbuf_t *text
> - = svn_stringbuf_create_ensure(len_hint ? len_hint : MIN_READ_SIZE,
> + = svn_stringbuf_create_ensure(len_hint + MIN_READ_SIZE,
> result_pool);
>
> do
>
Re: svn commit: r1707196 - /subversion/trunk/subversion/libsvn_subr/stream.c
Posted by Stefan Fuhrmann <st...@wandisco.com>.
On Wed, Oct 7, 2015 at 1:53 PM, Julian Foad <ju...@gmail.com> wrote:
> Ivan Zhakov wrote:
> > Bert Huijben wrote:
> >>> URL: http://svn.apache.org/viewvc?rev=1707196&view=rev
> >>> Log:
> >>> Slightly optimize svn_stringbuf_from_stream() to avoid allocating twice
> >>> more memory and unnecessary memcpy() when LEN_HINT is equal to final
> >>> stringbuf
> >>> length.
> >>>
> >>> * subversion/libsvn_subr/stream.c
> >>> (svn_stringbuf_from_stream): Always preallocate LEN_HINT +
> >>> MIN_READ_SIZE
> >>> bytes to be able perform final read without stringbuf reallocation.
> >>
> >> Can you explain why hint + MIN_READ_SIZE instead of something like
> >> MAX(len_hint+1, MIN_READ_SIZE)
> >>
> >> I don't know what MIN_READ_SIZE is from just this patch, but it could
> easily be
> >> something like 16 Kbyte, while len_hint could be something like 16 for
> a file like
> >> 'format' that was just statted to obtain the actual size
> >>
> >> 16 Kbyte + 16 bytes for a 16 bytes file looks a bit large... And
> allocating 16 + 32
> >> bytes is far more efficient than allocating that huge chunk at once.
> >>
> > MIN_READ_SIZE is 64 bytes and it's locally defined in this function.
> > We cannot use MAX(len_hint+1, MIN_READ_SIZE) because main loop
> > reallocates buffer if don't have MIN_READ_SIZE remaining. The current
> > code assumes that MIN_READ_SIZE is small value and I decided to leave
> > this code intact. It could be improved of course, but I wanted to make
> > minimum change first.
>
> I don't think that was the right fix. One deficiency with the code is
> that it doesn't break out of the loop when end-of-stream is detected,
> which can be reported by a 'short' read. Your change doesn't solve
> that problem in general, although it does solve it for the case where
> the supplied length hint is exactly right.
>
The short read detection got changed in r1561688.
Before that, svn_stream_read would be called for
which the short read detection was correct. Since
then we have a somewhat inefficient double EOS
check.
I guess the correct way of doing this is revert Ivan's
change and apply something like the attached patch.
> We also have the function
>
> svn_string_from_stream()
>
> which performs a virtually identical task. These two functions have
> completely different implementations. svn_string_from_stream() does
> break out of the loop as soon as it detects end-of-stream reported as
> a short read. They also use completely different read sizes
> (MIN_READ_SIZE = 64 bytes vs. SVN__STREAM_CHUNK_SIZE = 16384).
>
> That is silly: there is no good reason for that, and it is a waste of
> effort maintaining them separately, and discussing separate
> optimizations for each.
>
> We should make those two functions share the same "best" implementation.
>
Both functions are rarely used within SVN. I'd say,
implement a svn_string_from_stream as "read into
stringbuf, alloc'ed in scratch_pool + copy to final string".
That preserves and actually improves upon it's current
memory usage pattern while reusing the stringbuf code.
-- Stefan^2.
Re: svn commit: r1707196 - /subversion/trunk/subversion/libsvn_subr/stream.c
Posted by Ivan Zhakov <iv...@visualsvn.com>.
On 7 October 2015 at 15:47, Julian Foad <ju...@gmail.com> wrote:
> Stefan wrote:
>> I guess the correct way of doing this is revert Ivan's
>> change and apply something like the attached patch.
> Ivan wrote:
>> Here is the patch that I wanted commit later. What do you think?
>
> I am not interested in reviewing any more patches that tweak just one
> of the implementations. If you (anybody) want to do something, please
> combine the two functions into one implementation, and then I will be
> interested in reviewing the (single) implementation.
>
Merging svn_stringbuf_from_stream() and svn_stringbuf_from_stream()
implementations is completely ortogonal from my point of view.
But I really don't understand what was your point in r1707196 review
if you are not interested in review any changes to this function at
all.
--
Ivan Zhakov
Re: svn commit: r1707196 - /subversion/trunk/subversion/libsvn_subr/stream.c
Posted by Stefan Fuhrmann <st...@wandisco.com>.
On Wed, Oct 7, 2015 at 2:57 PM, Stefan Fuhrmann <
stefan.fuhrmann@wandisco.com> wrote:
> On Wed, Oct 7, 2015 at 2:47 PM, Julian Foad <ju...@gmail.com> wrote:
>
>> Stefan wrote:
>> > I guess the correct way of doing this is revert Ivan's
>> > change and apply something like the attached patch.
>> Ivan wrote:
>> > Here is the patch that I wanted commit later. What do you think?
>>
>> I am not interested in reviewing any more patches that tweak just one
>> of the implementations. If you (anybody) want to do something, please
>> combine the two functions into one implementation, and then I will be
>> interested in reviewing the (single) implementation.
>>
>
> Alright, here you go.
>
Ugh, pitfall: svn_string_from_stream closes the
stream after reading it. Updated patch.
-- Stefan^2.
Re: svn commit: r1707196 - /subversion/trunk/subversion/libsvn_subr/stream.c
Posted by Ivan Zhakov <iv...@visualsvn.com>.
On 7 October 2015 at 18:10, Julian Foad <ju...@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.
>
> I don't entirely follow your reasoning. What positive characteristic
> you are preserving? The old version of svn_string_... did the
> stringbuffer [re]allocation in the result pool and also used a read
> buffer in the scratch pool. Your version eliminates the read buffer,
> moves the stringbuffer [re]allocation to the scratch pool, eliminates
> one copying of the data during the loop, and then adds back an extra
> copying at the end. AFAICT that reduces the result pool usage (a good
> thing in itself), changes scratch pool usage (which doesn't matter),
> and keeps the original amount of copying in total. The alternative
> mentioned above, using result pool only, would preserve the original
> amount of result pool allocation, eliminate scratch pool usage
> (doesn't matter), and eliminate one copying of the data.
>
> So, if I did the analysis right, you chose to reduce the memory
> (result pool) usage rather than to reduce the run time. That's not a
> bad choice, but it wasn't at all obvious, and I don't know if you
> intended it or if I have misunderstood it and made a wrong conclusion?
>
> And if the caller benefits from use of a scratch pool, then surely you
> will want callers of the _stringbuf_ variant to be able to get the
> same benefit, won't you? If so, you'll want to rev that API to take a
> scratch pool too.
>
FWIW all current callers of svn_string_from_stream() uses the pool for
SCRATCH and RESULT pool. In this case proposed implementation will
waste twice memory, still perform more re-allocations and memcpy().
> What I don't want is two functions that look functionally identical
> but have hidden subtle differences -- this one optimized for speed and
> this one optimized for space. It's fine to have such variants, if we
> want them, but not as hidden and subtle differences. If you want such
> a difference, make it explicit.
>
+1.
--
Ivan Zhakov
Re: svn commit: r1707196 - /subversion/trunk/subversion/libsvn_subr/stream.c
Posted by Julian Foad <ju...@gmail.com>.
On 23 October 2015, Ivan Zhakov wrote:
> On 23 October 2015, Julian Foad wrote:
>> 1. The doc string should explain len_hint in the caller's terms. The
>> doc strings also differ in other unnecessary respects. I propose the
>> attached documentation patch.
>>
> I agree. Patch look great!
Thanks.
Actually, one part of the doc string I suggested is not good:
+ * The present implementation:
+ * - uses an effective hint of 64 bytes when a lower value or zero is
+ * specified;
+ * - uses minimal time and space when the actual length is less than or
+ * equal to the effective hint;
+ * - uses around 1.5x to 2x the minimal time and space when the actual
+ * length is greater than the effective hint.
That last comment, "around 1.5x to 2x the minimal time and space", is
misleading.
On thinking more carefully, I think in fact it adds a data copying
time overhead from 1x to 2x the actual length (on top of the necessary
1x copying to get the data into result_pool), and allocates additional
memory space from 2x to 3x the actual length (on top of the necessary
space to hold the result), because of the size doubling algorithm we
use.
It's probably better if we say something less specific in the doc
string, and perhaps add the above notes in the implementation file.
People who care that much can read the source.
Committed revision 1710201.
>> 2. The _string and _stringbuf function declarations still differ in
>> that only svn_string_from_stream2() takes a scratch pool. [...]
>>
>> In the absence of any other argument, I would accept that argument and
>> remove the scratch pool.
>>
> I don't have opinion on this, so feel free to remove scratch_pool
> argument if you think that it will be useful.
Consistency is good.
Committed revision 1710202.
- Julian
Re: svn commit: r1707196 - /subversion/trunk/subversion/libsvn_subr/stream.c
Posted by Ivan Zhakov <iv...@visualsvn.com>.
On 23 October 2015 at 13:13, Julian Foad <ju...@gmail.com> wrote:
> On 22 October 2015, Ivan Zhakov wrote:
>> On 19 October 2015, Ivan Zhakov wrote:
>>> [[[
>>> Revv svn_string_from_stream() function and share implementation with
>>> svn_stringbuf_from_stream().
>>>
>>> Suggested by: julianf
> [...]
>>> ]]]
>>>
>> Committed in r1710065.
>>
>>> [[[
>>> * 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.
>
> OK, looks good. (I haven't reviewed the implementation in detail, yet.)
>
> Two more things:
>
> 1. The doc string should explain len_hint in the caller's terms. The
> doc strings also differ in other unnecessary respects. I propose the
> attached documentation patch.
>
I agree. Patch look great!
> 2. The _string and _stringbuf function declarations still differ in
> that only svn_string_from_stream2() takes a scratch pool. The current
> implementation doesn't use it. We should unify them. Which is best,
> taking a scratch pool or not?
>
> Stefan Fuhrmann wrote earlier:
>> Maybe get rid of the scratch_pool because we won't use it nor do any of the current
>> callers care.
>
> In the absence of any other argument, I would accept that argument and
> remove the scratch pool.
>
I don't have opinion on this, so feel free to remove scratch_pool
argument if you think that it will be useful.
--
Ivan Zhakov
Re: svn commit: r1707196 - /subversion/trunk/subversion/libsvn_subr/stream.c
Posted by Julian Foad <ju...@gmail.com>.
On 22 October 2015, Ivan Zhakov wrote:
> On 19 October 2015, Ivan Zhakov wrote:
>> [[[
>> Revv svn_string_from_stream() function and share implementation with
>> svn_stringbuf_from_stream().
>>
>> Suggested by: julianf
[...]
>> ]]]
>>
> Committed in r1710065.
>
>> [[[
>> * 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.
OK, looks good. (I haven't reviewed the implementation in detail, yet.)
Two more things:
1. The doc string should explain len_hint in the caller's terms. The
doc strings also differ in other unnecessary respects. I propose the
attached documentation patch.
2. The _string and _stringbuf function declarations still differ in
that only svn_string_from_stream2() takes a scratch pool. The current
implementation doesn't use it. We should unify them. Which is best,
taking a scratch pool or not?
Stefan Fuhrmann wrote earlier:
> Maybe get rid of the scratch_pool because we won't use it nor do any of the current
> callers care.
In the absence of any other argument, I would accept that argument and
remove the scratch pool.
- Julian
Re: svn commit: r1707196 - /subversion/trunk/subversion/libsvn_subr/stream.c
Posted by Ivan Zhakov <iv...@visualsvn.com>.
On 19 October 2015 at 19:22, Ivan Zhakov <iv...@visualsvn.com> wrote:
> On 7 October 2015 at 18:10, Julian Foad <ju...@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
Re: svn commit: r1707196 - /subversion/trunk/subversion/libsvn_subr/stream.c
Posted by Stefan Fuhrmann <st...@wandisco.com>.
On Tue, Oct 20, 2015 at 11:35 AM, Ivan Zhakov <iv...@visualsvn.com> wrote:
> On 20 October 2015 at 12:23, Stefan Fuhrmann
> <st...@wandisco.com> wrote:
> > On Mon, Oct 19, 2015 at 6:22 PM, Ivan Zhakov <iv...@visualsvn.com> wrote:
> >>
> >> On 7 October 2015 at 18:10, Julian Foad <ju...@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.
Re: svn commit: r1707196 - /subversion/trunk/subversion/libsvn_subr/stream.c
Posted by Ivan Zhakov <iv...@visualsvn.com>.
On 20 October 2015 at 12:23, Stefan Fuhrmann
<st...@wandisco.com> wrote:
> On Mon, Oct 19, 2015 at 6:22 PM, Ivan Zhakov <iv...@visualsvn.com> wrote:
>>
>> On 7 October 2015 at 18:10, Julian Foad <ju...@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
Re: svn commit: r1707196 - /subversion/trunk/subversion/libsvn_subr/stream.c
Posted by Stefan Fuhrmann <st...@wandisco.com>.
On Mon, Oct 19, 2015 at 6:22 PM, Ivan Zhakov <iv...@visualsvn.com> wrote:
> On 7 October 2015 at 18:10, Julian Foad <ju...@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.
Re: svn commit: r1707196 - /subversion/trunk/subversion/libsvn_subr/stream.c
Posted by Ivan Zhakov <iv...@visualsvn.com>.
On 7 October 2015 at 18:10, Julian Foad <ju...@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
Re: svn commit: r1707196 - /subversion/trunk/subversion/libsvn_subr/stream.c
Posted by Julian Foad <ju...@gmail.com>.
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.
I don't entirely follow your reasoning. What positive characteristic
you are preserving? The old version of svn_string_... did the
stringbuffer [re]allocation in the result pool and also used a read
buffer in the scratch pool. Your version eliminates the read buffer,
moves the stringbuffer [re]allocation to the scratch pool, eliminates
one copying of the data during the loop, and then adds back an extra
copying at the end. AFAICT that reduces the result pool usage (a good
thing in itself), changes scratch pool usage (which doesn't matter),
and keeps the original amount of copying in total. The alternative
mentioned above, using result pool only, would preserve the original
amount of result pool allocation, eliminate scratch pool usage
(doesn't matter), and eliminate one copying of the data.
So, if I did the analysis right, you chose to reduce the memory
(result pool) usage rather than to reduce the run time. That's not a
bad choice, but it wasn't at all obvious, and I don't know if you
intended it or if I have misunderstood it and made a wrong conclusion?
And if the caller benefits from use of a scratch pool, then surely you
will want callers of the _stringbuf_ variant to be able to get the
same benefit, won't you? If so, you'll want to rev that API to take a
scratch pool too.
What I don't want is two functions that look functionally identical
but have hidden subtle differences -- this one optimized for speed and
this one optimized for space. It's fine to have such variants, if we
want them, but not as hidden and subtle differences. If you want such
a difference, make it explicit.
> 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.
- Julian
Re: svn commit: r1707196 - /subversion/trunk/subversion/libsvn_subr/stream.c
Posted by Stefan Fuhrmann <st...@wandisco.com>.
On Wed, Oct 7, 2015 at 3:21 PM, Julian Foad <ju...@gmail.com> wrote:
> >>> Julian Foad wrote:
> >>>> [...] I will be
> >>>> interested in reviewing the (single) implementation.
>
> Stefan wrote:
> > [...] Here the final version.
> > If that doesn't work either then I'm done for today.
>
> 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.
-- Stefan^2.
Re: svn commit: r1707196 - /subversion/trunk/subversion/libsvn_subr/stream.c
Posted by Julian Foad <ju...@gmail.com>.
Ivan Zhakov wrote:
> Julian Foad wrote:
>> Ivan Zhakov wrote:
>>> Thanks for pointing to svn_string_from_stream(), but this function
>>> slightly different: it has SCRATCH_POOL argument and doesn't have
>>> LEN_HINT argument. It's little difference in semantic.
>>
>> Yes, it is slightly different. That doesn't justify it having a
>> completely different implementation, does it? The length hint in
>> _stringbuf... is optional, so it also has to operate without a length
>> hint like _string_ does. The lack of a separate scratch pool is not a
>> meaningful semantic difference.
>>
>> I would also add: why should they have slightly different API forms?
>> They should not. Stupid differences like this just make things harder
>> for no good reason. Let's make them identical, deprecating and bumping
>> one or both of them to do so.
>>
>> It seems obvious to me that these functions are basically intended to
>> do the same job, just with different output data type. Do you
>> disagree? And if that is so, then they should have a common
>> implementation. Do you disagree?
>
> It looks like some misunderstanding between us: I never told that we
> should not merge them to one implementation. I'm just saying that this
> is ortogonal: making svn_string_from_stream() wrapper around
> svn_stringbuf_from_stream() is one task. Making
> svn_stringbuf_from_stream() work efficient in all cases is another. My
> point that svn_stringbuf_from_stream() should work fine in all cases:
> whether it was called through svn_string_from_stream() or directly.
>
> My personal opinion that svn_string_from_stream() should just call
> svn_stringbuf_from_stream() with result pool and morph result to
> string_t.
OK, good. Thanks for explaining your opinion. I basically agree with
you. (I don't care about exactly how they are combined, in detail.)
When you wrote "Thanks for pointing to svn_string_from_stream(), but
[it's different]" I (wrongly) thought you were implying that these
differences made my argument invalid. That's where I misunderstood
you.
- Julian
Re: svn commit: r1707196 - /subversion/trunk/subversion/libsvn_subr/stream.c
Posted by Ivan Zhakov <iv...@visualsvn.com>.
On 7 October 2015 at 16:50, Julian Foad <ju...@gmail.com> wrote:
> Ivan Zhakov wrote:
>> Thanks for pointing to svn_string_from_stream(), but this function
>> slightly different: it has SCRATCH_POOL argument and doesn't have
>> LEN_HINT argument. It's little difference in semantic.
>
> Yes, it is slightly different. That doesn't justify it having a
> completely different implementation, does it? The length hint in
> _stringbuf... is optional, so it also has to operate without a length
> hint like _string_ does. The lack of a separate scratch pool is not a
> meaningful semantic difference.
>
> I would also add: why should they have slightly different API forms?
> They should not. Stupid differences like this just make things harder
> for no good reason. Let's make them identical, deprecating and bumping
> one or both of them to do so.
>
> It seems obvious to me that these functions are basically intended to
> do the same job, just with different output data type. Do you
> disagree? And if that is so, then they should have a common
> implementation. Do you disagree?
>
It looks like some misunderstanding between us: I never told that we
should not merge them to one implementation. I'm just saying that this
is ortogonal: making svn_string_from_stream() wrapper around
svn_stringbuf_from_stream() is one task. Making
svn_stringbuf_from_stream() work efficient in all cases is another. My
point that svn_stringbuf_from_stream() should work fine in all cases:
whether it was called through svn_string_from_stream() or directly.
My personal opinion that svn_string_from_stream() should just call
svn_stringbuf_from_stream() with result pool and morph result to
string_t.
--
Ivan Zhakov
Re: svn commit: r1707196 - /subversion/trunk/subversion/libsvn_subr/stream.c
Posted by Julian Foad <ju...@gmail.com>.
Ivan Zhakov wrote:
> Thanks for pointing to svn_string_from_stream(), but this function
> slightly different: it has SCRATCH_POOL argument and doesn't have
> LEN_HINT argument. It's little difference in semantic.
Yes, it is slightly different. That doesn't justify it having a
completely different implementation, does it? The length hint in
_stringbuf... is optional, so it also has to operate without a length
hint like _string_ does. The lack of a separate scratch pool is not a
meaningful semantic difference.
I would also add: why should they have slightly different API forms?
They should not. Stupid differences like this just make things harder
for no good reason. Let's make them identical, deprecating and bumping
one or both of them to do so.
It seems obvious to me that these functions are basically intended to
do the same job, just with different output data type. Do you
disagree? And if that is so, then they should have a common
implementation. Do you disagree?
- Julian
Re: svn commit: r1707196 - /subversion/trunk/subversion/libsvn_subr/stream.c
Posted by Ivan Zhakov <iv...@visualsvn.com>.
On 7 October 2015 at 16:21, Julian Foad <ju...@gmail.com> wrote:
>>>> Julian Foad wrote:
>>>>> [...] I will be
>>>>> interested in reviewing the (single) implementation.
>
> Stefan wrote:
>> [...] Here the final version.
>> If that doesn't work either then I'm done for today.
>
> 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(). 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()?
>
> Ivan wrote:
>> Merging svn_stringbuf_from_stream() and svn_stringbuf_from_stream()
>> implementations is completely ortogonal from my point of view.
>>
>> But I really don't understand what was your point in r1707196 review
>> if you are not interested in review any changes to this function at
>> all.
>
> Ivan: I reviewed your commit because I saw it. During review I *then*
> thought "surely we must already have some code for doing this" and so
> I went looking for similar code and found the existing similar
> function. And combining them is not orthogonal to tweaking one of them
> on its own. (Combining them would be orthogonal to tweaking both of
> the implementations together, if the implementations were initially
> similar enough so that would make sense.) Tweaking one of them on its
> own doesn't give me any clue whether you have considered whether the
> other implementation might already be better than what you're doing.
>
Thanks for pointing to svn_string_from_stream(), but this function
slightly different: it has SCRATCH_POOL argument and doesn't have
LEN_HINT argument. It's little difference in semantic.
--
Ivan Zhakov
Re: svn commit: r1707196 - /subversion/trunk/subversion/libsvn_subr/stream.c
Posted by Julian Foad <ju...@gmail.com>.
>>> Julian Foad wrote:
>>>> [...] I will be
>>>> interested in reviewing the (single) implementation.
Stefan wrote:
> [...] Here the final version.
> If that doesn't work either then I'm done for today.
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(). 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()?
Ivan wrote:
> Merging svn_stringbuf_from_stream() and svn_stringbuf_from_stream()
> implementations is completely ortogonal from my point of view.
>
> But I really don't understand what was your point in r1707196 review
> if you are not interested in review any changes to this function at
> all.
Ivan: I reviewed your commit because I saw it. During review I *then*
thought "surely we must already have some code for doing this" and so
I went looking for similar code and found the existing similar
function. And combining them is not orthogonal to tweaking one of them
on its own. (Combining them would be orthogonal to tweaking both of
the implementations together, if the implementations were initially
similar enough so that would make sense.) Tweaking one of them on its
own doesn't give me any clue whether you have considered whether the
other implementation might already be better than what you're doing.
- Julian
Re: svn commit: r1707196 - /subversion/trunk/subversion/libsvn_subr/stream.c
Posted by Stefan Fuhrmann <st...@wandisco.com>.
On Wed, Oct 7, 2015 at 3:02 PM, Ivan Zhakov <iv...@visualsvn.com> wrote:
> On 7 October 2015 at 15:57, Stefan Fuhrmann
> <st...@wandisco.com> wrote:
> > On Wed, Oct 7, 2015 at 2:47 PM, Julian Foad <ju...@gmail.com>
> wrote:
> >>
> >> Stefan wrote:
> >> > I guess the correct way of doing this is revert Ivan's
> >> > change and apply something like the attached patch.
> >> Ivan wrote:
> >> > Here is the patch that I wanted commit later. What do you think?
> >>
> >> I am not interested in reviewing any more patches that tweak just one
> >> of the implementations. If you (anybody) want to do something, please
> >> combine the two functions into one implementation, and then I will be
> >> interested in reviewing the (single) implementation.
> >
> >
> > Alright, here you go.
> >
> Hi Stefan,
>
> Are you sure that code in proposed patch compiles at least? :)
>
It does, GCC is quite lenient when it comes to pointer types ;)
But it has caused a few test failures. Here the final version.
If that doesn't work either then I'm done for today.
-- Stefan^2.
Re: svn commit: r1707196 - /subversion/trunk/subversion/libsvn_subr/stream.c
Posted by Ivan Zhakov <iv...@visualsvn.com>.
On 7 October 2015 at 15:57, Stefan Fuhrmann
<st...@wandisco.com> wrote:
> On Wed, Oct 7, 2015 at 2:47 PM, Julian Foad <ju...@gmail.com> wrote:
>>
>> Stefan wrote:
>> > I guess the correct way of doing this is revert Ivan's
>> > change and apply something like the attached patch.
>> Ivan wrote:
>> > Here is the patch that I wanted commit later. What do you think?
>>
>> I am not interested in reviewing any more patches that tweak just one
>> of the implementations. If you (anybody) want to do something, please
>> combine the two functions into one implementation, and then I will be
>> interested in reviewing the (single) implementation.
>
>
> Alright, here you go.
>
Hi Stefan,
Are you sure that code in proposed patch compiles at least? :)
--
Ivan Zhakov
Re: svn commit: r1707196 - /subversion/trunk/subversion/libsvn_subr/stream.c
Posted by Stefan Fuhrmann <st...@wandisco.com>.
On Wed, Oct 7, 2015 at 2:47 PM, Julian Foad <ju...@gmail.com> wrote:
> Stefan wrote:
> > I guess the correct way of doing this is revert Ivan's
> > change and apply something like the attached patch.
> Ivan wrote:
> > Here is the patch that I wanted commit later. What do you think?
>
> I am not interested in reviewing any more patches that tweak just one
> of the implementations. If you (anybody) want to do something, please
> combine the two functions into one implementation, and then I will be
> interested in reviewing the (single) implementation.
>
Alright, here you go.
-- Stefan^2.
Re: svn commit: r1707196 - /subversion/trunk/subversion/libsvn_subr/stream.c
Posted by Julian Foad <ju...@gmail.com>.
Stefan wrote:
> I guess the correct way of doing this is revert Ivan's
> change and apply something like the attached patch.
Ivan wrote:
> Here is the patch that I wanted commit later. What do you think?
I am not interested in reviewing any more patches that tweak just one
of the implementations. If you (anybody) want to do something, please
combine the two functions into one implementation, and then I will be
interested in reviewing the (single) implementation.
- Julian
Re: svn commit: r1707196 - /subversion/trunk/subversion/libsvn_subr/stream.c
Posted by Ivan Zhakov <iv...@visualsvn.com>.
On 7 October 2015 at 15:21, Ivan Zhakov <iv...@visualsvn.com> wrote:
> On 7 October 2015 at 14:53, Julian Foad <ju...@gmail.com> wrote:
>> Ivan Zhakov wrote:
>>> Bert Huijben wrote:
>>>>> URL: http://svn.apache.org/viewvc?rev=1707196&view=rev
>>>>> Log:
>>>>> Slightly optimize svn_stringbuf_from_stream() to avoid allocating twice
>>>>> more memory and unnecessary memcpy() when LEN_HINT is equal to final
>>>>> stringbuf
>>>>> length.
>>>>>
>>>>> * subversion/libsvn_subr/stream.c
>>>>> (svn_stringbuf_from_stream): Always preallocate LEN_HINT +
>>>>> MIN_READ_SIZE
>>>>> bytes to be able perform final read without stringbuf reallocation.
>>>>
>>>> Can you explain why hint + MIN_READ_SIZE instead of something like
>>>> MAX(len_hint+1, MIN_READ_SIZE)
>>>>
>>>> I don't know what MIN_READ_SIZE is from just this patch, but it could easily be
>>>> something like 16 Kbyte, while len_hint could be something like 16 for a file like
>>>> 'format' that was just statted to obtain the actual size
>>>>
>>>> 16 Kbyte + 16 bytes for a 16 bytes file looks a bit large... And allocating 16 + 32
>>>> bytes is far more efficient than allocating that huge chunk at once.
>>>>
>>> MIN_READ_SIZE is 64 bytes and it's locally defined in this function.
>>> We cannot use MAX(len_hint+1, MIN_READ_SIZE) because main loop
>>> reallocates buffer if don't have MIN_READ_SIZE remaining. The current
>>> code assumes that MIN_READ_SIZE is small value and I decided to leave
>>> this code intact. It could be improved of course, but I wanted to make
>>> minimum change first.
>>
>> I don't think that was the right fix. One deficiency with the code is
>> that it doesn't break out of the loop when end-of-stream is detected,
>> which can be reported by a 'short' read.
> It's a different problem and I my plan was to fix in another commit. I
> don't think that problem in existing code should be reason to do not
> fix one of them.
>
Here is the patch that I wanted commit later. What do you think?
--
Ivan Zhakov
Re: svn commit: r1707196 - /subversion/trunk/subversion/libsvn_subr/stream.c
Posted by Ivan Zhakov <iv...@visualsvn.com>.
On 7 October 2015 at 14:53, Julian Foad <ju...@gmail.com> wrote:
> Ivan Zhakov wrote:
>> Bert Huijben wrote:
>>>> URL: http://svn.apache.org/viewvc?rev=1707196&view=rev
>>>> Log:
>>>> Slightly optimize svn_stringbuf_from_stream() to avoid allocating twice
>>>> more memory and unnecessary memcpy() when LEN_HINT is equal to final
>>>> stringbuf
>>>> length.
>>>>
>>>> * subversion/libsvn_subr/stream.c
>>>> (svn_stringbuf_from_stream): Always preallocate LEN_HINT +
>>>> MIN_READ_SIZE
>>>> bytes to be able perform final read without stringbuf reallocation.
>>>
>>> Can you explain why hint + MIN_READ_SIZE instead of something like
>>> MAX(len_hint+1, MIN_READ_SIZE)
>>>
>>> I don't know what MIN_READ_SIZE is from just this patch, but it could easily be
>>> something like 16 Kbyte, while len_hint could be something like 16 for a file like
>>> 'format' that was just statted to obtain the actual size
>>>
>>> 16 Kbyte + 16 bytes for a 16 bytes file looks a bit large... And allocating 16 + 32
>>> bytes is far more efficient than allocating that huge chunk at once.
>>>
>> MIN_READ_SIZE is 64 bytes and it's locally defined in this function.
>> We cannot use MAX(len_hint+1, MIN_READ_SIZE) because main loop
>> reallocates buffer if don't have MIN_READ_SIZE remaining. The current
>> code assumes that MIN_READ_SIZE is small value and I decided to leave
>> this code intact. It could be improved of course, but I wanted to make
>> minimum change first.
>
> I don't think that was the right fix. One deficiency with the code is
> that it doesn't break out of the loop when end-of-stream is detected,
> which can be reported by a 'short' read.
It's a different problem and I my plan was to fix in another commit. I
don't think that problem in existing code should be reason to do not
fix one of them.
> Your change doesn't solve
> that problem in general, although it does solve it for the case where
> the supplied length hint is exactly right.
>
> We also have the function
>
> svn_string_from_stream()
>
> which performs a virtually identical task. These two functions have
> completely different implementations. svn_string_from_stream() does
> break out of the loop as soon as it detects end-of-stream reported as
> a short read. They also use completely different read sizes
> (MIN_READ_SIZE = 64 bytes vs. SVN__STREAM_CHUNK_SIZE = 16384).
>
Did you notice that current svn_stringbuf_from_stream() implementation
increase read size twice on every loop iteration?
> That is silly: there is no good reason for that, and it is a waste of
> effort maintaining them separately, and discussing separate
> optimizations for each.
>
> We should make those two functions share the same "best" implementation.
>
I agree that it would be nice to have the same "best" implementation
for both functions. But svn_string_from_stream() function less
efficient it terms of memory operations, while reading stream using
bigger chunks at the beginning. Another difference that
svn_string_from_stream() closes source stream, while
svn_stringbuf_from_stream() don't.
Disclaimer: I didn't wrote svn_string_from_stream() nor
svn_stringbuf_from_stream() and I'm not aware why these functions have
different implementations. I've just spotted one particular problem
and fixed it.
--
Ivan Zhakov
Re: svn commit: r1707196 - /subversion/trunk/subversion/libsvn_subr/stream.c
Posted by Julian Foad <ju...@gmail.com>.
Ivan Zhakov wrote:
> Bert Huijben wrote:
>>> URL: http://svn.apache.org/viewvc?rev=1707196&view=rev
>>> Log:
>>> Slightly optimize svn_stringbuf_from_stream() to avoid allocating twice
>>> more memory and unnecessary memcpy() when LEN_HINT is equal to final
>>> stringbuf
>>> length.
>>>
>>> * subversion/libsvn_subr/stream.c
>>> (svn_stringbuf_from_stream): Always preallocate LEN_HINT +
>>> MIN_READ_SIZE
>>> bytes to be able perform final read without stringbuf reallocation.
>>
>> Can you explain why hint + MIN_READ_SIZE instead of something like
>> MAX(len_hint+1, MIN_READ_SIZE)
>>
>> I don't know what MIN_READ_SIZE is from just this patch, but it could easily be
>> something like 16 Kbyte, while len_hint could be something like 16 for a file like
>> 'format' that was just statted to obtain the actual size
>>
>> 16 Kbyte + 16 bytes for a 16 bytes file looks a bit large... And allocating 16 + 32
>> bytes is far more efficient than allocating that huge chunk at once.
>>
> MIN_READ_SIZE is 64 bytes and it's locally defined in this function.
> We cannot use MAX(len_hint+1, MIN_READ_SIZE) because main loop
> reallocates buffer if don't have MIN_READ_SIZE remaining. The current
> code assumes that MIN_READ_SIZE is small value and I decided to leave
> this code intact. It could be improved of course, but I wanted to make
> minimum change first.
I don't think that was the right fix. One deficiency with the code is
that it doesn't break out of the loop when end-of-stream is detected,
which can be reported by a 'short' read. Your change doesn't solve
that problem in general, although it does solve it for the case where
the supplied length hint is exactly right.
We also have the function
svn_string_from_stream()
which performs a virtually identical task. These two functions have
completely different implementations. svn_string_from_stream() does
break out of the loop as soon as it detects end-of-stream reported as
a short read. They also use completely different read sizes
(MIN_READ_SIZE = 64 bytes vs. SVN__STREAM_CHUNK_SIZE = 16384).
That is silly: there is no good reason for that, and it is a waste of
effort maintaining them separately, and discussing separate
optimizations for each.
We should make those two functions share the same "best" implementation.
- Julian
Re: svn commit: r1707196 - /subversion/trunk/subversion/libsvn_subr/stream.c
Posted by Ivan Zhakov <iv...@visualsvn.com>.
On 7 October 2015 at 14:08, Bert Huijben <be...@qqmail.nl> wrote:
>> -----Original Message-----
>> From: ivan@apache.org [mailto:ivan@apache.org]
>> Sent: woensdag 7 oktober 2015 09:57
>> To: commits@subversion.apache.org
>> Subject: svn commit: r1707196 -
>> /subversion/trunk/subversion/libsvn_subr/stream.c
>>
>> Author: ivan
>> Date: Wed Oct 7 07:57:05 2015
>> New Revision: 1707196
>>
>> URL: http://svn.apache.org/viewvc?rev=1707196&view=rev
>> Log:
>> Slightly optimize svn_stringbuf_from_stream() to avoid allocating twice
>> more memory and unnecessary memcpy() when LEN_HINT is equal to final
>> stringbuf
>> length.
>>
>> * subversion/libsvn_subr/stream.c
>> (svn_stringbuf_from_stream): Always preallocate LEN_HINT +
>> MIN_READ_SIZE
>> bytes to be able perform final read without stringbuf reallocation.
>
> Can you explain why hint + MIN_READ_SIZE instead of something like
> MAX(len_hint+1, MIN_READ_SIZE)
>
> I don't know what MIN_READ_SIZE is from just this patch, but it could easily be
> something like 16 Kbyte, while len_hint could be something like 16 for a file like
> 'format' that was just statted to obtain the actual size
>
> 16 Kbyte + 16 bytes for a 16 bytes file looks a bit large... And allocating 16 + 32
> bytes is far more efficient than allocating that huge chunk at once.
>
MIN_READ_SIZE is 64 bytes and it's locally defined in this function.
We cannot use MAX(len_hint+1, MIN_READ_SIZE) because main loop
reallocates buffer if don't have MIN_READ_SIZE remaining. The current
code assumes that MIN_READ_SIZE is small value and I decided to leave
this code intact. It could be improved of course, but I wanted to make
minimum change first.
--
Ivan Zhakov