Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix pango fontmap threading issue. #601

Merged
merged 1 commit into from
Apr 1, 2015

Conversation

JoakimSoderberg
Copy link
Contributor

Since pango v1.32.6 pango_cairo_font_map_get_default returns a per thread fontmap. But since we're allocating this statically on the first call to rrd_init,
if we then try to access it again via another thread glib will get stuck in an infinte assertion loop.

Instead use pango_cairo_font_map_new() when creating the fontmap. This will not create a per thread version and glib will be happy.

Also added some extra locking whenever using the im->layout pango structure that uses the fontmap internally as a precaution.

This fixes #600

Since pango v1.32.6 `pango_cairo_font_map_get_default` returns a per thread `fontmap`. But since we're allocating this statically on the first call to `rrd_init`,
if we then try to access it again via another thread glib will get stuck in an infinte assertion loop.

Instead use `pango_cairo_font_map_new()` when creating the `fontmap`. This will not create a per thread version and glib will be happy.

Also added some extra locking whenever using the `im->layout` pango structure that uses the `fontmap` internally as a precaution.
@oetiker
Copy link
Owner

oetiker commented Apr 1, 2015

  • what is performance impact of this approach in the single threaded case ?
  • at the moment librrdtool comes in two flavours, threaded and non threaded ... the major stumbling block apart from the one you just tackled is our use of getopt ... using popt might be an option for getting rid of this.
  • please make sure the platform support the mutex lock by adding appropriate tests to configure.ac

@JoakimSoderberg
Copy link
Contributor Author

Do you have any benchmarking script to check this, or do I have to roll my own?

I mean, I only added the locking because you mentioned it. I have not seen any issues related to it. I guess the fontmap isn't written to but simply read from by multiple threads? However I'm not familiar with all the glib reference counting, I guess that might create issues?

I'm not sure what you mean with the configure.ac comment. I simply used the built-in mutex_t wrapper. It is used in other places in the code. So what type of check is needed that isn't already done?

Regarding popt you could also checkout the command line parsing lib I've written if you're interested (https://github.com/JoakimSoderberg/cargo).

Replacing the entire use of getopt is a bit more than I will be able to do. This is basically a 1 line fix for an issue that causes a complete crash. The locking is just an extra precaution.

@oetiker
Copy link
Owner

oetiker commented Apr 1, 2015

the locking was just a guess ... no idea if it is necessary ... if not, all the better ...

about the multithreading thing ... thats from back in the day when not all platforms supported pthreads ... but I just realized that rrd_client.c is using mutex_t as well and it is included in the non-threaded version of rrdtool. So forget what I said about configure

I have no benchmarking code as such ... when I initially came up with this fontmap thing, I just create a loop to create a bunch of charts ...

And cargo ... didn't know about that one ...

oetiker added a commit that referenced this pull request Apr 1, 2015
@oetiker oetiker merged commit d95016e into oetiker:master Apr 1, 2015
@JoakimSoderberg
Copy link
Contributor Author

Thanks for merging :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Fails to graph with threaded calls
2 participants