Making PangoCairo threadsafe

[Behdad Esfahbod <behdad behdad org>]

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=76925&action=view

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

  http://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:

  https://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"