You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@subversion.apache.org by Lieven Govaerts <sv...@mobsol.be> on 2008/10/05 12:51:39 UTC
[PATCH] Drastically reduce the number of opened ra_sessions during
merge.
While profiling the merge code I noticed that svn opens a huge number of
connections with a serious performance impact. Attached patch will
drstically reduce the number of sessions and connections opened during
merge.
Example of a simple merge of a few revs from trunk to 1.5.x@33458 with
svn 1.5.x@33458 over ra_neon:
$ time subversion/svn/svn merge -c --non-interactive
33201,33212,33369,33447,32968,32975,33447 $SVN/trunk ../1.5.x
--- Merging r33201 into '../1.5.x':
C ../1.5.x/subversion/tests/cmdline/copy_tests.py
U ../1.5.x/subversion/libsvn_wc/copy.c
--- Merging r33212 into '../1.5.x':
G ../1.5.x/subversion/tests/cmdline/copy_tests.py
Skipped missing target: '../1.5.x/subversion/libsvn_subr/dirent_uri.c'
--- Merging r32968 into '../1.5.x':
U ../1.5.x/subversion/tests/cmdline/merge_tests.py
--- Merging r32975 into '../1.5.x':
G ../1.5.x/subversion/tests/cmdline/merge_tests.py
C ../1.5.x/subversion/libsvn_client/merge.c
real 1m14.014s
user 0m2.980s
sys 0m1.664s
Nr. of sessions: 43
Nr. of connections: 61
Nr. of requests: 358
Svn 1.5 With attached patch:
$ time subversion/svn/svn merge -c --non-interactive
33201,33212,33369,33447,32968,32975,33447 $SVN/trunk ../1.5.x
real 1m5.987s
user 0m2.536s
sys 0m0.532s
Nr. of sessions: 3
Nr. of connections: 21
Nr. of requests: 296
The performance win is 12% over http, it'll be much higher for https
and/or NTLM/Kerberos authz.
Attached patch makes the merge code reuse ra_sessions - if possible - in
two places. Note: the ensure_ra_session_url function is only needed if
in do_merge the merge sources can be from different repositories, which
I doubt can be the case.
I'd like to get this patch reviewed by someone familiar with the merge
code before I commit.
Lieven
[[[
Reuse opened ra_sessions where possible during merge.
* subversion/libsvn_client/merge.c
(ensure_ra_session_url): New function.
(do_merge): When merging from multiple merge sources, reuse the open
sessions when the sources are from the same repository.
(populate_remaining_ranges): Reuse the ra_session passed into this
function.
]]]
Re: [PATCH] Drastically reduce the number of opened ra_sessions during merge.
Posted by Ivan Zhakov <iv...@visualsvn.com>.
2008/10/5 Lieven Govaerts <sv...@mobsol.be>:
>
> While profiling the merge code I noticed that svn opens a huge number of
> connections with a serious performance impact. Attached patch will
> drstically reduce the number of sessions and connections opened during
> merge.
>
Looks pretty good for me. I'm +1 for commit.
--
Ivan Zhakov
VisualSVN Team
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: [PATCH] Drastically reduce the number of opened ra_sessions
during merge.
Posted by Daniel Shahaf <d....@daniel.shahaf.name>.
Lieven Govaerts wrote on Sun, 5 Oct 2008 at 14:51 +0200:
> +/** Ensure the caller receives a RA_SESSION object to URL. This function will
> + * reuse RA_SESSION if it is not NULL and is opened to the same repository as
> + * URL is pointing to. Otherwise a new session object will be created.
> + */
> +static svn_error_t *
> +ensure_ra_session_url(svn_ra_session_t **ra_session,
> + const char *url,
> + svn_client_ctx_t *ctx,
> + apr_pool_t *pool)
> +{
> + svn_error_t *err = SVN_NO_ERROR;
>
> + if (*ra_session)
> + {
> + const char *old_session_url;
> + err = svn_client__ensure_ra_session_url(&old_session_url,
> + *ra_session,
> + url,
> + pool);
> + }
> +
> + if ((err && err->apr_err == SVN_ERR_RA_ILLEGAL_URL) || ! *ra_session)
> + {
> + err = svn_client__open_ra_session_internal(ra_session, url,
> + NULL, NULL, NULL,
> + FALSE, TRUE, ctx, pool);
> + }
> + SVN_ERR(err);
> +
> + return SVN_NO_ERROR;
> +}
I think the ILLEGAL_URL error should be cleared.
Daniel
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: [PATCH] Drastically reduce the number of opened ra_sessions during merge.
Posted by Mark Phippard <ma...@gmail.com>.
2008/10/5 Lieven Govaerts <sv...@mobsol.be>:
>
> While profiling the merge code I noticed that svn opens a huge number of
> connections with a serious performance impact. Attached patch will
> drstically reduce the number of sessions and connections opened during
> merge.
Nice! This is something Kamesh has been trying to find time to look
into as he said that this sort of improvement was "there to be made".
I know he is going to be off line until after the summit though, so
not sure when he can review it.
--
Thanks
Mark Phippard
http://markphip.blogspot.com/
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: [PATCH] Drastically reduce the number of opened ra_sessions during merge.
Posted by Justin Erenkrantz <ju...@erenkrantz.com>.
2008/10/5 Lieven Govaerts <sv...@mobsol.be>:
> I'd like to get this patch reviewed by someone familiar with the merge
> code before I commit.
Looks good to me. Few nits below.
> Index: subversion/libsvn_client/merge.c
> ===================================================================
> --- subversion/libsvn_client/merge.c (revision 33458)
> +++ subversion/libsvn_client/merge.c (working copy)
> @@ -2969,7 +2969,8 @@ populate_remaining_ranges(apr_array_header_t *chil
> SVN_ERR(get_full_mergeinfo(&(child->pre_merge_mergeinfo),
> &(child->implicit_mergeinfo), child_entry,
> &(child->indirect_mergeinfo),
> - svn_mergeinfo_inherited, NULL, child->path,
> + svn_mergeinfo_inherited, ra_session,
> + child->path,
> MAX(revision1, revision2),
> MIN(revision1, revision2),
> adm_access, merge_b->ctx, pool));
One other optimization point is that if ra_session *were* NULL at this
point (not quite sure it *could* be, but if it were...), then the new
session value wouldn't be reused across invocations of this call. So,
perhaps get_full_mergeinfo should be taking a double pointer to
ra_session here instead. Not sure - but with it down to 3 sessions at
this point, it's probably far far better than it was. =)
> @@ -6085,7 +6086,38 @@ do_directory_merge(const char *url1,
> return err;
> }
>
> +/** Ensure the caller receives a RA_SESSION object to URL. This function will
> + * reuse RA_SESSION if it is not NULL and is opened to the same repository as
> + * URL is pointing to. Otherwise a new session object will be created.
> + */
> +static svn_error_t *
> +ensure_ra_session_url(svn_ra_session_t **ra_session,
> + const char *url,
> + svn_client_ctx_t *ctx,
> + apr_pool_t *pool)
> +{
> + svn_error_t *err = SVN_NO_ERROR;
>
> + if (*ra_session)
> + {
> + const char *old_session_url;
> + err = svn_client__ensure_ra_session_url(&old_session_url,
> + *ra_session,
> + url,
> + pool);
> + }
> +
> + if ((err && err->apr_err == SVN_ERR_RA_ILLEGAL_URL) || ! *ra_session)
For readability purposes, I'd flip the order of the conditional here. That is:
> + if (! *ra_session || (err && err->apr_err == SVN_ERR_RA_ILLEGAL_URL))
Looks good otherwise. Thanks. -- justin
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org