882173 - pixman isn't threadsafe

Status: RESOLVED FIXED

(Core :: Graphics, defect)

Description

[Joe Drew (not getting mail)]

We compile pixman without TLS support, which means our pixman isn't threadsafe.

Comment 1

[Joe Drew (not getting mail)]

Attachment: support multiple threads for pixman

Comment 2

[Jeff Muizelaar [:jrmuizel]]

Comment on attachment 761551 [details] [diff] [review]
support multiple threads for pixman

Review of attachment 761551 [details] [diff] [review]:
-----------------------------------------------------------------

::: configure.in
@@ +3274,5 @@
>  			fi
>  			;;
>  	esac
>      LDFLAGS="${_PTHREAD_LDFLAGS} ${LDFLAGS}"
> +    AC_DEFINE(HAVE_PTHREADS)

agal won't like you changing configure. Maybe you should make the makefile do the right thing.

::: gfx/cairo/libpixman/src/pixman-compiler.h
@@ -114,1 @@
>  

The pixman part needs to be upstreamable

Comment 3

[Benoit Girard (:BenWa)]

FWIW I don't like Makefile specific defines. It makes it much harder to get tools to work since we can't point it a mozilla-config.h or to the general -D list.

Comment 4

[Milan Sreckovic [:milan] (needinfo for best results)]

It's my understanding this has potential (negative) performance implications.  Can we measure this?

Comment 5

[Joe Drew (not getting mail)]

Yep - I plan to do so via Talos.

Comment 6

[Joe Drew (not getting mail)]

Attachment: define and subst MOZ_USE_PTHREADS when we want to use pthreads

Comment 7

[Joe Drew (not getting mail)]

Attachment: turn on TLS for pixman

Comment 8

[Joe Drew (not getting mail)]

Attachment: tls timing test

This is a test program I created to compare the performance of raw types, __declspec(thread) and TlsGetValue(). It shows that performance, as expected, goes roughly in that order, too.

Units are "whatever QueryPerformanceCounter returns".

tls: 3621
declspec: 1009
raw: 872

Comment 9

[Jeff Muizelaar [:jrmuizel]]

(In reply to Joe Drew (:JOEDREW! \o/) from comment #8)
> Created attachment 762042 [details]
> tls timing test
> 
> This is a test program I created to compare the performance of raw types,
> __declspec(thread) and TlsGetValue(). It shows that performance, as
> expected, goes roughly in that order, too.
> 
> Units are "whatever QueryPerformanceCounter returns".
> 
> tls: 3621
> declspec: 1009
> raw: 872

An additional interesting thing to time would be calls to pixman_image_composite to get an idea of how much difference the 2800 units makes.

Comment 10

[Joe Drew (not getting mail)]

Attachment: tls timing test

Doing a more meaningful test with -O2 shows

tls: 5085
declspec: 0
raw: 0

Comment 11

[Joe Drew (not getting mail)]

Composite and fill time tends to be from single digit all the way up to >= 3000, in whatever unit this is. Which is pretty good considering those 2800 unit difference are in 100,000 call increments, so the lowest composite time I saw (4 units) would mean that we're 0.7% slower.

Comment 12

[Jeff Muizelaar [:jrmuizel]]

Comment on attachment 761627 [details] [diff] [review]
turn on TLS for pixman

I'd use DLL instead of SOLIB, but otherwise this seems fine. Make sure you add a patch to the cairo dir.

Comment 13

[Gregory Szorc [:gps]]

Comment on attachment 761625 [details] [diff] [review]
define and subst MOZ_USE_PTHREADS when we want to use pthreads

Review of attachment 761625 [details] [diff] [review]:
-----------------------------------------------------------------

::: configure.in
@@ +3275,5 @@
>  			;;
>  	esac
>      LDFLAGS="${_PTHREAD_LDFLAGS} ${LDFLAGS}"
> +    AC_DEFINE(MOZ_USE_PTHREADS)
> +    AC_SUBST(MOZ_USE_PTHREADS)

Do you actually use the AC_DEFINE variable? AC_SUBST is what Makefile and moz.build use. Generally speaking, you should only define where usage is needed.

Comment 14

[Joe Drew (not getting mail)]

I don't use the AC_DEFINE variable! I'll remove it.

Comment 15

[Joe Drew (not getting mail)]

remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/19671f660736
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/addf66960e85

Comment 16

[Landry Breuil (:gaston)]

Just for the record : OpenBSD has pthreads, but has no pthread local storage support. That will break us badly, i'm afraid.

Comment 17

[Joe Drew (not getting mail)]

Landry also tells me that, not only does OpenBSD not have pthread TLS support, it has *no* TLS support, which is bad times. :(

Comment 18

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/19671f660736
https://hg.mozilla.org/mozilla-central/rev/addf66960e85

Comment 19

[Landry Breuil (:gaston)]

Ok, it seems it was a false alarm. Tree with that commit still builds on OpenBSD, because apparently pixman already has a fallback on OpenBSD, using pthread_setspecific instead of TLS, and we have pthread_setspecific() (dunno if that's part of what is called TLS support...)

That code was added in http://cgit.freedesktop.org/pixman/commit/pixman/pixman-compiler.h?id=7c9f121efe7ee6afafad8b294974f5498054559b - so as long as you dont try to use __thread directly, we should be fine.