705691 - Build cairo with the MUTEX option

Status: RESOLVED FIXED

(Core :: Graphics, defect)

Description

[Benoit Girard (:BenWa)]

Jeff doesn't expect the cairo option to cause a significant performance regression and is needed because of the shared buffers used by cairo. We need this option for off-main thread composting since we will be using cairo from the gecko and the compositing thread.

Comment 1

[Benoit Girard (:BenWa)]

Attachment: patch

Comment 2

[Benoit Girard (:BenWa)]

Attachment: patch

Comment 3

[Mozilla RelEng Bot]

Try run for e9d853679e7a is complete.
Detailed breakdown of the results available here:
    https://tbpl.mozilla.org/?tree=Try&rev=e9d853679e7a
Results (out of 142 total builds):
    exception: 13
    success: 100
    warnings: 21
    failure: 8
Builds available at http://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/[e-mail]-e9d853679e7a

Comment 4

[Mozilla RelEng Bot]

Try run for 60da465f2f5e is complete.
Detailed breakdown of the results available here:
    https://tbpl.mozilla.org/?tree=Try&rev=60da465f2f5e
Results (out of 27 total builds):
    success: 24
    warnings: 2
    failure: 1
Builds available at http://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/[e-mail]-60da465f2f5e

Comment 5

[Benoit Girard (:BenWa)]

Attachment: patch

This patch works everywhere except on win32 debug.

I'm torn between enabling this everywhere and getting this in ASAP for osx/android.

Comment 6

[Joe Drew (not getting mail)]

We're not using basic layers for Fennec.

Comment 7

[Mozilla RelEng Bot]

Try run for d585af23ebd3 is complete.
Detailed breakdown of the results available here:
    https://tbpl.mozilla.org/?tree=Try&rev=d585af23ebd3
Results (out of 117 total builds):
    exception: 2
    success: 90
    warnings: 25
Builds (or logs if builds failed) available at:
http://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/[e-mail]-d585af23ebd3

Comment 8

[Marco Castelluccio [:marco]]

Comment on attachment 577691 [details] [diff] [review]
patch

https://tbpl.mozilla.org/?tree=Try&rev=2b8de579770a

Comment 9

[Benoit Girard (:BenWa)]

Excellent, I'll land this once inbound clears up.

Comment 10

[Benoit Girard (:BenWa)]

Comment on attachment 577691 [details] [diff] [review]
patch

The fixes marco landed make this patch work.

Comment 11

[Benoit Girard (:BenWa)]

Comment on attachment 577691 [details] [diff] [review]
patch

Taking of check-in, please re-request once the patch is r+.

Comment 13

[Oleg Romashin (:romaxa)]

Comment on attachment 577691 [details] [diff] [review]
patch

so can we push this fix?

Comment 14

[Jeff Muizelaar [:jrmuizel]]

It's not clear to me what we're doing this for yet.

Comment 15

[Oleg Romashin (:romaxa)]

see bug 832794, I catched reproducible case when I do create gfxContext in one thread, and destroy context in another thread, context refcount in first thread decreased by context destroy in another thread... (compositor and gecko threads OMTC case)

Comment 16

[Jeff Muizelaar [:jrmuizel]]

(In reply to Oleg Romashin (:romaxa) from comment #15)
> see bug 832794, I catched reproducible case when I do create gfxContext in
> one thread, and destroy context in another thread, context refcount in first
> thread decreased by context destroy in another thread... (compositor and
> gecko threads OMTC case)

It's certainly true that cairo is not thread safe. However, none of the configurations that we ship in require using cairo from multiple threads. I'm fine with making this a build option, but I don't think we should do it unconditionally until we need it.

Comment 17

[Benoit Girard (:BenWa)]

(In reply to Oleg Romashin (:romaxa) from comment #15)
> see bug 832794, I catched reproducible case when I do create gfxContext in
> one thread, and destroy context in another thread, context refcount in first
> thread decreased by context destroy in another thread... (compositor and
> gecko threads OMTC case)

What's your use case for having a gfxContext between threads? We don't have this in m-c AFAIK so it seems silly to take this perf hit for something we don't do.

Comment 18

[Oleg Romashin (:romaxa)]

I have it with embedding environment... I have possibility to run it with SW rendering, and I do create gfxContext from bitmap image source in CompositorParent thread.

Comment 19

[Benoit Girard (:BenWa)]

That sounds reasonable. Keep in mind that adding software based compositing is going to get much easier once the graphics branch merges with central carrying refactoring for how we deal with layers and compositing.

Comment 20

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

Jeff, Would it be okay to do this now? This is making it hard to test e10s on mozilla-central, which is a big goal of ours.

Comment 21

[Jeff Muizelaar [:jrmuizel]]

(In reply to Bill McCloskey (:billm) from comment #20)
Yes. That seems fine.

Comment 22

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

(In reply to Jeff Muizelaar [:jrmuizel] from comment #21)
> (In reply to Bill McCloskey (:billm) from comment #20)
> Yes. That seems fine.

Can you review then?

Comment 23

[Matt Woodrow (:mattwoodrow)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/5495c78f77ea

Comment 24

[Phil Ringnalda (:philor)]

Backed out in http://hg.mozilla.org/integration/mozilla-inbound/rev/6b789dbea375 to see if this is really what caused us to go from zero timeouts in ten rounds of linux32 crashtest-ipc on the push before to between one in three and two in three on this and subsequent pushes.

Comment 25

[Phil Ringnalda (:philor)]

Apparently it wasn't this, still timing out after backing it out.

Comment 26

[Phil Ringnalda (:philor)]

Relanded in https://hg.mozilla.org/integration/mozilla-inbound/rev/b4f68206c912

Comment 27

[Phil Ringnalda (:philor)]

https://hg.mozilla.org/mozilla-central/rev/b4f68206c912