On 7 October 2015 at 18:10, Julian Foad <[email protected]> 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
Index: subversion/include/svn_io.h
===================================================================
--- subversion/include/svn_io.h (revision 1707273)
+++ subversion/include/svn_io.h (working copy)
@@ -1524,6 +1524,9 @@
* @a result. The stream will be closed when it has been successfully and
* completely read.
*
+ * @a len_hint specifies the initial capacity of the string buffer and
+ * may be 0.
+ *
* The returned memory is allocated in @a result_pool, and any temporary
* allocations are performed in @a scratch_pool.
*
@@ -1530,15 +1533,27 @@
* @note due to memory pseudo-reallocation behavior (due to pools), this
* can be a memory-intensive operation for large files.
*
- * @since New in 1.6
+ * @since New in 1.10
*/
svn_error_t *
+svn_string_from_stream2(svn_string_t **result,
+ svn_stream_t *stream,
+ apr_size_t len_hint,
+ apr_pool_t *result_pool,
+ apr_pool_t *scratch_pool);
+
+/** Similar to svn_string_from_stream2(), but always passes 0 for
+ * @a len_hint.
+ *
+ * @deprecated Provided for backwards compatibility with the 1.9 API.
+ */
+SVN_DEPRECATED
+svn_error_t *
svn_string_from_stream(svn_string_t **result,
svn_stream_t *stream,
apr_pool_t *result_pool,
apr_pool_t *scratch_pool);
-
/** A function type provided for use as a callback from
* @c svn_stream_lazyopen_create().
*
Index: subversion/libsvn_fs_x/reps.c
===================================================================
--- subversion/libsvn_fs_x/reps.c (revision 1707273)
+++ subversion/libsvn_fs_x/reps.c (working copy)
@@ -417,8 +417,8 @@
apr_size_t idx;
SVN_ERR(svn_fs_x__get_contents(&stream, builder->fs, rep, FALSE,
scratch_pool));
- SVN_ERR(svn_string_from_stream(&contents, stream, scratch_pool,
- scratch_pool));
+ SVN_ERR(svn_string_from_stream2(&contents, stream, SVN__STREAM_CHUNK_SIZE,
+ scratch_pool, scratch_pool));
SVN_ERR(svn_fs_x__reps_add(&idx, builder, contents));
base.revision = svn_fs_x__get_revnum(rep->id.change_set);
Index: subversion/libsvn_subr/deprecated.c
===================================================================
--- subversion/libsvn_subr/deprecated.c (revision 1707273)
+++ subversion/libsvn_subr/deprecated.c (working copy)
@@ -1209,6 +1209,16 @@
return s;
}
+svn_error_t *
+svn_string_from_stream(svn_string_t **result,
+ svn_stream_t *stream,
+ apr_pool_t *result_pool,
+ apr_pool_t *scratch_pool)
+{
+ return svn_error_trace(svn_string_from_stream2(result, stream, 0,
+ result_pool, scratch_pool));
+}
+
/*** From path.c ***/
const char *
Index: subversion/libsvn_subr/stream.c
===================================================================
--- subversion/libsvn_subr/stream.c (revision 1707273)
+++ subversion/libsvn_subr/stream.c (working copy)
@@ -1793,32 +1793,19 @@
svn_error_t *
-svn_string_from_stream(svn_string_t **result,
- svn_stream_t *stream,
- apr_pool_t *result_pool,
- apr_pool_t *scratch_pool)
+svn_string_from_stream2(svn_string_t **result,
+ svn_stream_t *stream,
+ apr_size_t len_hint,
+ apr_pool_t *result_pool,
+ apr_pool_t *scratch_pool)
{
- svn_stringbuf_t *work = svn_stringbuf_create_ensure(SVN__STREAM_CHUNK_SIZE,
- result_pool);
- char *buffer = apr_palloc(scratch_pool, SVN__STREAM_CHUNK_SIZE);
+ svn_stringbuf_t *buf;
- while (1)
- {
- apr_size_t len = SVN__STREAM_CHUNK_SIZE;
+ SVN_ERR(svn_stringbuf_from_stream(&buf, stream, len_hint, result_pool));
+ *result = svn_stringbuf__morph_into_string(buf);
- SVN_ERR(svn_stream_read_full(stream, buffer, &len));
- svn_stringbuf_appendbytes(work, buffer, len);
-
- if (len < SVN__STREAM_CHUNK_SIZE)
- break;
- }
-
SVN_ERR(svn_stream_close(stream));
- *result = apr_palloc(result_pool, sizeof(**result));
- (*result)->data = work->data;
- (*result)->len = work->len;
-
return SVN_NO_ERROR;
}
Index: subversion/libsvn_wc/old-and-busted.c
===================================================================
--- subversion/libsvn_wc/old-and-busted.c (revision 1707273)
+++ subversion/libsvn_wc/old-and-busted.c (working copy)
@@ -1203,7 +1203,8 @@
/* Open the entries file. */
SVN_ERR(svn_wc__open_adm_stream(&stream, dir_abspath, SVN_WC__ADM_ENTRIES,
scratch_pool, scratch_pool));
- SVN_ERR(svn_string_from_stream(&buf, stream, scratch_pool, scratch_pool));
+ SVN_ERR(svn_string_from_stream2(&buf, stream, SVN__STREAM_CHUNK_SIZE,
+ scratch_pool, scratch_pool));
/* We own the returned data; it is modifiable, so cast away... */
curp = (char *)buf->data;
Index: tools/dev/x509-parser.c
===================================================================
--- tools/dev/x509-parser.c (revision 1707273)
+++ tools/dev/x509-parser.c (working copy)
@@ -94,7 +94,8 @@
apr_pool_t *pool)
{
svn_string_t *raw;
- SVN_ERR(svn_string_from_stream(&raw, in, pool, pool));
+ SVN_ERR(svn_string_from_stream2(&raw, in, SVN__STREAM_CHUNK_SIZE,
+ pool, pool));
*der_cert = NULL;
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;
