[Azure] Cairo performance slow on Linux

RESOLVED FIXED in mozilla18

Status

()

Core
Canvas: 2D
RESOLVED FIXED
5 years ago
5 years ago

People

(Reporter: kentuckyfriedtakahe, Assigned: kentuckyfriedtakahe)

Tracking

Trunk
mozilla18
All
Linux
Points:
---
Dependency tree / graph
Bug Flags:
in-testsuite -

Firefox Tracking Flags

(Not tracked)

Details

(URL)

Attachments

(7 attachments, 8 obsolete attachments)

2.02 KB, patch
bas
: review+
Details | Diff | Splinter Review
3.29 KB, patch
jrmuizel
: review+
Details | Diff | Splinter Review
1.32 KB, patch
roc
: review+
Details | Diff | Splinter Review
6.95 KB, patch
roc
: review+
Details | Diff | Splinter Review
6.44 KB, patch
roc
: review+
Details | Diff | Splinter Review
5.64 KB, patch
roc
: review+
Details | Diff | Splinter Review
1.61 KB, patch
mattwoodrow
: review+
Details | Diff | Splinter Review
Canvas DrawImage performance 40 times slower on Azure Cairo compared to Thebes.
Created attachment 650773 [details] [diff] [review]
Fixed gfxPlatform::GetSourceSurfaceForSurface() to avoid converting from xlib to buffered image.
Attachment #650773 - Flags: review?(matt.woodrow)
Attachment #650773 - Flags: review?(matt.woodrow) → review+
Depends on: 591358
Blocks: 784582
Blocks: 773460
Created attachment 654074 [details] [diff] [review]
Part 1: Remove xlib to buffered image conversion [v2]
Attachment #650773 - Attachment is obsolete: true
Attachment #654074 - Flags: review?(bas.schouten)
Created attachment 654075 [details] [diff] [review]
Part 2: Pass text bounds to AdjustedTarget
Attachment #654075 - Flags: review?(bas.schouten)
Created attachment 654076 [details] [diff] [review]
Part 3: Shadow code clean up
Attachment #654076 - Flags: review?(jmuizelaar)
Created attachment 654078 [details] [diff] [review]
Part 4: Removed double padding with AlphaBoxBlur
Attachment #654078 - Flags: review?(joe)
Created attachment 654080 [details] [diff] [review]
Part 5: Avoid readback by teeing the drawing operations
Attachment #654080 - Flags: review?(jmuizelaar)
Created attachment 654081 [details] [diff] [review]
Part 6: Match thebes shadow image buffer size
Attachment #654081 - Flags: review?(bas.schouten)
Created attachment 654082 [details] [diff] [review]
Part 7: Keep unblurred patches in xlib
Attachment #654082 - Flags: review?(joe)
Comment on attachment 654074 [details] [diff] [review]
Part 1: Remove xlib to buffered image conversion [v2]

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

::: gfx/thebes/gfxPlatform.cpp
@@ +545,5 @@
> +    surf.mSurface = aSurface->CairoSurface();
> +    srcBuffer = aTarget->CreateSourceSurfaceFromNativeSurface(surf);
> +
> +    // It's cheap enough to make a new one so we won't keep it around and
> +    // keeping it creates a cycle.

Hrm, I'm not sure how I feel about this, but it's ok for now. I'm sure we can work around the cycle if we need to.
Attachment #654074 - Flags: review?(bas.schouten) → review+
Attachment #654075 - Flags: review?(bas.schouten) → review+

Comment 10

5 years ago
Try run for 6f1862f4a9e4 is complete.
Detailed breakdown of the results available here:
    https://tbpl.mozilla.org/?tree=Try&rev=6f1862f4a9e4
Results (out of 269 total builds):
    exception: 39
    success: 65
    warnings: 44
    failure: 121
Builds (or logs if builds failed) available at:
http://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/ajones@mozilla.com-6f1862f4a9e4

Comment 11

5 years ago
Try run for 08f39d31ccbb is complete.
Detailed breakdown of the results available here:
    https://tbpl.mozilla.org/?tree=Try&rev=08f39d31ccbb
Results (out of 66 total builds):
    exception: 14
    success: 5
    failure: 47
Builds (or logs if builds failed) available at:
http://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/ajones@mozilla.com-08f39d31ccbb

Comment 12

5 years ago
Try run for 94f4e85b0cb1 is complete.
Detailed breakdown of the results available here:
    https://tbpl.mozilla.org/?tree=Try&rev=94f4e85b0cb1
