[Azure] Cairo performance slow on Linux

RESOLVED FIXED in mozilla18

Status

()

defect
RESOLVED FIXED
7 years ago
7 years ago

People

(Reporter: ajones, Assigned: ajones)

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.schouten
: 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.
Attachment #650773 - Flags: review?(matt.woodrow) → review+
Attachment #654074 - Flags: review?(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.
Attachment #654074 - Flags: review?(bas.schouten) → review+
Attachment #654075 - Flags: review?(bas.schouten) → review+
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
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
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
Posted patch Part 8: Fixed empty shadows (obsolete) — Splinter Review
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 :-)
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.
Attachment #654082 - Flags: review?(joe) → review?(roc)
Attachment #654078 - Flags: review?(joe) → review?(roc)
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.
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 #654081 - Flags: review?(bas.schouten) → review+
Whiteboard: checkin-needed
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
Keywords: checkin-needed
Whiteboard: checkin-needed
Assignee: nobody → ajones

Comment 30

7 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+
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
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
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)
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]
Autoland Failure
Specified patches [7, 1, 2, 6, 4, 3, 5] do not exist, or are not posted to this bug.

Updated

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

Updated

7 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/
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+
Depends on: 791801
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

7 years ago
Depends on: 802658

Updated

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