Making PangoCairo threadsafe

Hi,

So, I there was this more-than-evil bug
Bug 356666 – pango is not thread-safe, nautilus does not
honour that
http
://bugzilla.gnome.org/show_bug.cgi?id=356666

that I wanted to fix.  The problem is that the per-fontmap
PangoCairoRenderer that pangocairo is using was being used
by two
threads (one Gtk+ UI stuff, another a librsvg thumbnailer)
and that's
not good.  After a quick check with mclasen and otaylor, I
chose to go
for mutex-ing the renderer, make a single cached instance of
it, and
create a new one if the cached one is being used.  Here is
the patch
that I committed and is in pango-1.14.8 that was just
released:

  http://bugzilla.gnome.org/attachment.cgi?id=769
25&action=view

The patch is fairly simple.  More interesting is the test
case I wrote:

  htt
p://bugzilla.gnome.org/attachment.cgi?id=76942

The cool thing about the test is that it's very simple, yet
hits the bug
very easily.  No more after the patch.  However, if you look
at the test
case, I'm calling into Pango once before spawning the
threads.  If you
comment that line, it will blow up in a number of places,
causing a
crash or getting boxes only in the output.

So, Pango is not thread-safe, and advertised so.  However,
people seem
to believe that it's safe to use it as long as a single
PangoContext or
PangoLayout instance is not used from multiple threads
simultaneously
(which is not true either).  I'm looking into achieving
that, and I can
use some help.

Both Pango and Cairo are designed to be thread-safe,
API-wise.  No
problem there.  However, there are bugs in both.  There is a
patch for
cairo already that may or may not be enough:

  htt
ps://bugs.freedesktop.org/show_bug.cgi?id=8801

As for pango, I did some grepping to identify places that
need a fix:

  - First, the static variables.  Assuming that
g_enum_register_static(), g_type_register_static(), and
g_quark_from_static_string() are idempotent, and ignoring
the pangox
backend, it comes down to:

$ grep -E '<static>[^(]*[;=]' *.c  | grep -v
'<const>' | grep -v -E
'<(GType|GQuark|querymodules.c)>' | grep -v -E
'querymodules|pangox[-.]|type_id>'
modules.c:static GList *maps = NULL;
modules.c:static GSList *registered_engines = NULL;
modules.c:static GSList *dlloaded_engines = NULL;
modules.c:static GHashTable *dlloaded_modules;
modules.c:static GObjectClass *parent_class;
modules.c:  static GEnumClass *class = NULL;
modules.c:  static gboolean init = FALSE;
modules.c:      static gboolean no_module_warning = FALSE;
pangoatsui-fontmap.c:static gpointer
pango_atsui_family_parent_class;
pangoatsui-fontmap.c:static gpointer
pango_atsui_face_parent_class;
pango-attributes.c:  static guint current_type = 0x1000000;
pangocairo-fontmap.c:  static PangoFontMap *default_font_map
= NULL;
pangocairo-render.c:static PangoCairoRenderer
*cached_renderer = NULL;
pangocairo-render.c:static GStaticMutex
cached_renderer_mutex = G_STATIC_MUTEX_INIT;
pango-engine.c:  static PangoEngineShape *fallback_shaper =
NULL;
pangofc-fontmap.c:  static gboolean registered_modules =
FALSE;
pangofc-fontmap.c:  static GEnumClass *class = NULL;
pango-fontmap.c:  static GHashTable *warned_fonts = NULL;
pango-fontset.c:static PangoFontsetClass
*simple_parent_class;  /* Parent class structure for
PangoFontsetSimple */
pangoft2.c:      static char *default_msg = NULL;
pangoft2-fontmap.c:static PangoFT2FontMap
*pango_ft2_global_fontmap = NULL;
pango-ot-info.c:static GObjectClass *parent_class;
pango-ot-ruleset.c:static GObjectClass *parent_class;
pango-script.c:  static int saved_mid =
PANGO_SCRIPT_TABLE_MIDPOINT;
pango-utils.c:static GHashTable *pango_aliases_ht = NULL;
pango-utils.c:static GHashTable *config_hash = NULL;
pango-utils.c:  static gchar *result = NULL;
pango-utils.c:  static gchar *result = NULL;
pango-utils.c:  static GHashTable *hash = NULL;
pangowin32.c:  static guint last = 0; /* Cache of one */
pangowin32-fontmap.c:static PangoWin32FontMap
*default_fontmap = NULL;
pangoxft-fontmap.c:static GSList *fontmaps = NULL;
pangoxft-fontmap.c:static GSList *registered_displays;

  - Next, g_object_set_[q]data*():

