You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@subversion.apache.org by Justin Erenkrantz <je...@apache.org> on 2002/11/08 04:19:13 UTC
[PATCH] Remove use of global_pool to apr_xlate
There might be some implications about opening multiple iconv streams
per process or thread, but I don't see anything that leads me to
believe that iconv is inherently thread-unsafe.
Philip, can you please try this patch out with your stress test? --
justin
* subversion/libsvn_subr/utf.c (get_ntou_xlate_handle,
get_uton_xlate_handle): No longer walk up the tree looking for
global_pool, but rather just use the passed-in pool.
Index: subversion/libsvn_subr/utf.c
===================================================================
--- subversion/libsvn_subr/utf.c (revision 3685)
+++ subversion/libsvn_subr/utf.c (working copy)
@@ -45,30 +45,17 @@
get_ntou_xlate_handle (apr_xlate_t **ret, apr_pool_t *pool)
{
void *old_handle = NULL;
- apr_pool_t *parent, *global_pool;
apr_status_t apr_err;
- /* ### I'm worried about the performance implications of searching
- * up the pool tree every time we call this function. Leaving it
in
- * for now, but if this turns out to be a bottleneck, then we
should
- * store the xlation handles in some more quickly accessible place.
- *
- * -kfogel, 7 July 2002
- */
- /* Find the global pool */
- for (global_pool = pool;
- (parent = apr_pool_get_parent (global_pool));
- global_pool = parent) ;
-
/* If we already have a handle, just return it. */
- apr_pool_userdata_get (&old_handle, SVN_UTF_NTOU_XLATE_HANDLE,
global_pool);
+ apr_pool_userdata_get (&old_handle, SVN_UTF_NTOU_XLATE_HANDLE,
pool);
if (old_handle != NULL) {
*ret = old_handle;
return SVN_NO_ERROR;
}
/* Try to create one. */
- apr_err = apr_xlate_open (ret, "UTF-8", APR_LOCALE_CHARSET,
global_pool);
+ apr_err = apr_xlate_open (ret, "UTF-8", APR_LOCALE_CHARSET, pool);
if (APR_STATUS_IS_EINVAL (apr_err) || APR_STATUS_IS_ENOTIMPL
(apr_err))
{
@@ -81,7 +68,7 @@
/* Save it for later. */
apr_pool_userdata_set (*ret, SVN_UTF_NTOU_XLATE_HANDLE,
- apr_pool_cleanup_null, global_pool);
+ apr_pool_cleanup_null, pool);
return SVN_NO_ERROR;
}
@@ -96,30 +83,17 @@
get_uton_xlate_handle (apr_xlate_t **ret, apr_pool_t *pool)
{
void *old_handle = NULL;
- apr_pool_t *parent, *global_pool;
apr_status_t apr_err;
- /* ### I'm worried about the performance implications of searching
- * up the pool tree every time we call this function. Leaving it
in
- * for now, but if this turns out to be a bottleneck, then we
should
- * store the xlation handles in some more quickly accessible place.
- *
- * -kfogel, 7 July 2002
- */
- /* Find the global pool */
- for (global_pool = pool;
- (parent = apr_pool_get_parent (global_pool));
- global_pool = parent) ;
-
/* If we already have a handle, just return it. */
- apr_pool_userdata_get (&old_handle, SVN_UTF_UTON_XLATE_HANDLE,
global_pool);
+ apr_pool_userdata_get (&old_handle, SVN_UTF_UTON_XLATE_HANDLE,
pool);
if (old_handle != NULL) {
*ret = old_handle;
return SVN_NO_ERROR;
}
/* Try to create one. */
- apr_err = apr_xlate_open (ret, APR_LOCALE_CHARSET, "UTF-8",
global_pool);
+ apr_err = apr_xlate_open (ret, APR_LOCALE_CHARSET, "UTF-8", pool);
if (APR_STATUS_IS_EINVAL (apr_err) || APR_STATUS_IS_ENOTIMPL
(apr_err))
{
@@ -132,7 +106,7 @@
/* Save it for later. */
apr_pool_userdata_set (*ret, SVN_UTF_UTON_XLATE_HANDLE,
- apr_pool_cleanup_null, global_pool);
+ apr_pool_cleanup_null, pool);
return SVN_NO_ERROR;
}
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: [PATCH] Remove use of global_pool to apr_xlate
Posted by Philip Martin <ph...@codematters.co.uk>.
Justin Erenkrantz <je...@apache.org> writes:
> Okay, since it seems to work, should I go and commit it?
I'd say yes :)
I think there is a performance impact for single threaded
applications, as there is much less reuse of apr_xlate_t objects.
Anything that does UTF8 conversion using several pools will need to
create one conversion object for every pool.
> BTW, yeah, I really haven't figured out how to get my mailer to not
> munge patches. I might have to attach next time. Grr. -- justin
The problem is that it splits long lines, when I joined the lines
together the patch was OK.
--
Philip Martin
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: [PATCH] Remove use of global_pool to apr_xlate
Posted by Karl Fogel <kf...@newton.ch.collab.net>.
Justin Erenkrantz <je...@apache.org> writes:
> Bing!
>
> Okay, since it seems to work, should I go and commit it?
IMHO sure, if it passes 'make check' of course.
(Don't forget to mention "issue #974" in the log message, and annotate
http://subversion.tigris.org/issues/show_bug.cgi?id=974 with the
revision of the commit.)
Thanks for the quick work, sir.
-K
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: [PATCH] Remove use of global_pool to apr_xlate
Posted by Justin Erenkrantz <je...@apache.org>.
--On Friday, November 8, 2002 1:57 PM +0000 Philip Martin
<ph...@codematters.co.uk> wrote:
> Well, a little thought explains it :) With your patch each thread
> will allocates its own apr_xlate_t as it only looks in its own
> thread specific pool for an existing instance.
Bing!
Okay, since it seems to work, should I go and commit it?
BTW, yeah, I really haven't figured out how to get my mailer to not
munge patches. I might have to attach next time. Grr. -- justin
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: [PATCH] Remove use of global_pool to apr_xlate
Posted by Philip Martin <ph...@codematters.co.uk>.
Philip Martin <ph...@codematters.co.uk> writes:
> So while your patch cures the problem, I don't know why it
> cures it.
Well, a little thought explains it :) With your patch each thread
will allocates its own apr_xlate_t as it only looks in its own thread
specific pool for an existing instance.
--
Philip Martin
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: [PATCH] Remove use of global_pool to apr_xlate
Posted by Philip Martin <ph...@codematters.co.uk>.
Philip Martin <ph...@codematters.co.uk> writes:
> > * subversion/libsvn_subr/utf.c (get_ntou_xlate_handle,
> > get_uton_xlate_handle): No longer walk up the tree looking for
> > global_pool, but rather just use the passed-in pool.
>
> I think you have fixed it. I've just run the test to get 600
> revisions comitted, hundreds more commit attempts, and 18 minutes of
> CPU spread over 6 httpd threads. Before applying the patch the test
> would normally fail before 50 revisions.
>
> Do you know what the problem was? Perhaps it was that the
> apr_pool_userdata_get in one thread is affected by another thread
> doing apr_pool_userdata_set? I can see that would cause problems if
> the set causes the hash table to reallocate.
I'm using this patch to catch the failures (I'm using all ASCII paths
so the conversions should all be trivial)
Index: ../svn/subversion/libsvn_subr/utf.c
===================================================================
--- ../svn/subversion/libsvn_subr/utf.c (revision 3691)
+++ ../svn/subversion/libsvn_subr/utf.c (working copy)
@@ -401,6 +401,16 @@
SVN_ERR (convert_to_stringbuf (convset, src, strlen (src),
&destbuf, pool));
*dest = destbuf->data;
+
+ if (strcmp (src, destbuf->data))
+ {
+ apr_xlate_t *convset2;
+ svn_boolean_t spin = TRUE;
+ while (spin)
+ {
+ }
+ svn_utf_cstring_from_utf8 (dest, src, pool);
+ }
}
else
{
so I can attach gdb just after it fails. Here's what I see
0x4043bc74 in svn_utf_cstring_from_utf8 (dest=0xbf1ff82c,
src=0x81735d0 "/home/pm/sw/subversion/obj/subversion/tests/repostress/locks/db.lock", pool=0x40638ed8) at ../svn/subversion/libsvn_subr/utf.c:409
409 while (spin)
(gdb) p src
$1 = 0x81735d0 "/home/pm/sw/subversion/obj/subversion/tests/repostress/locks/db.lock"
(gdb) p destbuf->data
$2 = 0x8163ca8 "Èw:@Èw:@"
(gdb) p convset
$3 = (struct apr_xlate_t *) 0x406075e0
Now I step into the repeated svn_utf_cstring_from_utf8 call which
retrieves the convset again and I get the same value as the failed
call. Stepping further the conversion is repeated but this time it
works. So while your patch cures the problem, I don't know why it
cures it.
--
Philip Martin
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: [PATCH] Remove use of global_pool to apr_xlate
Posted by Philip Martin <ph...@codematters.co.uk>.
Justin Erenkrantz <je...@apache.org> writes:
> There might be some implications about opening multiple iconv streams
> per process or thread, but I don't see anything that leads me to
> believe that iconv is inherently thread-unsafe.
I was looking at apr_xlate_t and it does look thread safe.
> Philip, can you please try this patch out with your stress test? --
> justin
Is it me, or did your mailer mangle this patch?
> * subversion/libsvn_subr/utf.c (get_ntou_xlate_handle,
> get_uton_xlate_handle): No longer walk up the tree looking for
> global_pool, but rather just use the passed-in pool.
I think you have fixed it. I've just run the test to get 600
revisions comitted, hundreds more commit attempts, and 18 minutes of
CPU spread over 6 httpd threads. Before applying the patch the test
would normally fail before 50 revisions.
Do you know what the problem was? Perhaps it was that the
apr_pool_userdata_get in one thread is affected by another thread
doing apr_pool_userdata_set? I can see that would cause problems if
the set causes the hash table to reallocate.
--
Philip Martin
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org