Re: Making PangoCairo threadsafe

[Behdad Esfahbod <behdad behdad org>]

Filed a bug for tracking this:

  http://bugzilla.gnome.org/show_bug.cgi?id=377539

On Mon, 2006-11-20 at 17:05 -0500, Behdad Esfahbod wrote:
> 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"