$ grep g_object_set_ *.c | grep -v warned_quark
pangocairo-font.c:      g_object_set_data_full (G_OBJECT
(cfont), "hex_box_info", NULL, NULL); 
pangocairo-font.c:  g_object_set_data_full (G_OBJECT
(cfont), "hex_box_info", hbi,
(GDestroyNotify)_pango_cairo_hex_box_info_destroy); 
pangocairo-fontmap.c:      g_object_set_qdata_full (G_OBJECT
(context), context_info_quark, 
pango-context.c:      g_object_set_qdata_full (G_OBJECT
(fontset), cache_quark,
pangox.c:  g_object_set_qdata_full (G_OBJECT (result),

  - And finally, all other caches:

$ grep 'cache.*=' *.c  | grep -v -E '^pangox[-.]' 
pangocairo-fcfont.c:  cffont->glyph_extents_cache =
g_new0 (GlyphExtentsCacheEntry, GLYPH_CACHE_NUM_ENTRIES);
pangocairo-fcfont.c: 
cffont->glyph_extents_cache[0].glyph = 1; /* glyph 1
cannot happen in bucket 0 */
pangocairo-fcfont.c:  if (cffont->glyph_extents_cache ==
NULL)
pangocairo-render.c:static PangoCairoRenderer
*cached_renderer = NULL;
pangocairo-render.c:static GStaticMutex
cached_renderer_mutex = G_STATIC_MUTEX_INIT;
pangocairo-render.c:        cached_renderer = g_object_new
(PANGO_TYPE_CAIRO_RENDERER, NULL);
pangocairo-win32font.c:  face->cached_fonts =
g_slist_prepend (face->cached_fonts, win32font);
pango-context.c: * We cache the results of character,fontset
=> font,shaper in a hash table
pango-context.c:  static GQuark cache_quark = 0;
pango-context.c:    cache_quark = g_quark_from_static_string
("pango-shaper-font-cache");
pango-context.c:  cache = g_object_get_qdata (G_OBJECT
(fontset), cache_quark);
pango-context.c:      cache = g_slice_new (ShaperFontCache);
pango-context.c:      cache->hash = g_hash_table_new_full
(g_direct_hash, NULL,
pango-context.c:  state->cache = NULL;
pango-context.c:      state->cache = NULL;
pango-context.c:      state->cache =
get_shaper_font_cache (state->current_fonts);
pangofc-font.c:  if (G_UNLIKELY
(priv->char_to_glyph_cache == NULL))
pangofc-font.c:      priv->char_to_glyph_cache = g_new0
(GUnicharToGlyphCacheEntry, GLYPH_CACHE_NUM_ENTRIES);
pangofc-font.c:      priv->char_to_glyph_cache[0].ch = 1;
/* char 1 cannot happen in bucket 0 */
pangofc-fontmap.c:  priv->fontset_cache = g_queue_new ();
pangofc-fontmap.c:      patterns->cache_link = NULL;
pangofc-fontmap.c:       patterns->cache_link !=
priv->fontset_cache->head))
pangofc-fontmap.c:  GQueue *cache = priv->fontset_cache;
pangofc-fontmap.c:      if (patterns->cache_link ==
cache->tail)
pangofc-fontmap.c:      cache->tail =
patterns->cache_link->prev;
pangofc-fontmap.c:      cache->head = g_list_remove_link
(cache->head, patterns->cache_link);
pangofc-fontmap.c:      if (cache->length ==
FONTSET_CACHE_SIZE)
pangofc-fontmap.c:        tmp_patterns->cache_link =
NULL;
pangofc-fontmap.c:      patterns->cache_link =
g_list_prepend (NULL, patterns);
pangofc-fontmap.c:  GQueue *cache = priv->fontset_cache;
pangofc-fontmap.c:  cache->head = NULL;
pangofc-fontmap.c:  cache->tail = NULL;
pangofc-fontmap.c:  cache->length = 0;
pangoft2.c:  info->cached_glyph = cached_glyph;
pangoft2.c:  PANGO_FT2_FONT (font)->glyph_cache_destroy =
destroy_notify;
pangoft2-render.c:  add_glyph_to_cache = FALSE;
pangoft2-render.c:      add_glyph_to_cache = TRUE;
pangowin32.c:      cache =
pango_win32_font_map_get_font_cache (win32font->fontmap);
pangowin32.c:  PangoWin32FontCache *cache =
pango_win32_font_map_get_font_cache (win32font->fontmap);
pangowin32-fontcache.c:  g_return_if_fail (cache != NULL);
pangowin32-fontcache.c:  cache = g_slice_new
(PangoWin32FontCache);
pangowin32-fontcache.c:  cache->forward =
g_hash_table_new (logfont_hash, logfont_equal);
pangowin32-fontcache.c:  cache->back = g_hash_table_new
(g_direct_hash, g_direct_equal);
pangowin32-fontcache.c:  cache->mru = NULL;
pangowin32-fontcache.c:  cache->mru_tail = NULL;
pangowin32-fontcache.c:  cache->mru_count = 0;
pangowin32-fontcache.c:  g_return_val_if_fail (cache !=
NULL, NULL);
pangowin32-fontcache.c:       cache->mru_tail =
cache->mru_tail->prev;
pangowin32-fontcache.c:       cache->mru_tail->next =
NULL;
pangowin32-fontcache.c:   cache->mru->prev =
entry->mru;
pangowin32-fontcache.c:   cache->mru = entry->mru;
pangowin32-fontcache.c:      if (cache->mru_count ==
CACHE_SIZE)
pangowin32-fontcache.c:   cache->mru_tail =
cache->mru_tail->prev;
pangowin32-fontcache.c:   cache->mru_tail->next =
NULL;
pangowin32-fontcache.c:      cache->mru = g_list_prepend
(cache->mru, entry);
pangowin32-fontcache.c: cache->mru_tail = cache->mru;
pangowin32-fontcache.c:  g_return_if_fail (cache != NULL);
pangowin32-fontmap.c:  win32fontmap->font_cache =
pango_win32_font_cache_new ();
pangowin32-fontmap.c:  face->cached_fonts =
g_slist_prepend (face->cached_fonts, win32font);
pangowin32-fontmap.c:  win32face->cached_fonts = NULL;
pangowin32-fontmap.c:  face->cached_fonts =
g_slist_remove (face->cached_fonts, font);
pangowin32-fontmap.c:  win32font->in_cache = TRUE;
pangowin32-fontmap.c:  win32font->in_cache = FALSE;

[These don't include the modules.  All greps done in
pango/pango/]

All seem like boring but easy fixes, using a few static
mutexes.  I have
a question though: For platforms that we care about, are int
and pointer
assignments atomic?  Ok, rkaway is telling me that they are
not.  In
that case, even the patch that I committed is not completely
correct.
What do people typically do, what are the idioms?  Other
comments?
Offering hand?

Regards,

-- 
behdad
http://behdad.org/

"Commandment Three says Do Not Kill, Amendment Two says
Blood Will Spill"
        -- Dan Bern, "New American Language"

_______________________________________________
gtk-i18n-list mailing list
gtk-i18n-listgnome.org

http://mail.gnome.org/mailman/listinfo/gtk-i18n-list

Making PangoCairo threadsafe

Thread: Making PangoCairo threadsafe