You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@subversion.apache.org by rh...@apache.org on 2014/01/06 12:23:06 UTC
svn commit: r1555716 - /subversion/trunk/subversion/libsvn_ra_serf/update.c
Author: rhuijben
Date: Mon Jan 6 11:23:05 2014
New Revision: 1555716
URL: http://svn.apache.org/r1555716
Log:
In the ra serf (update) editor, spool the report to a spillbuffer instead of
to a tempfile to avoid creating a tempfile for small requests.
* subversion/libsvn_ra_serf/update.c
(report_context_t): Update variable type. Remove TODO.
(set_path,
delete_path,
link_path): Update caller.
(create_update_report_body): Decide whether to send a file or a buffer.
Move sendfile optimization here.
(finish_report): Update caller. Move sendfile optimization to
create_update_report_body.
(make_update_reporter): Create spillbuf instead of tempfile. Update caller.
Modified:
subversion/trunk/subversion/libsvn_ra_serf/update.c
Modified: subversion/trunk/subversion/libsvn_ra_serf/update.c
URL: http://svn.apache.org/viewvc/subversion/trunk/subversion/libsvn_ra_serf/update.c?rev=1555716&r1=1555715&r2=1555716&view=diff
==============================================================================
--- subversion/trunk/subversion/libsvn_ra_serf/update.c (original)
+++ subversion/trunk/subversion/libsvn_ra_serf/update.c Mon Jan 6 11:23:05 2014
@@ -358,12 +358,9 @@ struct report_context_t {
const svn_delta_editor_t *update_editor;
void *update_baton;
- /* The file holding request body for the REPORT.
- *
- * ### todo: It will be better for performance to store small
- * request bodies (like 4k) in memory and bigger bodies on disk.
- */
- apr_file_t *body_file;
+ /* The potentially tempfile backed spillbuf holding request body for the
+ * REPORT */
+ svn_spillbuf_t *body_sb;
/* root directory object */
report_dir_t *root_dir;
@@ -2636,8 +2633,7 @@ set_path(void *report_baton,
svn_xml_escape_cdata_cstring(&buf, path, pool);
svn_xml_make_close_tag(&buf, pool, "S:entry");
- SVN_ERR(svn_io_file_write_full(report->body_file, buf->data, buf->len,
- NULL, pool));
+ SVN_ERR(svn_spillbuf__write(report->body_sb, buf->data, buf->len, pool));
if (lock_token)
{
@@ -2659,8 +2655,7 @@ delete_path(void *report_baton,
make_simple_xml_tag(&buf, "S:missing", path, pool);
- SVN_ERR(svn_io_file_write_full(report->body_file, buf->data, buf->len,
- NULL, pool));
+ SVN_ERR(svn_spillbuf__write(report->body_sb, buf->data, buf->len, pool));
return SVN_NO_ERROR;
}
@@ -2709,8 +2704,7 @@ link_path(void *report_baton,
svn_xml_escape_cdata_cstring(&buf, path, pool);
svn_xml_make_close_tag(&buf, pool, "S:entry");
- SVN_ERR(svn_io_file_write_full(report->body_file, buf->data, buf->len,
- NULL, pool));
+ SVN_ERR(svn_spillbuf__write(report->body_sb, buf->data, buf->len, pool));
/* Store the switch roots to allow generating repos_relpaths from just
the working copy paths. (Needed for HTTPv2) */
@@ -2779,12 +2773,42 @@ create_update_report_body(serf_bucket_t
apr_pool_t *pool)
{
report_context_t *report = baton;
- apr_off_t offset;
+ apr_file_t *body_file = svn_spillbuf__get_file(report->body_sb);
- offset = 0;
- SVN_ERR(svn_io_file_seek(report->body_file, APR_SET, &offset, pool));
+ if (body_file != NULL)
+ {
+ /* The spillbuffer was spooled to disk. Use the most optimized way
+ * to send it to serf, like when we didn't spool to memory first */
+ apr_off_t offset;
+
+ /* We need to flush the file, make it unbuffered (so that it can be
+ * zero-copied via mmap), and reset the position before attempting to
+ * deliver the file.
+ *
+ * N.B. If we have APR 1.3+, we can unbuffer the file to let us use mmap
+ * and zero-copy the PUT body. However, on older APR versions, we can't
+ * check the buffer status; but serf will fall through and create a file
+ * bucket for us on the buffered svndiff handle.
+ *
+ * ### Is this really a useful optimization for an update report?
+ */
+ SVN_ERR(svn_io_file_flush(body_file, pool));
+#if APR_VERSION_AT_LEAST(1, 3, 0)
+ apr_file_buffer_set(body_file, NULL, 0);
+#endif
- *body_bkt = serf_bucket_file_create(report->body_file, alloc);
+ offset = 0;
+ SVN_ERR(svn_io_file_seek(body_file, APR_SET, &offset, pool));
+
+ *body_bkt = serf_bucket_file_create(body_file, alloc);
+ }
+ else
+ {
+ /* Everything is already in memory. Just wrap as bucket.
+ * Note that this would just work for the file case if needed */
+ *body_bkt = svn_ra_serf__create_sb_bucket(report->body_sb, alloc,
+ report->pool, pool);
+ }
return SVN_NO_ERROR;
}
@@ -2829,22 +2853,7 @@ finish_report(void *report_baton,
apr_interval_time_t waittime_left = sess->timeout;
svn_xml_make_close_tag(&buf, iterpool, "S:update-report");
- SVN_ERR(svn_io_file_write_full(report->body_file, buf->data, buf->len,
- NULL, iterpool));
-
- /* We need to flush the file, make it unbuffered (so that it can be
- * zero-copied via mmap), and reset the position before attempting to
- * deliver the file.
- *
- * N.B. If we have APR 1.3+, we can unbuffer the file to let us use mmap
- * and zero-copy the PUT body. However, on older APR versions, we can't
- * check the buffer status; but serf will fall through and create a file
- * bucket for us on the buffered svndiff handle.
- */
- SVN_ERR(svn_io_file_flush(report->body_file, iterpool));
-#if APR_VERSION_AT_LEAST(1, 3, 0)
- apr_file_buffer_set(report->body_file, NULL, 0);
-#endif
+ SVN_ERR(svn_spillbuf__write(report->body_sb, buf->data, buf->len, iterpool));
SVN_ERR(svn_ra_serf__report_resource(&report_target, sess, NULL, pool));
@@ -3234,9 +3243,7 @@ make_update_reporter(svn_ra_session_t *r
*reporter = &ra_serf_reporter;
*report_baton = report;
- SVN_ERR(svn_io_open_unique_file3(&report->body_file, NULL, NULL,
- svn_io_file_del_on_pool_cleanup,
- report->pool, scratch_pool));
+ report->body_sb = svn_spillbuf__create(1024, 131072, report->pool);
if (sess->bulk_updates == svn_tristate_true)
{
@@ -3369,8 +3376,8 @@ make_update_reporter(svn_ra_session_t *r
make_simple_xml_tag(&buf, "S:depth", svn_depth_to_word(depth), scratch_pool);
- SVN_ERR(svn_io_file_write_full(report->body_file, buf->data, buf->len,
- NULL, scratch_pool));
+ SVN_ERR(svn_spillbuf__write(report->body_sb, buf->data, buf->len,
+ scratch_pool));
return SVN_NO_ERROR;
}
Re: svn commit: r1555716 - /subversion/trunk/subversion/libsvn_ra_serf/update.c
Posted by Ivan Zhakov <iv...@visualsvn.com>.
On 7 January 2014 12:42, Bert Huijben <be...@qqmail.nl> wrote:
> The current code works in both scenarios (tested via buffersizes of just a
> few bytes, and a debugger verifyimg the corner cases, etc). But I doubt it
> is worth all the trouble. The file is null until something is spilled and
> from that point onwards everything is in the file (until the spill is
> empty).
>
Few bytes is bad test because memory isn't used at all in this case.
I've tested and code doesn't work as expected. I've tested following
patch to update subversion 1.8.x working copy:
[[[
Index: subversion/libsvn_ra_serf/update.c
===================================================================
--- subversion/libsvn_ra_serf/update.c (revision 1556193)
+++ subversion/libsvn_ra_serf/update.c (working copy)
@@ -3429,7 +3429,7 @@
*reporter = &ra_serf_reporter;
*report_baton = report;
- report->body_sb = svn_spillbuf__create(1024, 131072, report->pool);
+ report->body_sb = svn_spillbuf__create(220, 220, report->pool);
if (sess->bulk_updates == svn_tristate_true)
{
]]]
It fails with:
[[[
..\..\..\subversion\svn\update-cmd.c:172,
..\..\..\subversion\libsvn_client\update.c:722,
..\..\..\subversion\libsvn_client\update.c:614,
..\..\..\subversion\libsvn_client\update.c:466,
..\..\..\subversion\libsvn_wc\adm_crawler.c:844,
..\..\..\subversion\libsvn_ra_serf\update.c:3131,
..\..\..\subversion\libsvn_ra_serf\util.c:915,
..\..\..\subversion\libsvn_ra_serf\util.c:2270,
..\..\..\subversion\libsvn_ra_serf\util.c:2043,
..\..\..\subversion\libsvn_ra_serf\util.c:2030:
(apr_err=SVN_ERR_RA_DAV_REQUEST_FAILED)
svn: E175002: Unexpected HTTP status 400 'Bad Request' on '/repos/asf/!svn/me'
]]]
> If you want to look at improving it now, feel free. I'm first working on
> switching the update logic to the new xml parser. (And will perform a
> further cleanup including these points round after that).
>
I suggest to remove this optimized code path completely: potential
optimization is not significant imho. Also such big update request
bodies are not usual. I can remove this code if you're busy with other
stuff.
--
Ivan Zhakov
CTO | VisualSVN | http://www.visualsvn.com
Re: svn commit: r1555716 - /subversion/trunk/subversion/libsvn_ra_serf/update.c
Posted by Bert Huijben <be...@qqmail.nl>.
The current code works in both scenarios (tested via buffersizes of just a few bytes, and a debugger verifyimg the corner cases, etc). But I doubt it is worth all the trouble. The file is null until something is spilled and from that point onwards everything is in the file (until the spill is empty).
If you want to look at improving it now, feel free. I'm first working on switching the update logic to the new xml parser. (And will perform a further cleanup including these points round after that).
Thanks for your reviews!
Bert
Sent from Windows Mail
From: Ivan Zhakov
Sent: Monday, January 6, 2014 11:26 PM
To: Subversion Development, Bert Huijben
On 6 January 2014 15:23, <rh...@apache.org> wrote:
> Author: rhuijben
> Date: Mon Jan 6 11:23:05 2014
> New Revision: 1555716
>
> URL: http://svn.apache.org/r1555716
> Log:
> In the ra serf (update) editor, spool the report to a spillbuffer instead of
> to a tempfile to avoid creating a tempfile for small requests.
>
> + if (body_file != NULL)
> + {
> + /* The spillbuffer was spooled to disk. Use the most optimized way
> + * to send it to serf, like when we didn't spool to memory first */
> + apr_off_t offset;
> +
> + /* We need to flush the file, make it unbuffered (so that it can be
> + * zero-copied via mmap), and reset the position before attempting to
> + * deliver the file.
> + *
> + * N.B. If we have APR 1.3+, we can unbuffer the file to let us use mmap
> + * and zero-copy the PUT body. However, on older APR versions, we can't
> + * check the buffer status; but serf will fall through and create a file
> + * bucket for us on the buffered svndiff handle.
> + *
> + * ### Is this really a useful optimization for an update report?
> + */
> + SVN_ERR(svn_io_file_flush(body_file, pool));
> +#if APR_VERSION_AT_LEAST(1, 3, 0)
> + apr_file_buffer_set(body_file, NULL, 0);
> +#endif
>
> - *body_bkt = serf_bucket_file_create(report->body_file, alloc);
> + offset = 0;
> + SVN_ERR(svn_io_file_seek(body_file, APR_SET, &offset, pool));
> +
> + *body_bkt = serf_bucket_file_create(body_file, alloc);
> + }
> + else
> + {
> + /* Everything is already in memory. Just wrap as bucket.
> + * Note that this would just work for the file case if needed */
> + *body_bkt = svn_ra_serf__create_sb_bucket(report->body_sb, alloc,
> + report->pool, pool);
> + }
Hi Bert.
Probably this logic should be moved to svn_ra_serf__create_sb_bucket()?
Also I think that current implementation shouldn't work because
spillbuf file doesn't include in-memory content by default, because
SPILL_ALL_CONTENTS is FALSE. Did you test this scenario?
The proper implementation should create aggregate bucket with memory
and file buckets. But I'm not sure that all this optimization worth.
--
Ivan Zhakov
CTO | VisualSVN | http://www.visualsvn.com
Re: svn commit: r1555716 - /subversion/trunk/subversion/libsvn_ra_serf/update.c
Posted by Ivan Zhakov <iv...@visualsvn.com>.
On 6 January 2014 15:23, <rh...@apache.org> wrote:
> Author: rhuijben
> Date: Mon Jan 6 11:23:05 2014
> New Revision: 1555716
>
> URL: http://svn.apache.org/r1555716
> Log:
> In the ra serf (update) editor, spool the report to a spillbuffer instead of
> to a tempfile to avoid creating a tempfile for small requests.
>
> + if (body_file != NULL)
> + {
> + /* The spillbuffer was spooled to disk. Use the most optimized way
> + * to send it to serf, like when we didn't spool to memory first */
> + apr_off_t offset;
> +
> + /* We need to flush the file, make it unbuffered (so that it can be
> + * zero-copied via mmap), and reset the position before attempting to
> + * deliver the file.
> + *
> + * N.B. If we have APR 1.3+, we can unbuffer the file to let us use mmap
> + * and zero-copy the PUT body. However, on older APR versions, we can't
> + * check the buffer status; but serf will fall through and create a file
> + * bucket for us on the buffered svndiff handle.
> + *
> + * ### Is this really a useful optimization for an update report?
> + */
> + SVN_ERR(svn_io_file_flush(body_file, pool));
> +#if APR_VERSION_AT_LEAST(1, 3, 0)
> + apr_file_buffer_set(body_file, NULL, 0);
> +#endif
>
> - *body_bkt = serf_bucket_file_create(report->body_file, alloc);
> + offset = 0;
> + SVN_ERR(svn_io_file_seek(body_file, APR_SET, &offset, pool));
> +
> + *body_bkt = serf_bucket_file_create(body_file, alloc);
> + }
> + else
> + {
> + /* Everything is already in memory. Just wrap as bucket.
> + * Note that this would just work for the file case if needed */
> + *body_bkt = svn_ra_serf__create_sb_bucket(report->body_sb, alloc,
> + report->pool, pool);
> + }
Hi Bert.
Probably this logic should be moved to svn_ra_serf__create_sb_bucket()?
Also I think that current implementation shouldn't work because
spillbuf file doesn't include in-memory content by default, because
SPILL_ALL_CONTENTS is FALSE. Did you test this scenario?
The proper implementation should create aggregate bucket with memory
and file buckets. But I'm not sure that all this optimization worth.
--
Ivan Zhakov
CTO | VisualSVN | http://www.visualsvn.com