[cairo] Counting semantics of cairo_ft_scaled_font_lock_face
Carl Worth
cworth at cworth.org
Sat Feb 10 10:16:51 PST 2007
[Sorry for the duplicated message to some of you---I hadn't realized
my first message wasn't directed to the list.]
So, Jan and I have been trying to fix the multi-threaded text locking
bugs. I've attached below my latest series of patches. It seems to
eliminate the bugs we've seen so far.
But, as is, this series is not ready yet as it introduces a
backwards-incompatible change to the semantics of
cairo_ft_scaled_font_lock_face. Previously, this function was
documented as something that could be called multiple times, but patch
#2 below puts a non-recursive mutex in there that would prevent that.
I understand that pango exports a "lock_face" function that's
implemented in terms of cairo's function and with semantics similar to
what cairo has been advertising. So the semantic change in the patch
series below really isn't an option.
I'd be interested in some feedback on how we should tackle this
problem, (and I can provide more detailed background on the issues as
needed).
Thanks,
-Carl
[1.2 0001-Add-missing-_cairo_ft_unscaled_font_unlock_face-to-_cairo_ft_scaled_font_create.txt <text/plain; US-ASCII (7bit)>]
From 1770139b1980db5cecdea28524e2cc6ef044ebeb Mon Sep 17 00:00:00 2001
From: Carl Worth <cworth at cworth.org>
Date: Sat, 10 Feb 2007 09:12:38 -0800
Subject: [PATCH] Add missing _cairo_ft_unscaled_font_unlock_face to _cairo_ft_scaled_font_create
---
src/cairo-ft-font.c | 2 ++
1 files changed, 2 insertions(+), 0 deletions(-)
diff --git a/src/cairo-ft-font.c b/src/cairo-ft-font.c
index 4d66274..0d10dce 100644
--- a/src/cairo-ft-font.c
+++ b/src/cairo-ft-font.c
@@ -1488,6 +1488,8 @@ _cairo_ft_scaled_font_create (cairo_ft_unscaled_font_t *unscaled,
_cairo_scaled_font_set_metrics (&scaled_font->base, &fs_metrics);
+ _cairo_ft_unscaled_font_unlock_face (unscaled);
+
return &scaled_font->base;
}
--
1.5.0.rc1.gf4b6c
[1.3 0002-Add-mutex-to-implement-_cairo_ft_unscaled_font_lock_face-and-_cairo_ft_unscaled_font_unlock_face.txt <text/plain; US-ASCII (7bit)>]
From 560aa761e87c9c7e808fead1e7361c0140055ca8 Mon Sep 17 00:00:00 2001
From: Carl Worth <cworth at cworth.org>
Date: Sat, 10 Feb 2007 09:14:07 -0800
Subject: [PATCH] Add mutex to implement _cairo_ft_unscaled_font_lock_face and _cairo_ft_unscaled_font_unlock_face
Previously we just had an integer counter here, but that is not
sufficient as multiple cairo_scaled_font_t objects, (which are
implicitly shared through the font caches), can reference the
same cairo_ft_unscaled_font_t so real locking is needed here.
This commit eliminates an assertion failure exposed by the
pthread-show-text test case:
lt-pthread-show-text: cairo-ft-font.c:562: _cairo_ft_unscaled_font_unlock_face: Assertion `unscaled->lock > 0' failed.
NOTE: With this change, there are two changes to the documented
behavior of the public cairo_ft_scaled_font_lock_face:
1. Previously, it was documented as not suitable for use in
multi-threaded applications. This is now simply fixed so that
note is eliminated from the documentation.
2. Previously, it was documented as being implemented with a counter,
(such that it could be locked multiple times from the same thread).
However, with the new implementation in terms of an uncounting
mutex this is no longer the case.
XXX: I'm unsure of the impact of this change with respect to
compatibility. What known uses of cairo_ft_scaled_font_lock_face
exist? There's pango at least. Does it call this multiple times?
Are there other known users? Should we figure out how to do
a counting mutex on all supported cairo platforms to avoid the
change in semantics?
---
src/cairo-ft-font.c | 66 ++++++++++++++++++++++++--------------------------
1 files changed, 32 insertions(+), 34 deletions(-)
diff --git a/src/cairo-ft-font.c b/src/cairo-ft-font.c
index 0d10dce..1f2e6e1 100644
--- a/src/cairo-ft-font.c
+++ b/src/cairo-ft-font.c
@@ -100,7 +100,8 @@ struct _cairo_ft_unscaled_font {
cairo_matrix_t current_shape;
FT_Matrix Current_Shape;
- int lock; /* count of how many times this font has been locked */
+ cairo_mutex_t mutex;
+ cairo_bool_t is_locked;
cairo_ft_font_face_t *faces; /* Linked list of faces for this font */
};
@@ -333,7 +334,8 @@ _cairo_ft_unscaled_font_init (cairo_ft_unscaled_font_t *unscaled,
}
unscaled->have_scale = FALSE;
- unscaled->lock = 0;
+ CAIRO_MUTEX_INIT (&unscaled->mutex);
+ unscaled->is_locked = FALSE;
unscaled->faces = NULL;
@@ -366,6 +368,8 @@ _cairo_ft_unscaled_font_fini (cairo_ft_unscaled_font_t *unscaled)
free (unscaled->filename);
unscaled->filename = NULL;
}
+
+ CAIRO_MUTEX_FINI (&unscaled->mutex);
}
static int
@@ -497,7 +501,7 @@ _has_unlocked_face (void *entry)
{
cairo_ft_unscaled_font_t *unscaled = entry;
- return (unscaled->lock == 0 && unscaled->face);
+ return (! unscaled->is_locked && unscaled->face);
}
/* Ensures that an unscaled font has a face object. If we exceed
@@ -512,44 +516,47 @@ _cairo_ft_unscaled_font_lock_face (cairo_ft_unscaled_font_t *unscaled)
cairo_ft_unscaled_font_map_t *font_map;
FT_Face face = NULL;
- if (unscaled->face) {
- unscaled->lock++;
+ CAIRO_MUTEX_LOCK (unscaled->mutex);
+ unscaled->is_locked = TRUE;
+
+ if (unscaled->face)
return unscaled->face;
- }
/* If this unscaled font was created from an FT_Face then we just
* returned it above. */
assert (!unscaled->from_face);
font_map = _cairo_ft_unscaled_font_map_lock ();
- assert (font_map != NULL);
-
- while (font_map->num_open_faces >= MAX_OPEN_FACES)
{
- cairo_ft_unscaled_font_t *entry;
+ assert (font_map != NULL);
- entry = _cairo_hash_table_random_entry (font_map->hash_table,
- _has_unlocked_face);
- if (entry == NULL)
- break;
+ while (font_map->num_open_faces >= MAX_OPEN_FACES)
+ {
+ cairo_ft_unscaled_font_t *entry;
- _font_map_release_face_lock_held (font_map, entry);
+ entry = _cairo_hash_table_random_entry (font_map->hash_table,
+ _has_unlocked_face);
+ if (entry == NULL)
+ break;
+
+ _font_map_release_face_lock_held (font_map, entry);
+ }
}
+ _cairo_ft_unscaled_font_map_unlock ();
if (FT_New_Face (font_map->ft_library,
unscaled->filename,
unscaled->id,
&face) != FT_Err_Ok)
- goto FAIL;
+ {
+ CAIRO_MUTEX_UNLOCK (unscaled->mutex);
+ return NULL;
+ }
unscaled->face = face;
- unscaled->lock++;
font_map->num_open_faces++;
- FAIL:
- _cairo_ft_unscaled_font_map_unlock ();
-
return face;
}
slim_hidden_def (cairo_ft_scaled_font_lock_face);
@@ -559,9 +566,11 @@ slim_hidden_def (cairo_ft_scaled_font_lock_face);
void
_cairo_ft_unscaled_font_unlock_face (cairo_ft_unscaled_font_t *unscaled)
{
- assert (unscaled->lock > 0);
+ assert (unscaled->is_locked);
- unscaled->lock--;
+ unscaled->is_locked = FALSE;
+
+ CAIRO_MUTEX_UNLOCK (unscaled->mutex);
}
slim_hidden_def (cairo_ft_scaled_font_unlock_face);
@@ -2408,19 +2417,8 @@ cairo_ft_font_face_create_for_ft_face (FT_Face face,
* release the face with cairo_ft_font_unlock_face()
* when you are done using it. Since the #FT_Face object can be
* shared between multiple #cairo_scaled_font_t objects, you must not
- * lock any other font objects until you unlock this one. A count is
- * kept of the number of times cairo_ft_font_lock_face() is
- * called. cairo_ft_font_unlock_face() must be called the same number
- * of times.
+ * lock any other font objects until you unlock this one.
*
- * You must be careful when using this function in a library or in a
- * threaded application, because other threads may lock faces that
- * share the same #FT_Face object. For this reason, you must call
- * cairo_ft_lock() before locking any face objects, and
- * cairo_ft_unlock() after you are done. (These functions are not yet
- * implemented, so this function cannot be currently safely used in a
- * threaded application.)
-
* Return value: The #FT_Face object for @font, scaled appropriately,
* or %NULL if @scaled_font is in an error state (see
* cairo_scaled_font_status()) or there is insufficient memory.
--
1.5.0.rc1.gf4b6c
[1.4 0003-Increase-pthread-show-text-thread-count-and-add-cairo_select_font_face-to-expose-more-bugs.txt <text/plain; US-ASCII (7bit)>]
From 8f212771f9ce156d7f76f23b8696c370cac27857 Mon Sep 17 00:00:00 2001
From: Carl Worth <cworth at cworth.org>
Date: Sat, 10 Feb 2007 09:38:13 -0800
Subject: [PATCH] Increase pthread-show-text thread count and add cairo_select_font_face to expose more bugs.
---
test/pthread-show-text.c | 5 ++++-
1 files changed, 4 insertions(+), 1 deletions(-)
diff --git a/test/pthread-show-text.c b/test/pthread-show-text.c
index 94724c7..b674e37 100644
--- a/test/pthread-show-text.c
+++ b/test/pthread-show-text.c
@@ -39,7 +39,7 @@
#include <fontconfig/fontconfig.h>
#endif
-#define NUM_THREADS_DEFAULT 20
+#define NUM_THREADS_DEFAULT 50
#define NUM_ITERATIONS 50
static void *
@@ -62,6 +62,9 @@ start (void *closure)
cairo_move_to (cr, 1, 1);
for (i=0; i < NUM_ITERATIONS; i++) {
+ cairo_select_font_face (cr, "serif",
+ CAIRO_FONT_SLANT_NORMAL,
+ CAIRO_FONT_WEIGHT_NORMAL);
cairo_set_font_size (cr, 8 + i);
cairo_show_text (cr, "Hello world.\n");
}
--
1.5.0.rc1.gf4b6c
[1.5 0004-Add-locking-to-cairo_font_face_reference-destroy.txt <text/plain; US-ASCII (7bit)>]
From bb9f33bc17a053a125c11bb8f2196f22e7b50be4 Mon Sep 17 00:00:00 2001
From: Carl Worth <cworth at cworth.org>
Date: Sat, 10 Feb 2007 09:39:09 -0800
Subject: [PATCH] Add locking to cairo_font_face_reference/destroy
The reference count of cairo_font_face_t is rather intimately tied,
(for toy font faces), with the cairo_font_face_hash_table, so we
expand the existing cairo_toy_font_face_hash_table_mutex to cover
the manipulation of font_face->ref_count as well.
---
src/cairo-font.c | 21 +++++++++++++++++----
1 files changed, 17 insertions(+), 4 deletions(-)
diff --git a/src/cairo-font.c b/src/cairo-font.c
index 3730686..cf32cf1 100644
--- a/src/cairo-font.c
+++ b/src/cairo-font.c
@@ -65,6 +65,8 @@ _cairo_font_face_init (cairo_font_face_t *font_face,
_cairo_user_data_array_init (&font_face->user_data);
}
+CAIRO_MUTEX_DECLARE (cairo_toy_font_face_hash_table_mutex);
+
/**
* cairo_font_face_reference:
* @font_face: a #cairo_font_face_t, (may be NULL in which case this
@@ -85,12 +87,16 @@ cairo_font_face_reference (cairo_font_face_t *font_face)
if (font_face->ref_count == CAIRO_REF_COUNT_INVALID)
return font_face;
+ CAIRO_MUTEX_LOCK (cairo_toy_font_face_hash_table_mutex);
+
/* We would normally assert (font_face->ref_count >0) here but we
* can't get away with that due to the zombie case as documented
* in _cairo_ft_font_face_destroy. */
font_face->ref_count++;
+ CAIRO_MUTEX_UNLOCK (cairo_toy_font_face_hash_table_mutex);
+
return font_face;
}
slim_hidden_def (cairo_font_face_reference);
@@ -112,10 +118,16 @@ cairo_font_face_destroy (cairo_font_face_t *font_face)
if (font_face->ref_count == CAIRO_REF_COUNT_INVALID)
return;
+ CAIRO_MUTEX_LOCK (cairo_toy_font_face_hash_table_mutex);
+
assert (font_face->ref_count > 0);
- if (--(font_face->ref_count) > 0)
+ if (--(font_face->ref_count) > 0) {
+ CAIRO_MUTEX_UNLOCK (cairo_toy_font_face_hash_table_mutex);
return;
+ }
+
+ CAIRO_MUTEX_UNLOCK (cairo_toy_font_face_hash_table_mutex);
font_face->backend->destroy (font_face);
@@ -230,8 +242,6 @@ _cairo_toy_font_face_keys_equal (const void *key_a,
static cairo_hash_table_t *cairo_toy_font_face_hash_table = NULL;
-CAIRO_MUTEX_DECLARE (cairo_toy_font_face_hash_table_mutex);
-
static cairo_hash_table_t *
_cairo_toy_font_face_hash_table_lock (void)
{
@@ -363,8 +373,11 @@ _cairo_toy_font_face_create (const char *family,
&key.base.hash_entry,
(cairo_hash_entry_t **) &font_face))
{
+ /* We increment the reference count here manually to avoid
+ double-locking. */
+ font_face->base.ref_count++;
_cairo_toy_font_face_hash_table_unlock ();
- return cairo_font_face_reference (&font_face->base);
+ return &font_face->base;
}
/* Otherwise create it and insert into hash table. */
--
1.5.0.rc1.gf4b6c
[1.6 0005-Rename-cairo_toy_font_face_hash_table_mutex-to-cairo_font_face_mutex.txt <text/plain; US-ASCII (7bit)>]
From 0acb7a29fb0fba2c38a5fbbc68227baca51f56e1 Mon Sep 17 00:00:00 2001
From: Carl Worth <cworth at cworth.org>
Date: Sat, 10 Feb 2007 09:39:24 -0800
Subject: [PATCH] Rename cairo_toy_font_face_hash_table_mutex to cairo_font_face_mutex
The new name more accurately reflects its recently expanded role.
---
src/cairo-font.c | 31 +++++++++++++++++++------------
1 files changed, 19 insertions(+), 12 deletions(-)
diff --git a/src/cairo-font.c b/src/cairo-font.c
index cf32cf1..a310542 100644
--- a/src/cairo-font.c
+++ b/src/cairo-font.c
@@ -65,7 +65,9 @@ _cairo_font_face_init (cairo_font_face_t *font_face,
_cairo_user_data_array_init (&font_face->user_data);
}
-CAIRO_MUTEX_DECLARE (cairo_toy_font_face_hash_table_mutex);
+/* This mutex protects both cairo_toy_font_hash_table as well as
+ reference count manipulations for all cairo_font_face_t. */
+CAIRO_MUTEX_DECLARE (cairo_font_face_mutex);
/**
* cairo_font_face_reference:
@@ -87,7 +89,7 @@ cairo_font_face_reference (cairo_font_face_t *font_face)
if (font_face->ref_count == CAIRO_REF_COUNT_INVALID)
return font_face;
- CAIRO_MUTEX_LOCK (cairo_toy_font_face_hash_table_mutex);
+ CAIRO_MUTEX_LOCK (cairo_font_face_mutex);
/* We would normally assert (font_face->ref_count >0) here but we
* can't get away with that due to the zombie case as documented
@@ -95,7 +97,7 @@ cairo_font_face_reference (cairo_font_face_t *font_face)
font_face->ref_count++;
- CAIRO_MUTEX_UNLOCK (cairo_toy_font_face_hash_table_mutex);
+ CAIRO_MUTEX_UNLOCK (cairo_font_face_mutex);
return font_face;
}
@@ -118,16 +120,16 @@ cairo_font_face_destroy (cairo_font_face_t *font_face)
if (font_face->ref_count == CAIRO_REF_COUNT_INVALID)
return;
- CAIRO_MUTEX_LOCK (cairo_toy_font_face_hash_table_mutex);
+ CAIRO_MUTEX_LOCK (cairo_font_face_mutex);
assert (font_face->ref_count > 0);
if (--(font_face->ref_count) > 0) {
- CAIRO_MUTEX_UNLOCK (cairo_toy_font_face_hash_table_mutex);
+ CAIRO_MUTEX_UNLOCK (cairo_font_face_mutex);
return;
}
- CAIRO_MUTEX_UNLOCK (cairo_toy_font_face_hash_table_mutex);
+ CAIRO_MUTEX_UNLOCK (cairo_font_face_mutex);
font_face->backend->destroy (font_face);
@@ -238,14 +240,16 @@ _cairo_toy_font_face_keys_equal (const void *key_a,
* our cache and mapping from cairo_font_face_t => cairo_scaled_font_t
* works. Once the corresponding cairo_font_face_t objects fall out of
* downstream caches, we don't need them in this hash table anymore.
+ *
+ * Modifications to this hash table are protected by
+ * cairo_font_face_mutex.
*/
-
static cairo_hash_table_t *cairo_toy_font_face_hash_table = NULL;
static cairo_hash_table_t *
_cairo_toy_font_face_hash_table_lock (void)
{
- CAIRO_MUTEX_LOCK (cairo_toy_font_face_hash_table_mutex);
+ CAIRO_MUTEX_LOCK (cairo_font_face_mutex);
if (cairo_toy_font_face_hash_table == NULL)
{
@@ -253,7 +257,7 @@ _cairo_toy_font_face_hash_table_lock (void)
_cairo_hash_table_create (_cairo_toy_font_face_keys_equal);
if (cairo_toy_font_face_hash_table == NULL) {
- CAIRO_MUTEX_UNLOCK (cairo_toy_font_face_hash_table_mutex);
+ CAIRO_MUTEX_UNLOCK (cairo_font_face_mutex);
return NULL;
}
}
@@ -264,7 +268,7 @@ _cairo_toy_font_face_hash_table_lock (void)
static void
_cairo_toy_font_face_hash_table_unlock (void)
{
- CAIRO_MUTEX_UNLOCK (cairo_toy_font_face_hash_table_mutex);
+ CAIRO_MUTEX_UNLOCK (cairo_font_face_mutex);
}
/**
@@ -484,8 +488,11 @@ _cairo_font_reset_static_data (void)
{
_cairo_scaled_font_map_destroy ();
- CAIRO_MUTEX_LOCK (cairo_toy_font_face_hash_table_mutex);
+ /* We manually acquire the lock rather than calling
+ * cairo_toy_font_face_hash_table_lock simply to avoid
+ * creating the table only to destroy it again. */
+ CAIRO_MUTEX_LOCK (cairo_font_face_mutex);
_cairo_hash_table_destroy (cairo_toy_font_face_hash_table);
cairo_toy_font_face_hash_table = NULL;
- CAIRO_MUTEX_UNLOCK (cairo_toy_font_face_hash_table_mutex);
+ CAIRO_MUTEX_UNLOCK (cairo_font_face_mutex);
}
--
1.5.0.rc1.gf4b6c
[2 <application/pgp-signature (7bit)>]
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 189 bytes
Desc: not available
Url : http://lists.freedesktop.org/archives/cairo/attachments/20070210/c5f99c5c/attachment.pgp
More information about the cairo
mailing list