You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@subversion.apache.org by "Peter N. Lundblad" <pe...@famlundblad.se> on 2004/08/16 20:45:56 UTC
[PATCH] Re: 1.1rc1 performance regression in 'svn status' (and also
1.1rc2)
On Sun, 15 Aug 2004, Greg Hudson wrote:
> On Sun, 2004-08-15 at 16:41, Peter N. Lundblad wrote:
> > I don't see a way to improve the performance via caching in pools. If we
> > want to fix this, I think we need to add init functions and be able to
> > detect if they were called. If this is problematic for 1.1 ( to which I
> > might agree), then we won't fix this until 1.2. This will hurt TSVN users
> > I think and that's bad because that's an important application on Windows.
>
> Let's introduce an optional initializer, but let's make it very limited
> and specific. "void svn_utf_initialize(void);". We acknowledge that
> it's a short-term hack. It can become a no-op in 1.x and can disappear
> in 2.0.
>
Here is this hack in the form of a patch that could have some review. If
someone is actually using the svn libs in a multi-threaded environment,
please test. I'm running the testsuite right now, and it seems to work.
I kept the original caching in the pool to not hurt performance even more
if svn_utf_initialize wasn't called. An alternative would be to drop it
and tell people to use this new API if they feel svn is too slow. Maybe
this cleanup could wait until we have the real solution in place.
Comments?
//Peter
Re: [PATCH] Issue 2016, next try (was Re: Re: 1.1rc1 performance
regression in 'svn status' (and also 1.1rc2))
Posted by "Peter N. Lundblad" <pe...@famlundblad.se>.
On Thu, 26 Aug 2004, Greg Hudson wrote:
> Your docstring for svn_utf_initialize says:
>
> +/** @since New in 1.1.
> + * Initialize the utf8 encoding/decoding routines.
> + * NOTE: It is optional to call this function, but if it is used, no other
> + * svn_utf function may be in use during the call of this function.
> + * Initializing the UTF-8 routines will improve performance.
> + */
> +void svn_utf_initialize (apr_pool_t *pool);
>
> There was a reason I said "no Subversion library function may be in use"
> when I described the constraint; it's clearer to describe it that way.
> Almost any Subversion library function is liable to use svn_utf
> functions.
>
I changed it to require that no svn function may be active during this
call. So you can create a pool before this call, which is important for
cleanup order and performance.
> If you're going to take a pool parameter, document that the pool must
> live for as long as Subversion library functions are in use.
> (Another option is to create a pool from scratch like we do in
> svn_error_create(). But on consideration, I think that's a bad idea,
> since that pool could never be cleaned up.)
>
OK. The docstring wasn't ready yet. Really, I wanted to get feedback on
the approach more than the exact patch, but I could have been clearer on
that... I removed the pool, since we need to create a subpool anyway,
which we can protect with the mutex. Pools created with no parent are
really owned by an internal pool in APR which will be destroyed during
apr_terminate. The patch works even if something uses UTF-8 translations
after the cache is cleaned up, but then without the cache ofcourse. I'm
assuming that all other threads are finished when apr_terminate gets
called. This must be required anyway...
> + if (utf_initialized++ == 0)
>
> If you're treating utf_initialized as a flag, then it's stylistically
> confusing to operate on it like it's a counter. APR treats its
> initialization static as a counter because it wants to count
> apr_terminate() calls to know when it should clean up.
>
You're right. I removed the flag completely and use the hash table pointer
as a flag instead.
> I'll review more thoroughly when you've written a log message. It's
> good to do that before asking for review.
>
You're right again. As I said, I asked more about the concept, but
expecting you to read my mind might be to require too much:-)
This time, it would be nice with some review. This got complicated, at
least for me...
Thanks,
//Peter
Re: [PATCH] Issue 2016 (was Re: Re: 1.1rc1 performance regression
in 'svn status' (and also 1.1rc2))
Posted by Greg Hudson <gh...@MIT.EDU>.
Your docstring for svn_utf_initialize says:
+/** @since New in 1.1.
+ * Initialize the utf8 encoding/decoding routines.
+ * NOTE: It is optional to call this function, but if it is used, no other
+ * svn_utf function may be in use during the call of this function.
+ * Initializing the UTF-8 routines will improve performance.
+ */
+void svn_utf_initialize (apr_pool_t *pool);
There was a reason I said "no Subversion library function may be in use"
when I described the constraint; it's clearer to describe it that way.
Almost any Subversion library function is liable to use svn_utf
functions.
If you're going to take a pool parameter, document that the pool must
live for as long as Subversion library functions are in use.
(Another option is to create a pool from scratch like we do in
svn_error_create(). But on consideration, I think that's a bad idea,
since that pool could never be cleaned up.)
+ if (utf_initialized++ == 0)
If you're treating utf_initialized as a flag, then it's stylistically
confusing to operate on it like it's a counter. APR treats its
initialization static as a counter because it wants to count
apr_terminate() calls to know when it should clean up.
I'll review more thoroughly when you've written a log message. It's
good to do that before asking for review.
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
[PATCH] Issue 2016 (was Re: Re: 1.1rc1 performance regression in
'svn status' (and also 1.1rc2))
Posted by "Peter N. Lundblad" <pe...@famlundblad.se>.
On Tue, 17 Aug 2004, Philip Martin wrote:
> "Peter N. Lundblad" <pe...@famlundblad.se> writes:
>
> > - Have a global hash with mappings from handle name (identifying the
> > translation) to a linked list of xlate handles.
> > - When you want to translate:
> > . Lock a mutex.
> > . Look for a handle in the hash. If one is found, remove it from the list
> > and use it.
> > . If no handle was found, create one.
> > . Unlock the mutex
> > . Use the handle.
> > . Lock, put the handle back into the list and unlock.
>
> Just make sure you add/remove from the list without allocating memory,
> > This avoids having the lock held during the translation. It will create as
> > many handles as is needed simultaneously and destroy them at program
> > termination.
> >
> > It would be simpler to just have one handle per combination of to/from
> > encoding, but as Greg pointed out, this might give to much contention on
> > the translation lock, especially since we do a lot of translations during
> > our WC crawling commands.
>
> It's notoriously difficult to predict thread contention accurately.
> However, instinctively I prefer your other design, the one that
> provides per-thread xlate handles, assuming it can be implemented
> correctly.
>
OK. Here is the patch. It keeps the lock during apr_xlate_open, but that
call should be very rare compared to the number of translations we do. It
isn't ready yet, but quite so and I'd like feedback.
Regards,
//Peter
Re: [PATCH] Re: 1.1rc1 performance regression in 'svn status' (and
also 1.1rc2)
Posted by Philip Martin <ph...@codematters.co.uk>.
"Peter N. Lundblad" <pe...@famlundblad.se> writes:
> - Have a global hash with mappings from handle name (identifying the
> translation) to a linked list of xlate handles.
> - When you want to translate:
> . Lock a mutex.
> . Look for a handle in the hash. If one is found, remove it from the list
> and use it.
> . If no handle was found, create one.
> . Unlock the mutex
> . Use the handle.
> . Lock, put the handle back into the list and unlock.
Just make sure you add/remove from the list without allocating memory,
otherwise the memory use will grow without bound. Allocating memory
per handle is OK, allocating memory per use of a handle is not.
> This avoids having the lock held during the translation. It will create as
> many handles as is needed simultaneously and destroy them at program
> termination.
>
> It would be simpler to just have one handle per combination of to/from
> encoding, but as Greg pointed out, this might give to much contention on
> the translation lock, especially since we do a lot of translations during
> our WC crawling commands.
It's notoriously difficult to predict thread contention accurately.
However, instinctively I prefer your other design, the one that
provides per-thread xlate handles, assuming it can be implemented
correctly.
> Comments/flames/... Maybe we just hve to give up on this one?
One report had "svn st" more then 10x slower on a large wc, that's
1.0.x compared to 1.1.x on win32. I'm glad I don't use Windows!
--
Philip Martin
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: [PATCH] Re: 1.1rc1 performance regression in 'svn status' (and
also 1.1rc2)
Posted by "Peter N. Lundblad" <pe...@famlundblad.se>.
Hi,
Here is another idea for solving this caching problem.
WARNING, WARNING, WARNING: I am starting to feel insane solving this one.
It's a bad regressions, but we have to stop somewhere.
- Have a global hash with mappings from handle name (identifying the
translation) to a linked list of xlate handles.
- When you want to translate:
. Lock a mutex.
. Look for a handle in the hash. If one is found, remove it from the list
and use it.
. If no handle was found, create one.
. Unlock the mutex
. Use the handle.
. Lock, put the handle back into the list and unlock.
This avoids having the lock held during the translation. It will create as
many handles as is needed simultaneously and destroy them at program
termination.
It would be simpler to just have one handle per combination of to/from
encoding, but as Greg pointed out, this might give to much contention on
the translation lock, especially since we do a lot of translations during
our WC crawling commands.
Comments/flames/... Maybe we just hve to give up on this one?
Regards,
//Peter
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: [PATCH] Re: 1.1rc1 performance regression in 'svn status' (and
also 1.1rc2)
Posted by "Peter N. Lundblad" <pe...@famlundblad.se>.
On Wed, 18 Aug 2004, Philip Martin wrote:
> Philip Martin <ph...@codematters.co.uk> writes:
>
> > "Peter N. Lundblad" <pe...@famlundblad.se> writes:
> >
> >>> > + 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.
> >>>
> >> xlate_cleanup will take care of that. It is called when the thread is
> >> destoyed.
> >
> > Unless you are planning to write some more code I don't understand.
> > xlate_cleanup appears to be applied to the pool passed to
> > svn_utf_initialize. That pool could well have a lifetime much longer
> > than the thread lifetime, typically I would expect the pool to last as
> > long as the application runs.
>
> I see now, it's a thread cleanup not a pool cleanup. That would work
> if APR were not broken as you point out below.
>
Then I understand your confusion...
> >> Problems is, when I check the implementation of
> >> apr_threadkey_private_create, the destructor is only used on POSIX and
> >> netware systems, not on Windows/OS2. So on these platforms, it is a leak
> >> after all. Maybe there is no way to register cleanups on these
> >> platforms,. This should have been documented in the APR headers!!!
>
> That does look broken, it should probably be brought up on the dev@apr
> list.
>
> Without a working thread destructor it will be very hard to use a
> thread private solution to solve the caching problem.
>
It will be hard to use thread-specific storage from a library in general,
or from an application when you don't have control of the thread
creation/termination. I don't know, however, if it is possible to get the
destructors called on Win32. The Win32 TLS doesn't have destructors.
POssibly you could solve it in DllMain if APR is linked dynamically.
Anyway, this is an APR question, not svn...
> >>
> >>> > + 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.
> >>>
> >> When the pool is cleared, the thread is dying, so the TSD for that thread
> >> isn't used again. This would be true where the pool is destroyed at all...
> >
> > Again I don't understand. pool is an argument to one of the
> > svn_utf_xxx_(to/from)_xxx functions, it could well have a lifetime
> > much shorter than the thread and may well get repeatedly cleared.
>
> Did you mean to allocate handle_hash from handle_pool rather than
> pool? Then the hash would have the correct lifetime. It's all a bit
> academic if thread destructors don't work on win32.
>
Yes and yes. I noticed that error later.
Regards,
//Peter
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: [PATCH] Re: 1.1rc1 performance regression in 'svn status' (and
also 1.1rc2)
Posted by Philip Martin <ph...@codematters.co.uk>.
Philip Martin <ph...@codematters.co.uk> writes:
> "Peter N. Lundblad" <pe...@famlundblad.se> writes:
>
>>> > + 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.
>>>
>> xlate_cleanup will take care of that. It is called when the thread is
>> destoyed.
>
> Unless you are planning to write some more code I don't understand.
> xlate_cleanup appears to be applied to the pool passed to
> svn_utf_initialize. That pool could well have a lifetime much longer
> than the thread lifetime, typically I would expect the pool to last as
> long as the application runs.
I see now, it's a thread cleanup not a pool cleanup. That would work
if APR were not broken as you point out below.
>> Problems is, when I check the implementation of
>> apr_threadkey_private_create, the destructor is only used on POSIX and
>> netware systems, not on Windows/OS2. So on these platforms, it is a leak
>> after all. Maybe there is no way to register cleanups on these
>> platforms,. This should have been documented in the APR headers!!!
That does look broken, it should probably be brought up on the dev@apr
list.
Without a working thread destructor it will be very hard to use a
thread private solution to solve the caching problem.
>>
>>> > + 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.
>>>
>> When the pool is cleared, the thread is dying, so the TSD for that thread
>> isn't used again. This would be true where the pool is destroyed at all...
>
> Again I don't understand. pool is an argument to one of the
> svn_utf_xxx_(to/from)_xxx functions, it could well have a lifetime
> much shorter than the thread and may well get repeatedly cleared.
Did you mean to allocate handle_hash from handle_pool rather than
pool? Then the hash would have the correct lifetime. It's all a bit
academic if thread destructors don't work on win32.
--
Philip Martin
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: [PATCH] Re: 1.1rc1 performance regression in 'svn status' (and
also 1.1rc2)
Posted by Philip Martin <ph...@codematters.co.uk>.
"Peter N. Lundblad" <pe...@famlundblad.se> writes:
>> > + 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.
>>
> xlate_cleanup will take care of that. It is called when the thread is
> destoyed.
Unless you are planning to write some more code I don't understand.
xlate_cleanup appears to be applied to the pool passed to
svn_utf_initialize. That pool could well have a lifetime much longer
than the thread lifetime, typically I would expect the pool to last as
long as the application runs.
> Problems is, when I check the implementation of
> apr_threadkey_private_create, the destructor is only used on POSIX and
> netware systems, not on Windows/OS2. So on these platforms, it is a leak
> after all. Maybe there is no way to register cleanups on these
> platforms,. This should have been documented in the APR headers!!!
>
>> > + 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.
>>
> When the pool is cleared, the thread is dying, so the TSD for that thread
> isn't used again. This would be true where the pool is destroyed at all...
Again I don't understand. pool is an argument to one of the
svn_utf_xxx_(to/from)_xxx functions, it could well have a lifetime
much shorter than the thread and may well get repeatedly cleared.
--
Philip Martin
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: [PATCH] Re: 1.1rc1 performance regression in 'svn status' (and
also 1.1rc2)
Posted by "Peter N. Lundblad" <pe...@famlundblad.se>.
On Tue, 17 Aug 2004, Philip Martin wrote:
> "Peter N. Lundblad" <pe...@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.
>
You're right. It isn't destroyed by the pool (strangely enough), so
apr_threadkey_private_delete needs to be registered in the pool
explicitly.
> > + 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.
>
xlate_cleanup will take care of that. It is called when the thread is
destoyed.
Problems is, when I check the implementation of
apr_threadkey_private_create, the destructor is only used on POSIX and
netware systems, not on Windows/OS2. So on these platforms, it is a leak
after all. Maybe there is no way to register cleanups on these
platforms,. This should have been documented in the APR headers!!!
> > + 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.
>
When the pool is cleared, the thread is dying, so the TSD for that thread
isn't used again. This would be true where the pool is destroyed at all...
Thanks for the review. Currently, I don't know how to get further.
Creating a set of xlate handles per thread is a leak, since there is no
way to clean it up. Caching the handles in parent pools doesn't work,
since that's not thread-safe. Well... Anyone has an idea??? Maybe we could
check if it is possible to decrease the number of translations by some
factor? I'll give it a try.
Regards,
//Peter
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: [PATCH] Re: 1.1rc1 performance regression in 'svn status' (and
also 1.1rc2)
Posted by Philip Martin <ph...@codematters.co.uk>.
"Peter N. Lundblad" <pe...@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