Closed Bug 705691 Opened 10 years ago Closed 8 years ago

Build cairo with the MUTEX option

Categories

(Core :: Graphics, defect)

x86
macOS
defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla25

People

(Reporter: BenWa, Unassigned)

References

Details

Attachments

(1 file, 2 obsolete files)

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.
Summary: Build cairo with the multi-thread option → Build cairo with the MUTEX option
Attached patch patch (obsolete) — Splinter Review
Assignee: nobody → bgirard
Status: NEW → ASSIGNED
Attached patch patch (obsolete) — Splinter Review
Attachment #577283 - Attachment is obsolete: true
Blocks: 703484
No longer blocks: omtc
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/b56girard@gmail.com-e9d853679e7a
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/b56girard@gmail.com-60da465f2f5e
Attached patch patchSplinter Review
This patch works everywhere except on win32 debug.

I'm torn between enabling this everywhere and getting this in ASAP for osx/android.
Attachment #577284 - Attachment is obsolete: true
Attachment #577691 - Flags: review?(jmuizelaar)
Attachment #577691 - Flags: review?(jmuizelaar)
We're not using basic layers for Fennec.
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/b56girard@gmail.com-d585af23ebd3
Depends on: 754329
Excellent, I'll land this once inbound clears up.
Comment on attachment 577691 [details] [diff] [review]
patch

The fixes marco landed make this patch work.
Attachment #577691 - Flags: review?(jmuizelaar)
Comment on attachment 577691 [details] [diff] [review]
patch

Taking of check-in, please re-request once the patch is r+.
Attachment #577691 - Flags: checkin?(bgirard)
Attachment #577691 - Flags: review?(jmuizelaar)
Duplicate of this bug: 832794
Comment on attachment 577691 [details] [diff] [review]
patch

so can we push this fix?
Attachment #577691 - Flags: review?(joe)
It's not clear to me what we're doing this for yet.
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)
(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.
Attachment #577691 - Flags: review?(joe) → review?(jmuizelaar)
(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.
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.
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.
Assignee: bgirard → nobody
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.
Flags: needinfo?(jmuizelaar)
(In reply to Bill McCloskey (:billm) from comment #20)
Yes. That seems fine.
Flags: needinfo?(jmuizelaar)
(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?
Attachment #577691 - Flags: review?(jmuizelaar) → review+
Depends on: 882173
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.
Apparently it wasn't this, still timing out after backing it out.
https://hg.mozilla.org/mozilla-central/rev/b4f68206c912
Status: ASSIGNED → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla25
You need to log in before you can comment on or make changes to this bug.