GNOME Bugzilla – Bug 377539
Make Pango thread-safe
Last modified: 2013-03-15 09:58:56 UTC
Bug for discussion of the upcoming patches to make Pango thread-safe (a bit).
My thoughts on it: http://mail.gnome.org/archives/gtk-i18n-list/2006-November/msg00001.html
For things like loading of modules, registering types, and other things that aren't going to happen a lot of times cumulatively, you can probably get away with a Big Fat Lock.
(In reply to comment #2) > For things like loading of modules, registering types, and other things that > aren't going to happen a lot of times cumulatively, you can probably get away > with a Big Fat Lock. Like, a static mutex that we lock/unlock whenever checking if modules are loaded?
Ok, so we need to types of changes: - For all static variables, use g_once_init/leave(). - For all shared objects (fontmap, ...) use g_object_lock() when that's available.
Created attachment 182488 [details] [review] test case for threading
*** Bug 586986 has been marked as a duplicate of this bug. ***
Created attachment 187932 [details] [review] First partial attampt at fixing the thread safety issue. I'm trying to tackle this issue. As suggested in comment #4 I'm starting to use g_once_init/leave pairs to protect initialization on static data. In the patch I've only worked on default_font_map to get early feedback on this track. Although this solves the race in initialization code, using the default font map on multi threaded code still look fairly dangerous. I'm especially dubious about pango_cairo_context_set_resolution.
(In reply to comment #7) > Created an attachment (id=187932) [details] [review] > First partial attampt at fixing the thread safety issue. > > I'm trying to tackle this issue. Thanks! > As suggested in comment #4 I'm starting to use > g_once_init/leave pairs to protect initialization on static data. In the patch > I've only worked on default_font_map to get early feedback on this track. I believe g_once_init_enter/leave() is designed such that you can call it on the pointer variable direction. Ie, instead of: - if (G_UNLIKELY (!default_font_map)) + if (g_once_init_enter (&default_font_map_initialized)) you just need: - if (G_UNLIKELY (!default_font_map)) + if (g_once_init_enter (&default_font_map)) > Although this solves the race in initialization code, using the default font > map on multi threaded code still look fairly dangerous. We need to add proper locking there, yes. > I'm especially dubious about pango_cairo_context_set_resolution. Why? Context and Layout objects do not need to be threadsafe.
(In reply to comment #8) > As suggested in comment #4 I'm starting to use > > g_once_init/leave pairs to protect initialization on static data. In the patch > > I've only worked on default_font_map to get early feedback on this track. > > I believe g_once_init_enter/leave() is designed such that you can call it on > the pointer variable direction. Ie, instead of: > > - if (G_UNLIKELY (!default_font_map)) > + if (g_once_init_enter (&default_font_map_initialized)) > > you just need: > > - if (G_UNLIKELY (!default_font_map)) > + if (g_once_init_enter (&default_font_map)) I also though about this solution but avoid it as it would require an ugly cast from pointer to gsize and I was not sure about the acceptability of such code. If you think it's ok then we can save a few bytes. > > > I'm especially dubious about pango_cairo_context_set_resolution. > > Why? Context and Layout objects do not need to be threadsafe. Sorry, I meant pango_cairo_font_map_set_resolution
(In reply to comment #9) > (In reply to comment #8) > > As suggested in comment #4 I'm starting to use > > > g_once_init/leave pairs to protect initialization on static data. In the patch > > > I've only worked on default_font_map to get early feedback on this track. > > > > I believe g_once_init_enter/leave() is designed such that you can call it on > > the pointer variable direction. Ie, instead of: > > > > - if (G_UNLIKELY (!default_font_map)) > > + if (g_once_init_enter (&default_font_map_initialized)) > > > > you just need: > > > > - if (G_UNLIKELY (!default_font_map)) > > + if (g_once_init_enter (&default_font_map)) > I also though about this solution but avoid it as it would require an ugly cast > from pointer to gsize and I was not sure about the acceptability of such code. > If you think it's ok then we can save a few bytes. That's exactly why the function takes gsize, and not int. Definitely fine with me. > > > I'm especially dubious about pango_cairo_context_set_resolution. > > > > Why? Context and Layout objects do not need to be threadsafe. > Sorry, I meant pango_cairo_font_map_set_resolution Not worse than the actual font-accessing functions though. g_object_lock() would make it much easier, but then one has to finish that one. Maybe adding a mutex to the fontmap is fine afterall...
I've counted around 90 usage of static data, excluding static const data as that should be safe. I think a few of the 90 can be constified. I'll patch the other initializations to be safe. Moreover proper looking seems to be needed around font config calls, I plan to address that after static data. Is this plan ok?
I don't think we have that many. For everything in modules.c, a single g_once_init_enter/leave() would do. And all the parent_class stuff you can ignore since type registration is threadsafe. I can think of pango-language stuff and default fontmaps, but not much else. What have you got? Also, ignore pangox and pangoxft. Even pangoft2 maybe. For now, in fact, we can focus on pangofc and pangocairo.
Created attachment 188000 [details] static data with locations This is a list of the static data that should probably be made thread safe. pango{x,xft,ft2} stuff is at the end.
(In reply to comment #11) > Is this plan ok? How are you testing this? The code from comment 5 ? Your custom code?
Created attachment 188004 [details] test case I also have a similar test case. Attached. Run like: ./test-pangocairo-threads 100 100 Though the one in comment 5 ensures that all threads start together, so may hit more bugs more easily.
I'm testing with code in comment 5+valgrind. I'm actually trying to understand the code as thread safety issues are not easy to trigger with tests. I'll test with code in comment 15 too. The first two patches I'm attaching should be anyway safe. The first one make some static data more const so that synchronization is clearly not needed. The other one make some static data that is always overwritten as non-static
Created attachment 188005 [details] [review] s/static const char*/static const char* const/g
Created attachment 188006 [details] [review] Remove static keyword from non actually static data
Review of attachment 188006 [details] [review]: ::: pango/pangowin32-fontmap.c @@ -1124,3 @@ charset_name (int charset) { - static char num[10]; You can't do this. The function returns this array, so it can't be stack allocated. Need to restructure code to avoid this. Take the array as an argument for example. @@ -1158,3 @@ ff_name (int ff) { - static char num[10]; Same.
Review of attachment 188006 [details] [review]: In fact, these are only used for debugging, so you can ignore them.
Review of attachment 188005 [details] [review]: I'm committing this, but as I'll discuss in another comment, we should remove the alias stuff to pangowin32 and make it g_once_init'able.
(In reply to comment #19) > Review of attachment 188006 [details] [review]: > > ::: pango/pangowin32-fontmap.c > @@ -1124,3 @@ > charset_name (int charset) > { > - static char num[10]; > > You can't do this. The function returns this array, so it can't be stack > allocated. Need to restructure code to avoid this. Take the array as an > argument for example. > > @@ -1158,3 @@ > ff_name (int ff) > { > - static char num[10]; > > Same. Sorry for this silly mistake. I'll provide a new patch ASAP
Comment on attachment 188000 [details] static data with locations >pango/pango-utils.c:static GHashTable *pango_aliases_ht = NULL; >pango/pango-utils.c:static GHashTable *config_hash = NULL; Lets deprecate the alias APIs and move the implementation to pangowin32 that uses it. Regardless, we need threadsafe hashtables. >pango/pango-utils.c:static HMODULE pango_dll; This one's fine. >pango/pango-utils.c: static const gchar *result = NULL; >pango/pango-utils.c: static const gchar *result = NULL; g_once_init >pango/pango-enum-types.c: static GType etype = 0; >pango/pango-enum-types.c: static GType etype = 0; >pango/pango-enum-types.c: static GType etype = 0; >pango/pango-enum-types.c: static GType etype = 0; >pango/pango-enum-types.c: static GType etype = 0; >pango/pango-enum-types.c: static GType etype = 0; >pango/pango-enum-types.c: static GType etype = 0; >pango/pango-enum-types.c: static GType etype = 0; >pango/pango-enum-types.c: static GType etype = 0; >pango/pango-enum-types.c: static GType etype = 0; >pango/pango-enum-types.c: static GType etype = 0; >pango/pango-enum-types.c: static GType etype = 0; >pango/pango-enum-types.c: static GType etype = 0; >pango/pango-enum-types.c: static GType etype = 0; >pango/pango-enum-types.c: static GType etype = 0; >pango/pango-enum-types.c: static GType etype = 0; >pango/pango-enum-types.c: static GType etype = 0; >pango/pango-enum-types.c: static GType etype = 0; Don't worry about these. That's bug 627241. >pango/pango-ot-ruleset.c: static GQuark rulesets_quark = 0; Depends on bug 627240. >pango/querymodules.c: static GEnumClass *class = NULL; Ignore this. It's not part of the library. >pango/pangofc-font.c: static guint engine_type_id = 0; >pango/pangofc-font.c: static guint render_type_id = 0; These are also quarks, so bug 627240 preferably. Or g_once_init alternatively. >pango/pangocairo-fcfont.c: static GEnumClass *class = NULL; >pango/pangowin32-fontmap.c:static PangoWin32FontMap *default_fontmap = NULL; >pango/pangowin32-fontmap.c: static GType object_type = 0; >pango/pangowin32-fontmap.c: static GType object_type = 0; >pango/pango-engine.c: static PangoEngineShape *fallback_shaper = NULL; >pango/pangoatsui-fontmap.c: static GType object_type = 0; >pango/pango-context.c: static GQuark cache_quark = 0; >pango/pango-context.c: static guint engine_type_id = 0; >pango/pango-context.c: static guint render_type_id = 0; >pango/pango-context.c: static GEnumClass *class = NULL; >pango/pangofc-fontmap.c: static GEnumClass *class = NULL; >pango/pangofc-fontmap.c: static gboolean registered_modules = FALSE; >pango/pangocairo-render.c:static PangoCairoRenderer *cached_renderer = NULL; >pango/pangocairo-context.c:static PangoCairoContextInfo * >pango/pangocairo-context.c: static GQuark context_info_quark; >pango/pango-fontmap.c: static GHashTable *warned_fonts = NULL; >pango/shape.c: static GQuark warned_quark = 0; Mostly variations of g_once_init and quark. >pango/pango-language.c: static PangoLanguage *result = NULL; >pango/pango-language.c: static GHashTable *hash = NULL; >pango/pango-language.c: static gboolean initialized = FALSE; >pango/pango-language.c: static PangoLanguage * const * languages = NULL; >pango/pango-language.c: static GHashTable *hash = NULL; Big fat lock should do. >pango/pango-attributes.c:static GHashTable *name_map = NULL; >pango/pango-attributes.c: static guint current_type = 0x1000000; Big fat lock. >pango/pangocairo-font.c: static GQuark warned_quark = 0; >pango/break.c: static guint engine_type_id = 0; >pango/break.c: static guint render_type_id = 0; Quarks. >pango/pango-ot-buffer.c:static hb_buffer_t *cached_buffer = NULL; This one's locked already. >pango/modules.c:static GList *maps = NULL; >pango/modules.c:static GSList *registered_engines = NULL; >pango/modules.c:static GSList *dlloaded_engines = NULL; >pango/modules.c:static GHashTable *dlloaded_modules; >pango/modules.c: static GQuark warned_quark = 0; >pango/modules.c: static GEnumClass *class = NULL; >pango/modules.c: static gboolean init = FALSE; >pango/modules.c: static gboolean no_module_warning = FALSE; Big fat lock (except for quarks). >pango/pangocairo-fontmap.c:static PangoFontMap *default_font_map = NULL; g_once_init. >pango/pangowin32.c: static guint engine_type_id = 0; >pango/pangowin32.c: static guint render_type_id = 0; Quarks. >pango/pangowin32.c: static guint last = 0; /* Cache of one */ > >pango/pangox.c: static GQuark quark = 0; >pango/pangox.c: static guint engine_type_id = 0; >pango/pangox.c: static guint render_type_id = 0; >pango/pangox-fontmap.c:static GSList *registered_displays; >pango/pangox-fontmap.c:static GList *fontmaps = NULL; >pango/pangox-fontmap.c: static gboolean registered_modules = FALSE; >pango/pangox-fontmap.c:static gboolean error_occurred; Don't touch. I'll actually remove pangox soon. >pango/pangoxft-fontmap.c:static GSList *fontmaps = NULL; g_once_init. >pango/pangoxft-fontmap.c:static GSList *registered_displays; Don't bother. >pango/pangoft2.c: static char *default_msg = NULL; Don't bother. >pango/pangoft2-fontmap.c:static PangoFT2FontMap *pango_ft2_global_fontmap = NULL; g_once_init. Thanks for working on this! So, maybe you can start doing the g_once_init ones, and I'll see if I can get the fixes in glib. Lets do one commit per one logical change. Better if you can push a git tree somewhere so I can review and pull.
Colin, can you look into getting Bug 627240 - add G_DEFINE_QUARK and Bug 627241 - add G_DEFINE_[ENUM|FLAGS]_TYPE in?
I've opened a pango mirror on github: https://github.com/alexp-sssup/pango The less-static branch contains an update fix for the static string in pangowin32-fontmap.c The safe-static branch contains the first patch (detailed below) to make static data thread safe. New patches will be added in that branch (In reply to comment #23) > >pango/pango-utils.c:static GHashTable *pango_aliases_ht = NULL; > >pango/pango-utils.c:static GHashTable *config_hash = NULL; > > Lets deprecate the alias APIs and move the implementation to pangowin32 that > uses it. Regardless, we need threadsafe hashtables. Should I take a look at making hashtables thread safe myself? > > > >pango/pango-utils.c:static HMODULE pango_dll; > > This one's fine. You mean we can leave it alone? > > > >pango/pango-utils.c: static const gchar *result = NULL; > >pango/pango-utils.c: static const gchar *result = NULL; > > g_once_init Those are fixed in the safe-static branch
(In reply to comment #25) > (In reply to comment #23) > > >pango/pango-utils.c:static GHashTable *pango_aliases_ht = NULL; > > >pango/pango-utils.c:static GHashTable *config_hash = NULL; > > > > Lets deprecate the alias APIs and move the implementation to pangowin32 that > > uses it. Regardless, we need threadsafe hashtables. > > Should I take a look at making hashtables thread safe myself? I talked to Ryan Lortie, and also filed Bug 650459 - hash table consistency while calling destroy notify funcs. With that fixed, we can just wrap all our hashtable ops with a recursive-mutex and should be fine. When I said we can't just wrap it with a lock I wasn't thinking about recursive-mutexes. > > >pango/pango-utils.c:static HMODULE pango_dll; > > > > This one's fine. > You mean we can leave it alone? Yes. > > >pango/pango-utils.c: static const gchar *result = NULL; > > >pango/pango-utils.c: static const gchar *result = NULL; > > > > g_once_init > Those are fixed in the safe-static branch Thanks. Will check.
> > >pango/pangocairo-fcfont.c: static GEnumClass *class = NULL; > >pango/pangowin32-fontmap.c:static PangoWin32FontMap *default_fontmap = NULL; > >pango/pangowin32-fontmap.c: static GType object_type = 0; > >pango/pangowin32-fontmap.c: static GType object_type = 0; > >pango/pango-engine.c: static PangoEngineShape *fallback_shaper = NULL; > >pango/pangoatsui-fontmap.c: static GType object_type = 0; > >pango/pango-context.c: static GQuark cache_quark = 0; > >pango/pango-context.c: static guint engine_type_id = 0; > >pango/pango-context.c: static guint render_type_id = 0; > >pango/pango-context.c: static GEnumClass *class = NULL; > >pango/pangofc-fontmap.c: static GEnumClass *class = NULL; > >pango/pangofc-fontmap.c: static gboolean registered_modules = FALSE; > >pango/pangocairo-render.c:static PangoCairoRenderer *cached_renderer = NULL; > >pango/pangocairo-context.c:static PangoCairoContextInfo * > >pango/pangocairo-context.c: static GQuark context_info_quark; > >pango/pango-fontmap.c: static GHashTable *warned_fonts = NULL; > >pango/shape.c: static GQuark warned_quark = 0; > > Mostly variations of g_once_init and quark. I'm waiting news for the quark issue before tackling those > > > >pango/pango-language.c: static PangoLanguage *result = NULL; > >pango/pango-language.c: static GHashTable *hash = NULL; > >pango/pango-language.c: static gboolean initialized = FALSE; > >pango/pango-language.c: static PangoLanguage * const * languages = NULL; > >pango/pango-language.c: static GHashTable *hash = NULL; > > Big fat lock should do. commit 23f7d0f43fb86105e1526cd7016149c179c20b89 solves those. I've used a g_once_enter/leave pair and two mutexes
>>pango/pango-attributes.c:static GHashTable *name_map = NULL; >>pango/pango-attributes.c: static guint current_type = 0x1000000; > >Big fat lock. > > >>pango/modules.c:static GList *maps = NULL; >>pango/modules.c:static GSList *registered_engines = NULL; >>pango/modules.c:static GSList *dlloaded_engines = NULL; >>pango/modules.c:static GHashTable *dlloaded_modules; >>pango/modules.c: static GQuark warned_quark = 0; >>pango/modules.c: static GEnumClass *class = NULL; >>pango/modules.c: static gboolean init = FALSE; >>pango/modules.c: static gboolean no_module_warning = FALSE; > >Big fat lock (except for quarks). > > >>pango/pangocairo-fontmap.c:static PangoFontMap *default_font_map = NULL; > >g_once_init. Those should be also fixed in the github branch. Are the patches ok until now? Should I work to make the quarks safe or wait?
The patch on modules.c caused a deadlock. This should be solved in the updated commit.
In modules.c:295 the g_type_module_use function is used. As it is not thread safe the module may end up being loaded multiple times. Any suggestion on how to handle this?
Humm? We should have a big fat mutex for modules.c. Why wouldn't that do?
Yes, locking around the call should be ok. I'll go down that road
>>pango/pangoxft-fontmap.c:static GSList *fontmaps = NULL; > >g_once_init. >>pango/pangoft2-fontmap.c:static PangoFT2FontMap *pango_ft2_global_fontmap = NULL; > >g_once_init. For both it seems more appropriate to use a mutex, as those are modified around the code. Don't you think?
>pango/pangocairo-fcfont.c: static GEnumClass *class = NULL; >pango/pangowin32-fontmap.c: static GType object_type = 0; >pango/pangowin32-fontmap.c: static GType object_type = 0; >pango/pango-engine.c: static PangoEngineShape *fallback_shaper = NULL; >pango/pangoatsui-fontmap.c: static GType object_type = 0; >pango/pango-context.c: static GEnumClass *class = NULL; >pango/pangofc-fontmap.c: static GEnumClass *class = NULL; >pango/pangofc-fontmap.c: static gboolean registered_modules = FALSE; Fixed those using g_once_enter/leave >pango/pangowin32-fontmap.c:static PangoWin32FontMap *default_fontmap = NULL; It seems that also this one must be handled with a mutex, the variable is also used in pango_win32_shutdown_display >pango/pango-context.c: static GQuark cache_quark = 0; >pango/pango-context.c: static guint engine_type_id = 0; >pango/pango-context.c: static guint render_type_id = 0; >pango/pangocairo-context.c: static GQuark context_info_quark; >pango/shape.c: static GQuark warned_quark = 0; Waiting news about handling quarks >pango/pangocairo-render.c:static PangoCairoRenderer *cached_renderer = NULL; This seems to be already locked >pango/pango-fontmap.c: static GHashTable *warned_fonts = NULL; This will also need a mutex Patches for the fixed issues are available from github. I think all the things fixable using g_once_init are done. It would be really nice to have a review of the current status and a suggestion about what to do next.
(In reply to comment #33) > >>pango/pangoxft-fontmap.c:static GSList *fontmaps = NULL; > > > >g_once_init. You're right about this one. > >>pango/pangoft2-fontmap.c:static PangoFT2FontMap *pango_ft2_global_fontmap = NULL; > > > >g_once_init. This one we initialize once only, right? > For both it seems more appropriate to use a mutex, as those are modified around > the code. Don't you think?
(In reply to comment #34) > >pango/pangocairo-fcfont.c: static GEnumClass *class = NULL; > >pango/pangowin32-fontmap.c: static GType object_type = 0; > >pango/pangowin32-fontmap.c: static GType object_type = 0; > >pango/pango-engine.c: static PangoEngineShape *fallback_shaper = NULL; > >pango/pangoatsui-fontmap.c: static GType object_type = 0; > >pango/pango-context.c: static GEnumClass *class = NULL; > >pango/pangofc-fontmap.c: static GEnumClass *class = NULL; > >pango/pangofc-fontmap.c: static gboolean registered_modules = FALSE; > Fixed those using g_once_enter/leave > > >pango/pangowin32-fontmap.c:static PangoWin32FontMap *default_fontmap = NULL; > It seems that also this one must be handled with a mutex, the variable is also > used in pango_win32_shutdown_display One can argue that if you call shutdown_display() (or similar functions in other backends), it's your responsibility not to use that display/backend anymore. So I wouldn't worry about this one too much. > >pango/pango-context.c: static GQuark cache_quark = 0; > >pango/pango-context.c: static guint engine_type_id = 0; > >pango/pango-context.c: static guint render_type_id = 0; > >pango/pangocairo-context.c: static GQuark context_info_quark; > >pango/shape.c: static GQuark warned_quark = 0; > Waiting news about handling quarks You can go ahead and define a PANGO_DEFINE_QUARK(name) for now, that would define a function called name##_get_quark(), and use that around the code. > >pango/pangocairo-render.c:static PangoCairoRenderer *cached_renderer = NULL; > This seems to be already locked > > >pango/pango-fontmap.c: static GHashTable *warned_fonts = NULL; > This will also need a mutex > > Patches for the fixed issues are available from github. Thanks! > I think all the things fixable using g_once_init are done. It would be really > nice to have a review of the current status and a suggestion about what to do > next. I'll try to do that this week.
> > >>pango/pangoft2-fontmap.c:static PangoFT2FontMap *pango_ft2_global_fontmap = NULL; > > > > > >g_once_init. > > This one we initialize once only, right? > > > >pango/pangowin32-fontmap.c:static PangoWin32FontMap *default_fontmap = NULL; > > It seems that also this one must be handled with a mutex, the variable is also > > used in pango_win32_shutdown_display > > One can argue that if you call shutdown_display() (or similar functions in > other backends), it's your responsibility not to use that display/backend > anymore. So I wouldn't worry about this one too much. Both are now fixed.
>> >>pango/pangoxft-fontmap.c:static GSList *fontmaps = NULL; >> > >> >g_once_init. > >You're right about this one. Fixed this one as well on github
> > >pango/pango-context.c: static GQuark cache_quark = 0; > > >pango/pango-context.c: static guint engine_type_id = 0; > > >pango/pango-context.c: static guint render_type_id = 0; > > >pango/pangocairo-context.c: static GQuark context_info_quark; > > >pango/shape.c: static GQuark warned_quark = 0; > > Waiting news about handling quarks > > You can go ahead and define a PANGO_DEFINE_QUARK(name) for now, that would > define a function called name##_get_quark(), and use that around the code. > Could you please elaborate on this, I'm not sure about what you mean. How would such macro make the code thread safe?
(In reply to comment #39) > > > >pango/pango-context.c: static GQuark cache_quark = 0; > > > >pango/pango-context.c: static guint engine_type_id = 0; > > > >pango/pango-context.c: static guint render_type_id = 0; > > > >pango/pangocairo-context.c: static GQuark context_info_quark; > > > >pango/shape.c: static GQuark warned_quark = 0; > > > Waiting news about handling quarks > > > > You can go ahead and define a PANGO_DEFINE_QUARK(name) for now, that would > > define a function called name##_get_quark(), and use that around the code. > > > Could you please elaborate on this, I'm not sure about what you mean. How would > such macro make the code thread safe? Try something like this: #define PANGO_DEFINE_QUARK(name, str) \ GQuark \ name##_get_quark (void) \ { \ static GQuark quark = 0; \ if (G_UNLIKELY (!quark) \ quark = g_quark_from_static_string (str); \ return quark; \ } I was going to use g_once_init_enter/leave but I noticed that since g_quark_from_static_string is safe and idempotent, and int operations are atomic, this code is threadsafe. No need for more involved stuff. Should go in pango-impl-utils.c for now. Then you can use it to define all your quarks. Or, given that no magic is needed, maybe just leave the quark ones as is?
Leaving them as they are seems ok to me. I think the next step could be protecting FontConfig calls as those seem not safe
I've pushed on github two patches that deals with fontconfig The first one only protect with a mutex the ones using the global FcConfig, as those are at first sight the only unsafe calls. As I still had crashes on the testthreads test I also protected all the other fontconfig calls. I'm not sure if that is right though. I still think the modules code needs to be made safe, moreover I hit some crashes that I think may be related to the safety of hash maps. I tried to make the basic module thread safe. The patch is the last on on github. With all the patches applied I managed to get the testthread example running and it crashes only once in a while. It would be very nice at this point to have a bit of review and guidance. Moreover, I was wondering if the patches, at least the simpler ones dealing with initialization, could be accepted upstream. Please consider that until now multithreaded code was completely unsupported and those should have no effect on single threaded apps.
(In reply to comment #42) > I've pushed on github two patches that deals with fontconfig > > The first one only protect with a mutex the ones using the global FcConfig, as > those are at first sight the only unsafe calls. > > As I still had crashes on the testthreads test I also protected all the other > fontconfig calls. I'm not sure if that is right though. Will take a look. > I still think the modules code needs to be made safe, moreover I hit some > crashes that I think may be related to the safety of hash maps. > > I tried to make the basic module thread safe. The patch is the last on on > github. > > With all the patches applied I managed to get the testthread example running > and it crashes only once in a while. It would be very nice at this point to > have a bit of review and guidance. Moreover, I was wondering if the patches, at > least the simpler ones dealing with initialization, could be accepted upstream. > Please consider that until now multithreaded code was completely unsupported > and those should have no effect on single threaded apps. I don't think the modules need any change. I started reviewing your changes, most look trivial and good to go. The modules.c changes however looked wrong and needed detailed review, so I postponed it. With correct modules.c locking, you don't have to worry about the shaper modules I guess. Will start merging the trivial parts.
Any news?
Didn't get to do yet. Will try tonight.
Can I get on a ping on this too? This would be helpful for the Shell so we can load SVG off of the main thread.
Many of the commits are trivial and can go in. But the nontrivial ones need more work, and so far I haven't got the time to review and fix them :(.
*** Bug 499514 has been marked as a duplicate of this bug. ***
(In reply to comment #23) > (From update of attachment 188000 [details]) > >pango/pango-utils.c:static GHashTable *pango_aliases_ht = NULL; > >pango/pango-utils.c:static GHashTable *config_hash = NULL; > > Lets deprecate the alias APIs and move the implementation to pangowin32 that > uses it. Regardless, we need threadsafe hashtables. > Here's a patch that does this part.
Created attachment 222604 [details] [review] deperecate pango_lookup_aliases
Ack. I'm starting reviewing the patchset.
Created attachment 222606 [details] [review] deal with pango-language.c
I pushed out the trivial bits. Trickier parts remain.
Created attachment 222698 [details] [review] make enum get_type functions thread-safe
To summarize, what's left to do: - Switch to G_DEFINE_QUARK. Small potatoes. - Figure out config reading stuff. Easy. - Figure out module loading stuff. Medium. - Figure out Fontconfig. Perhaps switch to using per PangoFcFontmap FcConfig's. Medium. - Figure out g_object_set_qdata-based thread-safety. Medium, the API for that is being developed. - Figure out general thread-safety, or at a minimum, PangoFc level. Hard. Needs careful examination of all objects involved (PangoFcFontmap, PantoFcFontset, PangoFcFont, ...). I probably will attack them in that order approximately. The good news is, we get more threadsafe as we go, so it's not a black/white decision.
Review of attachment 222698 [details] [review]: .
Created attachment 224324 [details] Crashy logo Inkscape sometimes produces SVGs that are instant crashers for Nautilus. As soon as it tries to render one of these SVGs, it crashes immediately. I was chatting in #gnome-design about this, and it was suggested that I upload the offending SVG to this bug. I'm on Fedora 17 with GNOME 3.4, and Nautilus immediately crashes whenever it attempts to render this graphic, regardless of file name or directory it is contained within. Note: The file selector's thumbnail preview works properly, oddly enough.
Garrett: It doesn't crash here, but it does use text so will render with pango, which may race with svg rendering on the main thread when nautilus renders the thumbnail in a thread.
Created attachment 224375 [details] [review] rework config file reading
Created attachment 224379 [details] [review] make modules.c thread-safe
Created attachment 224381 [details] [review] deal with fontconfig
After these patches, I see failures stemming from cross-thread use of fontmaps
(In reply to comment #59) > Created an attachment (id=224375) [details] [review] > rework config file reading Humm. For some reason this patch doesn't seem to apply. Can you please check? Did you work off something other than master perhaps? And, feel free to push BTW.
Review of attachment 224379 [details] [review]: Same thing here. Doesn't apply for me. Not sure if I'm doing something ridiculously wrong... In the future we can probably make the common case lock-free, but I'm more than happy to take this now. Feel free to just push.
Review of attachment 224381 [details] [review]: This is definitely a good start, but I a bit excessive I think. We have to clearly defined what the requirements are and filter the patch to adhere to those. For example, looks to me like the patch as is would do multiple lockings where the FcPattern being used is either still being constructed and hence not shared, or is immutable. Also, I really need to figure out whether I want to go down this path or just fix Fontconfig instead.
Thanks again Matthias. After we resolve your two first patches, I can kick this for a couple hours (Sunday night) and hopefully we'll have something working. The two major items after those patches of yours are indeed 1) fontconfig, and 2) fontmap. The first one at least we have your patch as fallback, for the second one, it doesn't sound like an awful lot of work to me, but rather tricky. I'll see what I get to do with them.
Oh, they didn't apply because I had forgotten to attach one preliminary config parsing patch. Sorry about that. I've pushed the config parsing and modules.c changes now.
On the fontconfig locking, I really wasn't able to discern if there's any part of the fontconfig api that would be safe to use without locking. I started reading FcPatternGetDouble, and that quickly lead me to FcObjectFromName -> FcObjectFindByName -> FcObjectInsert which is modifying a global hash table...
Fontconfig is fixed now. It's in wip/threadsafety branch, but I expect it to be merged in the next couple of days. Now I see crashes in pangofc's hash tables, which is expected and the last remaining piece of the puzzle. Will get to them.
To update here: I created a branch of about twenty commits, trying to make PangoFcFontMap threadsafe. I failed. After much talk with desrt, I now think we want to declare PangoFontMap NOT threadsafe, and instad make pango_cairo_font_map_get_default() return a thread-private copy. I think that can be made to work quickly, but I have not got to do that.
Ok, good news! Fontconfig is threadsafe in master now. Pango is threadsafe now! See log below. What remains to do: - Wait for fontconfig release and require it, - Enable the test-pangocairo-threads by default, and increase number of threads and iterations. commit dd90b07699fe5bf0ea5dfee7e907662e8ca8280b Author: Behdad Esfahbod <behdad@behdad.org> Date: Thu Jan 3 03:52:09 2013 -0600 Make pango threadsafe! By way of declaring fontmaps NOT threadsafe, and making pango_cairo_font_map_get_default() return a thread-private fontmap. test-pangocairo-threads doesn't crash anymore, if used with fontconfig master. commit 00997204f7df5f4e2f75f8c1c2f72130f302e7c6 Author: Behdad Esfahbod <behdad@behdad.org> Date: Thu Jan 3 03:51:10 2013 -0600 Add test-pangocairo-threads.c Currently is not run automatically. But run it with args "10 100" and see it crash... commit 97b14f5ca62d37311798ecdd39ea4368785a2fa8 Author: Behdad Esfahbod <behdad@behdad.org> Date: Wed Nov 7 10:58:40 2012 -0800 Link to gthread-2.0
And update docs.
Created attachment 238766 [details] [review] Poer PangoWin32 to use GWeakRef Hi, Sorry if I posted in the wrong bug report... I did a little bit of work to port the PangoWin32 code (specifically pangowin32.c and pangowin32-fontmap.c) to use the GWeakRef api, and it does seem to me that it fares better in the test-pangocairo-threads test program (after I changed the snprintf call in it to g_snprintf), as I did manage to have it run without crashing in the process. However, what I get there is a bunch of: (test-pangocairo-threads.exe:4340): Pango-WARNING **: All font fallbacks failed! !!! and the produced .png is different from the reference .png. Yet to investigate why. I guess this might be a head start for thread safety on PangoWin32, so this is what I have now. By the way, I was looking at Behdad's bug report on making pangowin32-fontmap use G_DEFINE_TYPE-can someone elaborate what work needs to be done there? Hopefully I can look into that as well ASAP. With blessings, thank you!
(In reply to comment #73) > Created an attachment (id=238766) [details] [review] > Poer PangoWin32 to use GWeakRef > > Hi, > > Sorry if I posted in the wrong bug report... > > I did a little bit of work to port the PangoWin32 code (specifically > pangowin32.c and pangowin32-fontmap.c) to use the GWeakRef api, and it does > seem to me that it fares better in the test-pangocairo-threads test program > (after I changed the snprintf call in it to g_snprintf), as I did manage to > have it run without crashing in the process. Great! Please push the g_snprintf() change out. > However, what I get there is a bunch of: > > (test-pangocairo-threads.exe:4340): Pango-WARNING **: All font fallbacks > failed! > !!! Did your patch from the other bug fix that? > and the produced .png is different from the reference .png. Yet to investigate > why. Keep us posted. Please file new bugs. > I guess this might be a head start for thread safety on PangoWin32, so this is > what I have now. That's great. Thanks! > By the way, I was looking at Behdad's bug report on making pangowin32-fontmap > use G_DEFINE_TYPE-can someone elaborate what work needs to be done there? > Hopefully I can look into that as well ASAP. > > With blessings, thank you!
Marking this fixed. Please file new bugs for remaining issues.
Comment on attachment 238766 [details] [review] Poer PangoWin32 to use GWeakRef I committed these.