On 9/27/06, Philip Martin <philip@codematters.co.uk> wrote:
> The new support for "X:" Windows paths has a substantial performance
> penalty, on Unix at least. When running "svn st" on a pristine
> working copy of the Subversion source code the new code needs about
> four times the CPU and runtime of the old code. r21277 seems to be
> the main culprit:
>
> ------------------------------------------------------------------------
> r21277 | lgo | 2006-08-26 21:33:01 +0100 (Sat, 26 Aug 2006) | 35 lines
>
> Prepare fix for issue #2556: abstract root folder checks in svn_path_is_root.
> Add support for 'X:/' as a root folder on Windows.
>
> Patch by: me
> Review by: brane
> dionisos
> philip
> (brane & dionisos basically helped me write the patch)
> Approved by: dionisos
>
> * subversion/include/svn_path.h
> (svn_path_is_root): New function declaration.
> (svn_path_is_empty): Added comment.
>
> * subversion/libsvn_client/commit.c
> (svn_client_commit4): make sure to abort when the iterating through the
> parent folders passese the root folder.
>
> * subversion/libsvn_subr/path.c
> (svn_path_is_root): New function. Tests for all forms considered canonical
> root specs on the platform at hand ('/', 'X:/' etc.)
> (is_canonical): 'X:/' syntax on Windows is canonical.
> (svn_path_join, svn_path_dirname, svn_path_basename,
> svn_path_join_many, previous_segment,
> get_path_ancestor_length): Support the new type of root path on Windows,
> mostly by replacing direct comparisons of path and '/' with a call to
> svn_path_is_root.
> (svn_path_canonicalize): don't strip the trailing slash if the path is
> of the 'X:/' syntax.
>
> * subversion/tests/libsvn_subr/path-test.c
> (test_is_root): New test for validation of svn_path_is_root.
> (test_path_split, test_join,
> test_basename, test_remove_component): add extra cases for 'X:/'.
> (test_funcs): run the new test test_is_root.
> ------------------------------------------------------------------------
>
> I can get most of the performance back by using this hack:
>
> Index: subversion/libsvn_subr/path.c
> ===================================================================
> --- subversion/libsvn_subr/path.c (revision 21676)
> +++ subversion/libsvn_subr/path.c (working copy)
> @@ -437,6 +437,7 @@
> svn_boolean_t
> svn_path_is_root(const char *path, apr_size_t len, apr_pool_t *pool)
> {
> +#if 0
> const char *root_path = NULL;
> apr_status_t status;
> apr_pool_t *strpool = (pool) ? pool : svn_pool_create(NULL);
> @@ -471,6 +472,9 @@
> if (!pool)
> apr_pool_destroy(strpool);
> return result;
> +#else
> + return len == 1 && path[0] == '/';
> +#endif
> }
>
> but it's still not as fast as 1.4. I don't know whether the "X:"
> stuff is responsible for the remaining performance regression. I'm
> using a development build: unoptimised, pool debugging, etc., so
> timings for a release build might vary, but I'd be surprised if it
> made much difference.
>
> I suppose we could make the expensive svn_path_is_root stuff Windows
> specific, but really I don't think the current stuff is suitable
> anywhere. When I added the is_canonical stuff it was intended to be
> fast, I even commented out some of the calls I used during development
> in order to avoid extra strlen calls. Now is_canonical is creating
> pools and doing utf8 conversions, it's simply much too slow.
Well, we could put all those is_canonical calls behind a compiler
conditional SVN_DEBUG_PATH_API or something like it. Even normal debug
builds won't include that code if we do that. You hinted yourself at
removal of is_canonical when the pool creation was discussed.
There are probably some calls remaining where there is no pool in a
public api which is now calling the path_is_root functions. I have
suggested we create new api functions for those which need a pool and
deprecate the ones without a pool parameter. Instead of forwarding the
old api calls to the new api, we could leave in the broken code,
fixing only clients which are updated to the new api (but keeping the
old performance in all cases).
bye,
Erik.
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Received on Wed Sep 27 14:46:59 2006