781731 - [Azure] Cairo performance slow on Linux

Status: RESOLVED FIXED

(Core :: Graphics: Canvas2D, defect)

Description

[Anthony Jones (:ajones, :kentuckyfriedtakahe, :k17e)]

Canvas DrawImage performance 40 times slower on Azure Cairo compared to Thebes.

Comment 1

[Anthony Jones (:ajones, :kentuckyfriedtakahe, :k17e)]

Attachment: Fixed gfxPlatform::GetSourceSurfaceForSurface() to avoid converting from xlib to buffered image.

Comment 2

[Anthony Jones (:ajones, :kentuckyfriedtakahe, :k17e)]

Attachment: Part 1: Remove xlib to buffered image conversion [v2]

Comment 3

[Anthony Jones (:ajones, :kentuckyfriedtakahe, :k17e)]

Attachment: Part 2: Pass text bounds to AdjustedTarget

Comment 4

[Anthony Jones (:ajones, :kentuckyfriedtakahe, :k17e)]

Attachment: Part 3: Shadow code clean up

Comment 5

[Anthony Jones (:ajones, :kentuckyfriedtakahe, :k17e)]

Attachment: Part 4: Removed double padding with AlphaBoxBlur

Comment 6

[Anthony Jones (:ajones, :kentuckyfriedtakahe, :k17e)]

Attachment: Part 5: Avoid readback by teeing the drawing operations

Comment 7

[Anthony Jones (:ajones, :kentuckyfriedtakahe, :k17e)]

Attachment: Part 6: Match thebes shadow image buffer size

Comment 8

[Anthony Jones (:ajones, :kentuckyfriedtakahe, :k17e)]

Attachment: Part 7: Keep unblurred patches in xlib

Comment 9

[Bas Schouten (:bas.schouten)]

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.

Comment 10

[Mozilla RelEng Bot]

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/[e-mail]-6f1862f4a9e4

Comment 11

[Mozilla RelEng Bot]

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/[e-mail]-08f39d31ccbb

Comment 12

[Mozilla RelEng Bot]

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/[e-mail]-94f4e85b0cb1

Comment 13

[Jeff Muizelaar [:jrmuizel]]

(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?

Comment 14

[Anthony Jones (:ajones, :kentuckyfriedtakahe, :k17e)]

[: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

Comment 15

[Anthony Jones (:ajones, :kentuckyfriedtakahe, :k17e)]

Attachment: Part 8: Fixed empty shadows

Comment 16

[Jeff Muizelaar [:jrmuizel]]

(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?

Comment 17

[Anthony Jones (:ajones, :kentuckyfriedtakahe, :k17e)]

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 18

[Bas Schouten (:bas.schouten)]

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.

Comment 19

[Anthony Jones (:ajones, :kentuckyfriedtakahe, :k17e)]

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 :-)

Comment 20

[Robert O'Callahan (:roc) (email my personal email if necessary)]

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.

Comment 21

[Anthony Jones (:ajones, :kentuckyfriedtakahe, :k17e)]

Attachment: Part 9: Clean up code based on review feedback

Comment 22

[Robert O'Callahan (:roc) (email my personal email if necessary)]

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.

Comment 23

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Actually, you should fold part 9 in part 5 after all.

Comment 24

[Anthony Jones (:ajones, :kentuckyfriedtakahe, :k17e)]

Attachment: Part 5: Avoid readback by teeing the drawing operations v2

Comment 25

[Anthony Jones (:ajones, :kentuckyfriedtakahe, :k17e)]

Attachment: Part 7: Keep unblurred patches in xlib v2

Comment 26

[Mozilla RelEng Bot]

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/[e-mail]-a5ee9b2d1a6e

Comment 27

[Anthony Jones (:ajones, :kentuckyfriedtakahe, :k17e)]

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

Comment 28

[Dão Gottwald [:dao]]

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

Comment 29

[Anthony Jones (:ajones, :kentuckyfriedtakahe, :k17e)]

Attachment: Part 9: Fixed failed assertion in reftest test_canvas.html

Comment 30

[cajbir (:cajbir)]

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.

Comment 31

[cajbir (:cajbir)]

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

Comment 32

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

Backed out

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

Comment 33

[Mozilla RelEng Bot]

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/[e-mail]-f4c7e27c718e

Comment 34

[Anthony Jones (:ajones, :kentuckyfriedtakahe, :k17e)]

Attachment: Part 4: Removed double padding with AlphaBoxBlur v2

Comment 35

[Anthony Jones (:ajones, :kentuckyfriedtakahe, :k17e)]

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 36

[Anthony Jones (:ajones, :kentuckyfriedtakahe, :k17e)]

Comment on attachment 655844 [details] [diff] [review]
Part 4: Removed double padding with AlphaBoxBlur v2

Changed int8_t aStride to int32_t aStride.

Comment 37

[Ryan VanderMeulen [:RyanVM]]

(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

Comment 38

[Ryan VanderMeulen [:RyanVM]]

Backed out again for OSX bustage.
https://hg.mozilla.org/integration/mozilla-inbound/rev/d9a7f7115ec6

Comment 39

[Anthony Jones (:ajones, :kentuckyfriedtakahe, :k17e)]

Attachment: Part 1: Remove xlib to buffered image conversion [v3]

Needs a check for aTarget->GetType() == BACKEND_CAIRO

Comment 40

[Mozilla RelEng Bot]

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

Comment 41

[Joe Drew (not getting mail)]

fwiw, autoland doesn't work any more :( http://lukasblakk.com/why-isnt-autoland-working/

Comment 42

[Mozilla RelEng Bot]

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/[e-mail]-ba77974558ac

Comment 43

[Ryan VanderMeulen [:RyanVM]]

Part 1 hasn't received r+ from Bas yet.

Comment 44

[Ryan VanderMeulen [:RyanVM]]

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

Comment 45

[Ryan VanderMeulen [:RyanVM]]

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

Comment 46

[Mozilla RelEng Bot]

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.