On 7 October 2015 at 15:21, Ivan Zhakov <[email protected]> wrote:
> On 7 October 2015 at 14:53, Julian Foad <[email protected]> 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
Index: subversion/libsvn_subr/stream.c
===================================================================
--- subversion/libsvn_subr/stream.c (revision 1707273)
+++ subversion/libsvn_subr/stream.c (working copy)
@@ -42,6 +42,7 @@
#include "svn_checksum.h"
#include "svn_path.h"
#include "svn_private_config.h"
+#include "svn_sorts.h"
#include "private/svn_atomic.h"
#include "private/svn_error_private.h"
#include "private/svn_eol_private.h"
@@ -1487,22 +1488,24 @@
apr_pool_t *result_pool)
{
#define MIN_READ_SIZE 64
-
- apr_size_t to_read = 0;
svn_stringbuf_t *text
- = svn_stringbuf_create_ensure(len_hint + MIN_READ_SIZE,
+ = svn_stringbuf_create_ensure(MAX(len_hint + 1, MIN_READ_SIZE),
result_pool);
- do
+ while(TRUE)
{
- to_read = text->blocksize - 1 - text->len;
- SVN_ERR(svn_stream_read_full(stream, text->data + text->len, &to_read));
- text->len += to_read;
+ apr_size_t to_read = text->blocksize - 1 - text->len;
+ apr_size_t actually_read = to_read;
- if (to_read && text->blocksize < text->len + MIN_READ_SIZE)
+ SVN_ERR(svn_stream_read_full(stream, text->data + text->len,
&actually_read));
+ text->len += actually_read;
+
+ if (actually_read < to_read)
+ break;
+
+ if (text->blocksize < text->len + MIN_READ_SIZE)
svn_stringbuf_ensure(text, text->blocksize * 2);
}
- while (to_read);
text->data[text->len] = '\0';
*str = text;