Results (out of 264 total builds):
    exception: 12
    success: 48
    warnings: 23
    failure: 181
Builds (or logs if builds failed) available at:
http://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/ajones@mozilla.com-94f4e85b0cb1
Attachment #654076 - Flags: review?(jmuizelaar) → review+
(In reply to Anthony Jones (:kentuckyfriedtakahe) from comment #6)
> Created attachment 654080 [details] [diff] [review]
> Part 5: Avoid readback by teeing the drawing operations

Do you have numbers for how much of a performance difference this makes?
[:jrmuizel] from comment #13)
> Do you have numbers for how much of a performance difference this makes?

AsteroidsBench without patch 1557 frames
AsteroidsBench with patch    2118 frames
Created attachment 654919 [details] [diff] [review]
Part 8: Fixed empty shadows
Attachment #654919 - Flags: review?(bas.schouten)
(In reply to Anthony Jones (:kentuckyfriedtakahe) from comment #14)
> [:jrmuizel] from comment #13)
> > Do you have numbers for how much of a performance difference this makes?
> 
> AsteroidsBench without patch 1557 frames
> AsteroidsBench with patch    2118 frames

What were the pre-azure numbers?
These were Azure numbers before and after the tee patch. On android the difference is 266 before the tee and 274 after the tee. That is for that individual patch.
Comment on attachment 654919 [details] [diff] [review]
Part 8: Fixed empty shadows

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

::: content/canvas/src/nsCanvasRenderingContext2DAzure.cpp
@@ +319,5 @@
>      transform._32 -= mTempRect.y;
>        
>      mTarget =
>        mCtx->mTarget->CreateShadowDrawTarget(IntSize(int32_t(mTempRect.width), int32_t(mTempRect.height)),
> +                                               FORMAT_B8G8R8A8, mSigma);

Indent here is still off it seems.
Attachment #654919 - Flags: review?(bas.schouten) → review+
I must have got too comfortable using (In reply to Bas Schouten (:bas) from comment #18)
> Indent here is still off it seems.

I must have got too comfortable using a formatter :-)
Attachment #654080 - Flags: review?(roc)
Comment on attachment 654080 [details] [diff] [review]
Part 5: Avoid readback by teeing the drawing operations

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

::: gfx/2d/DrawTargetCairo.cpp
@@ +290,5 @@
>    if (mSurface) {
> +    // If we're using a tee surface then we need to destroy all the children
> +    if (cairo_surface_get_type(mSurface) == CAIRO_SURFACE_TYPE_TEE) {
> +      for (int i = 0; ; i++) {
> +        cairo_surface_t* child = cairo_tee_surface_index(mSurface, i);

Why do you need to do this? Normally a cairo_tee_surface keeps strong references to its children and releases them when it's destroyed.

@@ +405,2 @@
>   
> +  if (cairo_surface_get_type(blursurf) == CAIRO_SURFACE_TYPE_IMAGE) {

This confusing. How could this be false? If it is false, then we won't draw the blurred shadow, which would be wrong. Maybe this should be an assertion instead?

@@ +799,5 @@
> +                                                         aSize.width,
> +                                                         aSize.height);
> +
> +  if (cairo_surface_status(similar) || cairo_surface_status(blursurf)) {
> +    return nullptr;

You should destroy similar/blursurf here, otherwise this could leak.
Created attachment 655467 [details] [diff] [review]
Part 9: Clean up code based on review feedback
Attachment #655467 - Flags: review?(roc)
Attachment #654082 - Flags: review?(joe) → review?(roc)
Attachment #654078 - Flags: review?(joe) → review?(roc)
Attachment #654078 - Flags: review?(roc) → review+
Comment on attachment 654082 [details] [diff] [review]
Part 7: Keep unblurred patches in xlib

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

::: gfx/2d/2D.h
@@ +727,5 @@
>    virtual TemporaryRef<DrawTarget>
>      CreateSimilarDrawTarget(const IntSize &aSize, SurfaceFormat aFormat) const = 0;
>  
>    /*
>     * Create a draw target optimized for drawing a shadow.

Note that aSigma is the blur radius that must be used when we draw the shadow. Also note that this doesn't affect the size of the allocated surface, the caller is still responsible for including the shadow area in its size.
Attachment #654082 - Flags: review?(roc) → review+
Actually, you should fold part 9 in part 5 after all.
Created attachment 655494 [details] [diff] [review]
Part 5: Avoid readback by teeing the drawing operations v2
Attachment #655494 - Flags: review?(roc)
Created attachment 655495 [details] [diff] [review]
Part 7: Keep unblurred patches in xlib v2
Attachment #654080 - Attachment is obsolete: true
Attachment #654082 - Attachment is obsolete: true
Attachment #655467 - Attachment is obsolete: true
Attachment #654080 - Flags: review?(roc)
Attachment #654080 - Flags: review?(jmuizelaar)
Attachment #655467 - Flags: review?(roc)
Attachment #655495 - Flags: review?(roc)
Attachment #655494 - Flags: review?(roc) → review+
Attachment #655495 - Flags: review?(roc) → review+
Attachment #654081 - Flags: review?(bas.schouten) → review+
Whiteboard: checkin-needed

Comment 26

5 years ago
Try run for a5ee9b2d1a6e is complete.
Detailed breakdown of the results available here:
    https://tbpl.mozilla.org/?tree=Try&rev=a5ee9b2d1a6e
Results (out of 18 total builds):
    failure: 18
Builds (or logs if builds failed) available at:
http://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/ajones@mozilla.com-a5ee9b2d1a6e
https://tbpl.mozilla.org/?tree=Try&rev=f4c7e27c718e
Keywords: checkin-needed
Whiteboard: checkin-needed

Updated

5 years ago
Assignee: nobody → ajones
https://hg.mozilla.org/integration/mozilla-inbound/rev/f3fa6d8742ff
https://hg.mozilla.org/integration/mozilla-inbound/rev/7402befeff1c
https://hg.mozilla.org/integration/mozilla-inbound/rev/ddff99db5f7c
https://hg.mozilla.org/integration/mozilla-inbound/rev/4041007e49cd
https://hg.mozilla.org/integration/mozilla-inbound/rev/aec0729913fe
https://hg.mozilla.org/integration/mozilla-inbound/rev/3b2797ed0518
https://hg.mozilla.org/integration/mozilla-inbound/rev/8ac3a7df2b6a
https://hg.mozilla.org/integration/mozilla-inbound/rev/dfdfdf5ae236
Keywords: checkin-needed
Created attachment 655550 [details] [diff] [review]
Part 9: Fixed failed assertion in reftest test_canvas.html
Attachment #655550 - Flags: review?(chris.double)

Comment 30

5 years ago
Comment on attachment 655550 [details] [diff] [review]
Part 9: Fixed failed assertion in reftest test_canvas.html

Reviewing fix for broken code to land to avoid having to back everything out.
Attachment #655550 - Flags: review?(chris.double) → review+

Comment 31

5 years ago
https://hg.mozilla.org/integration/mozilla-inbound/rev/cd5eb6f6f495
Backed out

https://hg.mozilla.org/integration/mozilla-inbound/rev/a8f183cf4b38

Comment 33

5 years ago
Try run for f4c7e27c718e is complete.
Detailed breakdown of the results available here:
    https://tbpl.mozilla.org/?tree=Try&rev=f4c7e27c718e
Results (out of 265 total builds):
    exception: 38
    success: 155
    warnings: 27
    failure: 45
Builds (or logs if builds failed) available at:
http://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/ajones@mozilla.com-f4c7e27c718e

Updated

5 years ago
Attachment #655550 - Attachment description: Part 8: Fixed failed assertion in reftest test_canvas.html → Part 9: Fixed failed assertion in reftest test_canvas.html
Status: NEW → ASSIGNED
Created attachment 655844 [details] [diff] [review]
Part 4: Removed double padding with AlphaBoxBlur v2
Attachment #654078 - Attachment is obsolete: true
Attachment #654919 - Attachment is obsolete: true
Attachment #655550 - Attachment is obsolete: true
https://tbpl.mozilla.org/?tree=Try&rev=207b3e44752d

The failing reftest in this try push is part of the yet-to-be-committed bug 784573.
Comment on attachment 655844 [details] [diff] [review]
Part 4: Removed double padding with AlphaBoxBlur v2

Changed int8_t aStride to int32_t aStride.
Attachment #655844 - Flags: review?(roc)
Attachment #655844 - Flags: review?(roc) → review+
Keywords: checkin-needed
(In reply to Anthony Jones (:kentuckyfriedtakahe) from comment #35)
> https://tbpl.mozilla.org/?tree=Try&rev=207b3e44752d

Taking your word for it about the reftest. Green on Try.

https://hg.mozilla.org/integration/mozilla-inbound/rev/503884273a3a
https://hg.mozilla.org/integration/mozilla-inbound/rev/dc33092d9cd8
https://hg.mozilla.org/integration/mozilla-inbound/rev/1099c9f0962d
https://hg.mozilla.org/integration/mozilla-inbound/rev/682a61f34c81
https://hg.mozilla.org/integration/mozilla-inbound/rev/fc26333c7525
https://hg.mozilla.org/integration/mozilla-inbound/rev/e823337a4f78
https://hg.mozilla.org/integration/mozilla-inbound/rev/cfa7cdede09c
Flags: in-testsuite-
Keywords: checkin-needed
Backed out again for OSX bustage.
https://hg.mozilla.org/integration/mozilla-inbound/rev/d9a7f7115ec6
Created attachment 656653 [details] [diff] [review]
Part 1: Remove xlib to buffered image conversion [v3]

Needs a check for aTarget->GetType() == BACKEND_CAIRO
Attachment #654074 - Attachment is obsolete: true
Attachment #656653 - Flags: review?(bas.schouten)
Whiteboard: [autoland:7,1,2,6,4,3,5]
Whiteboard: [autoland:7,1,2,6,4,3,5] → [autoland-try:7,1,2,6,4,3,5]

Comment 40

5 years ago
Autoland Failure
Specified patches [7, 1, 2, 6, 4, 3, 5] do not exist, or are not posted to this bug.

Updated

5 years ago
Whiteboard: [autoland-try:7,1,2,6,4,3,5]
Whiteboard: [autoland-try: 656653, 654075, 654076, 655844, 655494, 654081, 655495]

Updated

5 years ago
Whiteboard: [autoland-try: 656653, 654075, 654076, 655844, 655494, 654081, 655495] → [autoland-in-queue]
fwiw, autoland doesn't work any more :( http://lukasblakk.com/why-isnt-autoland-working/

Comment 42

5 years ago
Try run for ba77974558ac is complete.
Detailed breakdown of the results available here:
    https://tbpl.mozilla.org/?tree=Try&rev=ba77974558ac
Results (out of 258 total builds):
    exception: 2
    success: 232
    warnings: 23
    failure: 1
Builds (or logs if builds failed) available at:
http://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/ajones@mozilla.com-ba77974558ac
Keywords: checkin-needed
Whiteboard: [autoland-in-queue]
Part 1 hasn't received r+ from Bas yet.
Keywords: checkin-needed
Attachment #656653 - Flags: review?(bas.schouten) → review?(matt.woodrow)
Attachment #656653 - Flags: review?(matt.woodrow) → review+
Keywords: checkin-needed
https://hg.mozilla.org/integration/mozilla-inbound/rev/c4b4498ed59b
https://hg.mozilla.org/integration/mozilla-inbound/rev/e7878a522d8b
https://hg.mozilla.org/integration/mozilla-inbound/rev/208e1a753a8b
https://hg.mozilla.org/integration/mozilla-inbound/rev/b3ab8d4a18fe
https://hg.mozilla.org/integration/mozilla-inbound/rev/79df91a7ff8c
https://hg.mozilla.org/integration/mozilla-inbound/rev/07c5d694b7f3
https://hg.mozilla.org/integration/mozilla-inbound/rev/6c66c3997381
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/c4b4498ed59b
https://hg.mozilla.org/mozilla-central/rev/e7878a522d8b
https://hg.mozilla.org/mozilla-central/rev/208e1a753a8b
https://hg.mozilla.org/mozilla-central/rev/b3ab8d4a18fe
https://hg.mozilla.org/mozilla-central/rev/79df91a7ff8c
https://hg.mozilla.org/mozilla-central/rev/07c5d694b7f3
https://hg.mozilla.org/mozilla-central/rev/6c66c3997381
Status: ASSIGNED → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla18
Depends on: 791801

Comment 46

5 years ago
Autoland Patchset:
	Patches: 656653, 654075, 654076, 655844, 655494, 654081, 655495
	Branch: mozilla-central => try
Patch 656653 could not be applied to mozilla-central.
patching file gfx/thebes/gfxPlatform.cpp
Hunk #1 FAILED at 529
1 out of 1 hunks FAILED -- saving rejects to file gfx/thebes/gfxPlatform.cpp.rej
patch failed, unable to continue (try -v)
patch failed, rejects left in working dir

Patchset could not be applied and pushed.

Updated

5 years ago
Depends on: 802658

Updated

5 years ago
Depends on: 815648
You need to log in before you can comment on or make changes to this bug.