You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@subversion.apache.org by st...@apache.org on 2012/09/20 22:34:52 UTC
svn commit: r1388202 - in /subversion/branches/10Gb/subversion:
include/svn_ra_svn.h libsvn_ra_svn/client.c libsvn_ra_svn/marshal.c
libsvn_ra_svn/ra_svn.h svnserve/main.c svnserve/serve.c svnserve/server.h
Author: stefan2
Date: Thu Sep 20 20:34:52 2012
New Revision: 1388202
URL: http://svn.apache.org/viewvc?rev=1388202&view=rev
Log:
On 10Gb branch: Add --zero-copy-limit parameter to svnserve and pass
it down to the reporter.
* subversion/include/svn_ra_svn.h
(svn_ra_svn_create_conn3): rev API function
(svn_ra_svn_create_conn2): deprecate the old one
(svn_ra_svn_zero_copy_limit): new connection parameter access
* subversion/libsvn_ra_svn/marshal.c
(svn_ra_svn_create_conn3): implement rev'ed API
(svn_ra_svn_create_conn2,
svn_ra_svn_create_conn): implement deprecated in terms of rev'ed API
(svn_ra_svn_zero_copy_limit): implement
* subversion/libsvn_ra_svn/client.c
(handle_child_process_error,
make_tunnel,
open_session): use rev'ed API with zero-copy disabled
* subversion/libsvn_ra_svn/ra_svn.h
(svn_ra_svn_conn_st): add new parameter
* subversion/svnserve/server.h
(serve_params_t): add new parameter
* subversion/svnserve/main.c
(SVNSERVE_OPT_ZERO_COPY_LIMIT): new CL option
(svnserve__options): add entry for it
(main): parse it, pass to rev'ed API
* subversion/svnserve/serve.c
(accept_report): pass new option to new report API
Modified:
subversion/branches/10Gb/subversion/include/svn_ra_svn.h
subversion/branches/10Gb/subversion/libsvn_ra_svn/client.c
subversion/branches/10Gb/subversion/libsvn_ra_svn/marshal.c
subversion/branches/10Gb/subversion/libsvn_ra_svn/ra_svn.h
subversion/branches/10Gb/subversion/svnserve/main.c
subversion/branches/10Gb/subversion/svnserve/serve.c
subversion/branches/10Gb/subversion/svnserve/server.h
Modified: subversion/branches/10Gb/subversion/include/svn_ra_svn.h
URL: http://svn.apache.org/viewvc/subversion/branches/10Gb/subversion/include/svn_ra_svn.h?rev=1388202&r1=1388201&r2=1388202&view=diff
==============================================================================
--- subversion/branches/10Gb/subversion/include/svn_ra_svn.h (original)
+++ subversion/branches/10Gb/subversion/include/svn_ra_svn.h Thu Sep 20 20:34:52 2012
@@ -173,7 +173,21 @@ svn_ra_svn__set_shim_callbacks(svn_ra_sv
* Specify the desired network data compression level (zlib) from
* 0 (no compression) to 9 (best but slowest).
*
+ * @since New in 1.9.
+ */
+svn_ra_svn_conn_t *svn_ra_svn_create_conn3(apr_socket_t *sock,
+ apr_file_t *in_file,
+ apr_file_t *out_file,
+ int compression_level,
+ apr_size_t zero_copy_limit,
+ apr_pool_t *pool);
+
+/** Similar to svn_ra_svn_create_conn3() but disables the zero copy code
+ * path.
+ *
* @since New in 1.7.
+ *
+ * @deprecated Provided for backward compatibility with the 1.8 API.
*/
svn_ra_svn_conn_t *
svn_ra_svn_create_conn2(apr_socket_t *sock,
@@ -182,7 +196,7 @@ svn_ra_svn_create_conn2(apr_socket_t *so
int compression_level,
apr_pool_t *pool);
-/** Similar to svn_ra_svn_create_conn2() but uses default
+/** Similar to svn_ra_svn_create_conn2() but uses the default
* compression level (#SVN_DELTA_COMPRESSION_LEVEL_DEFAULT) for network
* transmissions.
*
@@ -219,6 +233,13 @@ svn_ra_svn_has_capability(svn_ra_svn_con
int
svn_ra_svn_compression_level(svn_ra_svn_conn_t *conn);
+/** Return the zero-copy data block limit to use for network transmissions
+ *
+ * @since New in 1.9.
+ */
+apr_size_t
+svn_ra_svn_zero_copy_limit(svn_ra_svn_conn_t *conn);
+
/** Returns the remote address of the connection as a string, if known,
* or NULL if inapplicable. */
const char *
Modified: subversion/branches/10Gb/subversion/libsvn_ra_svn/client.c
URL: http://svn.apache.org/viewvc/subversion/branches/10Gb/subversion/libsvn_ra_svn/client.c?rev=1388202&r1=1388201&r2=1388202&view=diff
==============================================================================
--- subversion/branches/10Gb/subversion/libsvn_ra_svn/client.c (original)
+++ subversion/branches/10Gb/subversion/libsvn_ra_svn/client.c Thu Sep 20 20:34:52 2012
@@ -454,8 +454,9 @@ static void handle_child_process_error(a
|| apr_file_open_stdout(&out_file, pool))
return;
- conn = svn_ra_svn_create_conn2(NULL, in_file, out_file,
- SVN_DELTA_COMPRESSION_LEVEL_DEFAULT, pool);
+ conn = svn_ra_svn_create_conn3(NULL, in_file, out_file,
+ SVN_DELTA_COMPRESSION_LEVEL_DEFAULT, 0,
+ pool);
err = svn_error_wrap_apr(status, _("Error in child process: %s"), desc);
svn_error_clear(svn_ra_svn_write_cmd_failure(conn, pool, err));
svn_error_clear(err);
@@ -522,8 +523,9 @@ static svn_error_t *make_tunnel(const ch
apr_file_inherit_unset(proc->out);
/* Guard against dotfile output to stdout on the server. */
- *conn = svn_ra_svn_create_conn2(NULL, proc->out, proc->in,
- SVN_DELTA_COMPRESSION_LEVEL_DEFAULT, pool);
+ *conn = svn_ra_svn_create_conn3(NULL, proc->out, proc->in,
+ SVN_DELTA_COMPRESSION_LEVEL_DEFAULT,
+ 0, pool);
err = svn_ra_svn_skip_leading_garbage(*conn, pool);
if (err)
return svn_error_quick_wrap(
@@ -592,9 +594,9 @@ static svn_error_t *open_session(svn_ra_
else
{
SVN_ERR(make_connection(uri->hostname, uri->port, &sock, pool));
- conn = svn_ra_svn_create_conn2(sock, NULL, NULL,
+ conn = svn_ra_svn_create_conn3(sock, NULL, NULL,
SVN_DELTA_COMPRESSION_LEVEL_DEFAULT,
- pool);
+ 0, pool);
}
/* Make sure we set conn->session before reading from it,
Modified: subversion/branches/10Gb/subversion/libsvn_ra_svn/marshal.c
URL: http://svn.apache.org/viewvc/subversion/branches/10Gb/subversion/libsvn_ra_svn/marshal.c?rev=1388202&r1=1388201&r2=1388202&view=diff
==============================================================================
--- subversion/branches/10Gb/subversion/libsvn_ra_svn/marshal.c (original)
+++ subversion/branches/10Gb/subversion/libsvn_ra_svn/marshal.c Thu Sep 20 20:34:52 2012
@@ -56,10 +56,11 @@
/* --- CONNECTION INITIALIZATION --- */
-svn_ra_svn_conn_t *svn_ra_svn_create_conn2(apr_socket_t *sock,
+svn_ra_svn_conn_t *svn_ra_svn_create_conn3(apr_socket_t *sock,
apr_file_t *in_file,
apr_file_t *out_file,
int compression_level,
+ apr_size_t zero_copy_limit,
apr_pool_t *pool)
{
svn_ra_svn_conn_t *conn = apr_palloc(pool, sizeof(*conn));
@@ -77,6 +78,7 @@ svn_ra_svn_conn_t *svn_ra_svn_create_con
conn->block_baton = NULL;
conn->capabilities = apr_hash_make(pool);
conn->compression_level = compression_level;
+ conn->zero_copy_limit = zero_copy_limit;
conn->pool = pool;
if (sock != NULL)
@@ -96,14 +98,25 @@ svn_ra_svn_conn_t *svn_ra_svn_create_con
return conn;
}
+svn_ra_svn_conn_t *svn_ra_svn_create_conn2(apr_socket_t *sock,
+ apr_file_t *in_file,
+ apr_file_t *out_file,
+ int compression_level,
+ apr_pool_t *pool)
+{
+ return svn_ra_svn_create_conn3(sock, in_file, out_file,
+ compression_level, 0, pool);
+}
+
/* backward-compatible implementation using the default compression level */
svn_ra_svn_conn_t *svn_ra_svn_create_conn(apr_socket_t *sock,
apr_file_t *in_file,
apr_file_t *out_file,
apr_pool_t *pool)
{
- return svn_ra_svn_create_conn2(sock, in_file, out_file,
- SVN_DELTA_COMPRESSION_LEVEL_DEFAULT, pool);
+ return svn_ra_svn_create_conn3(sock, in_file, out_file,
+ SVN_DELTA_COMPRESSION_LEVEL_DEFAULT, 0,
+ pool);
}
svn_error_t *svn_ra_svn_set_capabilities(svn_ra_svn_conn_t *conn,
@@ -146,6 +159,12 @@ svn_ra_svn_compression_level(svn_ra_svn_
return conn->compression_level;
}
+apr_size_t
+svn_ra_svn_zero_copy_limit(svn_ra_svn_conn_t *conn)
+{
+ return conn->zero_copy_limit;
+}
+
const char *svn_ra_svn_conn_remote_host(svn_ra_svn_conn_t *conn)
{
return conn->remote_ip;
Modified: subversion/branches/10Gb/subversion/libsvn_ra_svn/ra_svn.h
URL: http://svn.apache.org/viewvc/subversion/branches/10Gb/subversion/libsvn_ra_svn/ra_svn.h?rev=1388202&r1=1388201&r2=1388202&view=diff
==============================================================================
--- subversion/branches/10Gb/subversion/libsvn_ra_svn/ra_svn.h (original)
+++ subversion/branches/10Gb/subversion/libsvn_ra_svn/ra_svn.h Thu Sep 20 20:34:52 2012
@@ -86,6 +86,7 @@ struct svn_ra_svn_conn_st {
void *block_baton;
apr_hash_t *capabilities;
int compression_level;
+ apr_size_t zero_copy_limit;
char *remote_ip;
svn_delta_shim_callbacks_t *shim_callbacks;
apr_pool_t *pool;
Modified: subversion/branches/10Gb/subversion/svnserve/main.c
URL: http://svn.apache.org/viewvc/subversion/branches/10Gb/subversion/svnserve/main.c?rev=1388202&r1=1388201&r2=1388202&view=diff
==============================================================================
--- subversion/branches/10Gb/subversion/svnserve/main.c (original)
+++ subversion/branches/10Gb/subversion/svnserve/main.c Thu Sep 20 20:34:52 2012
@@ -149,6 +149,7 @@ void winservice_notify_stop(void)
#define SVNSERVE_OPT_CACHE_FULLTEXTS 266
#define SVNSERVE_OPT_CACHE_REVPROPS 267
#define SVNSERVE_OPT_SINGLE_CONN 268
+#define SVNSERVE_OPT_ZERO_COPY_LIMIT 269
static const apr_getopt_option_t svnserve__options[] =
{
@@ -235,6 +236,16 @@ static const apr_getopt_option_t svnserv
"Default is no.\n"
" "
"[used for FSFS repositories only]")},
+ {"zero-copy-limit", SVNSERVE_OPT_ZERO_COPY_LIMIT, 1,
+ N_("send files smaller than this directly from the\n"
+ " "
+ "caches to the network stack.\n"
+ " "
+ "Consult the documentation before activating this.\n"
+ " "
+ "Default is 0 (optimization disabled).\n"
+ " "
+ "[used for FSFS repositories only]")},
#ifdef CONNECTION_HAVE_THREAD_OPTION
/* ### Making the assumption here that WIN32 never has fork and so
* ### this option never exists when --service exists. */
@@ -497,6 +508,7 @@ int main(int argc, const char *argv[])
params.cache_fulltexts = TRUE;
params.cache_txdeltas = FALSE;
params.cache_revprops = FALSE;
+ params.zero_copy_limit = 0;
while (1)
{
@@ -645,6 +657,10 @@ int main(int argc, const char *argv[])
= svn_tristate__from_word(arg) == svn_tristate_true;
break;
+ case SVNSERVE_OPT_ZERO_COPY_LIMIT:
+ params.zero_copy_limit = (apr_size_t)apr_strtoi64(arg, NULL, 0);
+ break;
+
#ifdef WIN32
case SVNSERVE_OPT_SERVICE:
if (run_mode != run_mode_service)
@@ -755,8 +771,9 @@ int main(int argc, const char *argv[])
* the pool cleanup handlers that call sasl_dispose() (connection_pool)
* and sasl_done() (pool) are run in the right order. See issue #3664. */
connection_pool = svn_pool_create(pool);
- conn = svn_ra_svn_create_conn2(NULL, in_file, out_file,
+ conn = svn_ra_svn_create_conn3(NULL, in_file, out_file,
params.compression_level,
+ params.zero_copy_limit,
connection_pool);
svn_error_clear(serve(conn, ¶ms, connection_pool));
exit(0);
@@ -988,8 +1005,9 @@ int main(int argc, const char *argv[])
/* It's not a fatal error if we cannot enable keep-alives. */
}
- conn = svn_ra_svn_create_conn2(usock, NULL, NULL,
+ conn = svn_ra_svn_create_conn3(usock, NULL, NULL,
params.compression_level,
+ params.zero_copy_limit,
connection_pool);
if (run_mode == run_mode_listen_once)
Modified: subversion/branches/10Gb/subversion/svnserve/serve.c
URL: http://svn.apache.org/viewvc/subversion/branches/10Gb/subversion/svnserve/serve.c?rev=1388202&r1=1388201&r2=1388202&view=diff
==============================================================================
--- subversion/branches/10Gb/subversion/svnserve/serve.c (original)
+++ subversion/branches/10Gb/subversion/svnserve/serve.c Thu Sep 20 20:34:52 2012
@@ -886,13 +886,14 @@ static svn_error_t *accept_report(svn_bo
/* Make an svn_repos report baton. Tell it to drive the network editor
* when the report is complete. */
svn_ra_svn_get_editor(&editor, &edit_baton, conn, pool, NULL, NULL);
- SVN_CMD_ERR(svn_repos_begin_report2(&report_baton, rev, b->repos,
+ SVN_CMD_ERR(svn_repos_begin_report3(&report_baton, rev, b->repos,
b->fs_path->data, target, tgt_path,
text_deltas, depth, ignore_ancestry,
send_copyfrom_args,
editor, edit_baton,
authz_check_access_cb_func(b),
- b, pool));
+ b, svn_ra_svn_zero_copy_limit(conn),
+ pool));
rb.sb = b;
rb.repos_url = svn_path_uri_decode(b->repos_url, pool);
Modified: subversion/branches/10Gb/subversion/svnserve/server.h
URL: http://svn.apache.org/viewvc/subversion/branches/10Gb/subversion/svnserve/server.h?rev=1388202&r1=1388201&r2=1388202&view=diff
==============================================================================
--- subversion/branches/10Gb/subversion/svnserve/server.h (original)
+++ subversion/branches/10Gb/subversion/svnserve/server.h Thu Sep 20 20:34:52 2012
@@ -128,6 +128,9 @@ typedef struct serve_params_t {
Defaults to SVN_DELTA_COMPRESSION_LEVEL_DEFAULT. */
int compression_level;
+ /* Item size up to which we use the zero-copy code path to transmit
+ them over the network. 0 disables that code path. */
+ apr_size_t zero_copy_limit;
} serve_params_t;
/* Serve the connection CONN according to the parameters PARAMS. */
Re: svn commit: r1388202 - in /subversion/branches/10Gb/subversion:
include/svn_ra_svn.h libsvn_ra_svn/client.c libsvn_ra_svn/marshal.c
libsvn_ra_svn/ra_svn.h svnserve/main.c svnserve/serve.c svnserve/server.h
Posted by Stefan Fuhrmann <st...@wandisco.com>.
On Thu, Sep 20, 2012 at 11:37 PM, Stefan Sperling <st...@elego.de> wrote:
> On Thu, Sep 20, 2012 at 08:34:52PM -0000, stefan2@apache.org wrote:
> > Author: stefan2
> > Date: Thu Sep 20 20:34:52 2012
> > New Revision: 1388202
> >
> > URL: http://svn.apache.org/viewvc?rev=1388202&view=rev
> > Log:
> > On 10Gb branch: Add --zero-copy-limit parameter to svnserve and pass
> > it down to the reporter.
>
> Should we really expose a switch like that at the UI?
>
It is the server UI. We might as well use some settings file.
Both new settings are similar to TCP/IP parameter tuning.
Most of the times, you won't need it but it can allow you to
handle specific workloadsmore efficiently.
> The description you give is:
>
> > @@ -235,6 +236,16 @@ static const apr_getopt_option_t svnserv
> > "Default is no.\n"
> > " "
> > "[used for FSFS repositories only]")},
> > + {"zero-copy-limit", SVNSERVE_OPT_ZERO_COPY_LIMIT, 1,
> > + N_("send files smaller than this directly from the\n"
> > + " "
> > + "caches to the network stack.\n"
> > + " "
> > + "Consult the documentation before activating this.\n"
> > + " "
> > + "Default is 0 (optimization disabled).\n"
> > + " "
> > + "[used for FSFS repositories only]")},
>
> Which to me looks like a scary flag I'd rather leave alone. :)
>
Yes and rightly so. It has side-effects and may hurt
performance when multiple clients are being served
concurrently and not all data is available in cache.
I plan to change the cache implementation such that
readers will no longer block write attempts. The side-
effects will then be much less problematic.
> Can't we use some heuristic to determine whether or not to enable
> this optimisation, so the user won't have to reason about whether
> or not to use this option? Is designing the heuristic just a matter
> of finding a good threshold by experimentation on sample data sets?
>
The technical background is that data buffers in the
cache will be directly handed over to the network stack
(if longer than 8k - otherwise is may go into the ra_svn
TX buffer). I.e. we cannot modify the respective cache
segment until the socked call returned.
So, even with the above change to the membuffer code,
the situation remains as follows:
* the setting is only relevant when fulltext caching has
been enabled
* the reporter will always push data out until the client
cannot keep up and the socket blocks
* short file contents (<<8k) are rare and will be collected
in our TX buffer. If they should be frequent in your use-
case, you may see some speed up. However, once the
TX buffer is full, pushing that to the socket will still block
cache writes.
* enable it only if you can assume that almost all data
comes from cache (after some warm-up)
* don't use it with long-latency connections
* enable it if concurrent server access isn't the norm.
It will be most efficient with 10Gb connected clients
fetching a few GB (or less) at a time. The request will
likely be completed before the next one comes in.
* enable it if your TCP/IP stack is in parts handled by
I/O hardware. You have a 10/40Gb NIC then and this
setting is your only chance to use this bandwidth.
Some of the above can be checked by observation (typical
server load etc.). Beyond that, you need to experiment.
A good threshold might be some 70 .. 80% percentile
(i.e. 70% of your files are below the threshold):
* performance gain on 40..50% of your data
* sockets block on contents that does *not* use the
zero-copy-code
* blocks on the shorter files become less likely and
the cache becomes writable approx. 50% of the time.
-- Stefan^2.
--
*
Join us this October at Subversion Live
2012<http://www.wandisco.com/svn-live-2012>
for two days of best practice SVN training, networking, live demos,
committer meet and greet, and more! Space is limited, so get signed up
today<http://www.wandisco.com/svn-live-2012>
!
*
Re: svn commit: r1388202 - in /subversion/branches/10Gb/subversion:
include/svn_ra_svn.h libsvn_ra_svn/client.c libsvn_ra_svn/marshal.c
libsvn_ra_svn/ra_svn.h svnserve/main.c svnserve/serve.c svnserve/server.h
Posted by Stefan Sperling <st...@elego.de>.
On Thu, Sep 20, 2012 at 08:34:52PM -0000, stefan2@apache.org wrote:
> Author: stefan2
> Date: Thu Sep 20 20:34:52 2012
> New Revision: 1388202
>
> URL: http://svn.apache.org/viewvc?rev=1388202&view=rev
> Log:
> On 10Gb branch: Add --zero-copy-limit parameter to svnserve and pass
> it down to the reporter.
Should we really expose a switch like that at the UI?
The description you give is:
> @@ -235,6 +236,16 @@ static const apr_getopt_option_t svnserv
> "Default is no.\n"
> " "
> "[used for FSFS repositories only]")},
> + {"zero-copy-limit", SVNSERVE_OPT_ZERO_COPY_LIMIT, 1,
> + N_("send files smaller than this directly from the\n"
> + " "
> + "caches to the network stack.\n"
> + " "
> + "Consult the documentation before activating this.\n"
> + " "
> + "Default is 0 (optimization disabled).\n"
> + " "
> + "[used for FSFS repositories only]")},
Which to me looks like a scary flag I'd rather leave alone. :)
Can't we use some heuristic to determine whether or not to enable
this optimisation, so the user won't have to reason about whether
or not to use this option? Is designing the heuristic just a matter
of finding a good threshold by experimentation on sample data sets?