Re: [PATCH] Re: 1.1rc1 performance regression in 'svn status' (and also 1.1rc2)

"Peter N. Lundblad" <peter@famlundblad.se> writes:

> +void
> +svn_utf_initialize(apr_pool_t *pool)
> +{
> +#if APR_HAS_THREADS
> + apr_threadkey_t *key;
> + if (utf_initialized++ == 0)
> + {
> + if (apr_threadkey_private_create(&key, xlate_cleanup, pool)
> + != APR_SUCCESS)
> + return;

I don't see a call to apr_threadkey_private_destroy. Is this a
resource leak? It's probably trivial.

> + xlate_handle_key = key;
> + }
> +#endif
> +}
> +
> /* Return an apr_xlate handle for converting from FROMPAGE to
> TOPAGE. Create one if it doesn't exist in USERDATA_KEY. If
> unable to find a handle, or unable to create one because
> @@ -50,11 +83,47 @@
> {
> void *old_handle = NULL;
> apr_status_t apr_err;
> + apr_pool_t *handle_pool = NULL;
> +#if APR_HAS_THREADS
> + apr_hash_t *handle_hash = NULL;
> +#endif
>
> /* If we already have a handle, just return it. */
> if (userdata_key)
> {
> - apr_pool_userdata_get (&old_handle, userdata_key, pool);
> +#if APR_HAS_THREADS
> + /* First try our thread private handle. */
> + if (xlate_handle_key)
> + {
> + void *val;
> +
> + if (apr_threadkey_private_get (&val, xlate_handle_key) == APR_SUCCESS)
> + {
> + handle_hash = val;
> + }
> +
> + if (handle_hash)
> + {
> + old_handle = apr_hash_get(handle_hash, userdata_key,
> + APR_HASH_KEY_STRING);
> + handle_pool = apr_hash_pool_get (handle_hash);
> + }
> + else
> + {
> + handle_pool = svn_pool_create (NULL);

This creates a pool in every thread, but I don't see the pool getting
cleared or destroyed. An application that creates threads continually
(e.g. threaded svnserve) will cause the number of pools to grow
without bound.

> + handle_hash = apr_hash_make (pool);
> + if (apr_threadkey_private_set (handle_hash, xlate_handle_key)

If pool is cleared handle_hash will become invalid, but will still be
referenced by the thread private data. The next time the thread calls
get_xlate_handle it will crash, or do something worse.

> + != APR_SUCCESS)
> + {
> + handle_hash = NULL;
> + handle_pool = NULL;
> + }
> + }
> + }
> +#endif
> + /* Try what might be cached in the pool. */
> + if (old_handle == NULL)
> + apr_pool_userdata_get (&old_handle, userdata_key, pool);
> if (old_handle != NULL)
> {
> *ret = old_handle;

-- 
Philip Martin
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org