David Mankin wrote:
> A couple weeks ago, someone was doing profiling on the code. It was
> briefly claimed (and later discounted) that strlen() was causing a
> noticeable slow-down. So, I went through the code base and looked at
> each use of strlen() to find out if it was necessary. It turns out
> the svn code is not doing very many excessive strlen calls. But there
> are a few. Find below the first strlen optimization patches.
> Probably, in the grand scheme of things, it's not worth doing this
> optimization. However, since I've already done it, I say it should be
> reviewed and applied. :-)
Well, here's the review, and it should not be applied (except possibly
the one correct change).
> Index: subversion/include/svn_string.h
> ===================================================================
> --- subversion/include/svn_string.h (revision 3951)
> +++ subversion/include/svn_string.h (working copy)
> @@ -262,6 +262,24 @@
> svn_boolean_t chop_whitespace,
> apr_pool_t *pool);
>
> +/* Checks to see if cstring S is at least LEN characters.
> + * Better than calling strlen because it only compares at most
> + * LEN characters.
> + *
> + * Returns TRUE when S is at least LEN characters, FALSE otherwise.
> + */
> +svn_boolean_t svn_cstring_ensure_len (const char *s,
> + apr_size_t len);
> +
> +
> +/* Returns 1 if A is longer than B, -1 if B is longer than A, and
> + * 0 if A and B are the same length.
> + *
> + * This is better than using (strlen(a) > strlen(b)) because only
> + * O(min(len(a), len(b))) chars are examined instead of O(len(a) +
> len(b))
> + */
> +int svn_cstring_strlencmp (const char* a,
> + const char* b);
It's a common misconception that hand-coded functions will be faster
than what the compiler provides. Modern compilers will generate highly
optimized inline code for string functions that uses word-length
instructions where possible. So it's entirely possible, expecially
taking function call overhead into account, that your functions are
actually slower than what was being used now. You should _always_
measure the effects of your "optimizations", even though they might
seem oviously correct to you.
> Index: subversion/libsvn_client/auth.c
> ===================================================================
> --- subversion/libsvn_client/auth.c (revision 3951)
> +++ subversion/libsvn_client/auth.c (working copy)
> @@ -149,7 +149,7 @@
> svn_client__callback_baton_t *cb = baton;
> svn_client_auth_baton_t *ab = cb->auth_baton;
>
> - if (strlen(username) > 0)
> + if (*username)
> prompt = apr_psprintf (pool, "%s's password: ", username);
> else
> prompt = apr_psprintf (pool, "password: ");
This change is correct.
> Index: subversion/clients/cmdline/feedback.c
> ===================================================================
> --- subversion/clients/cmdline/feedback.c (revision 3951)
> +++ subversion/clients/cmdline/feedback.c (working copy)
> @@ -191,7 +191,6 @@
>
> case svn_wc_notify_commit_added:
> if (mime_type
> - && ((strlen (mime_type)) > 5)
> && ((strncmp (mime_type, "text/", 5)) != 0))
> printf ("Adding (bin) %s\n", path_native);
> else
This one, OTOH, is _wrong_. Your change will match "text/", which is
_not_ a valid MIME type!
> Index: subversion/libsvn_ra_dav/fetch.c
> ===================================================================
> --- subversion/libsvn_ra_dav/fetch.c (revision 3951)
> +++ subversion/libsvn_ra_dav/fetch.c (working copy)
> @@ -1224,7 +1224,7 @@
> }
> }
>
> - if (strlen(url) > strlen(bc_root))
> + if (svn_cstring_strlencmp(url, bc_root) > 0)
> {
> const char *comp;
> comp = svn_path_uri_decode(svn_path_basename(url, subpool),
See above. -1 on committing things like this until you can prove that
your change actually makes a measurable difference -- for the better.
--
Brane Čibej <brane_at_xbc.nu> http://www.xbc.nu/brane/
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Received on Fri Dec 6 04:34:00 2002