Closed Bug 882173 Opened 7 years ago Closed 7 years ago

pixman isn't threadsafe

Categories

(Core :: Graphics, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla24

People

(Reporter: joe, Assigned: joe)

References

Details

Attachments

(3 files, 2 obsolete files)

We compile pixman without TLS support, which means our pixman isn't threadsafe.
OS: Mac OS X → All
Hardware: x86 → All
Attachment #761551 - Flags: review?(jmuizelaar)
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
Attachment #761551 - Flags: review?(jmuizelaar) → review-
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.
It's my understanding this has potential (negative) performance implications.  Can we measure this?
Yep - I plan to do so via Talos.
Attachment #761551 - Attachment is obsolete: true
Attachment #761625 - Flags: review?(gps)
Attachment #761627 - Flags: review?(jmuizelaar)
Blocks: 705691
No longer blocks: 705692
Attached file tls timing test (obsolete) —
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
(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.
Attached file tls timing test
Doing a more meaningful test with -O2 shows

tls: 5085
declspec: 0
raw: 0
Attachment #762042 - Attachment is obsolete: true
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 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.
Attachment #761627 - Flags: review?(jmuizelaar) → review+
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.
Attachment #761625 - Flags: review?(gps) → review+
I don't use the AC_DEFINE variable! I'll remove it.
Just for the record : OpenBSD has pthreads, but has no pthread local storage support. That will break us badly, i'm afraid.
Landry also tells me that, not only does OpenBSD not have pthread TLS support, it has *no* TLS support, which is bad times. :(
https://hg.mozilla.org/mozilla-central/rev/19671f660736
https://hg.mozilla.org/mozilla-central/rev/addf66960e85
Status: NEW → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla24
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.
You need to log in before you can comment on or make changes to this bug.