Last Comment Bug 781731 - [Azure] Cairo performance slow on Linux
: [Azure] Cairo performance slow on Linux
Status: RESOLVED FIXED
:
Product: Core
Classification: Components
Component: Canvas: 2D (show other bugs)
: Trunk
: All Linux
: -- normal with 1 vote (vote)
: mozilla18
Assigned To: Anthony Jones (:kentuckyfriedtakahe, :k17e)
:
: Milan Sreckovic [:milan]
Mentors:
http://ie.microsoft.com/testdrive/Per...
Depends on: 591358 791801 802658 815648
Blocks: 773460 784582
  Show dependency treegraph
 
Reported: 2012-08-09 20:52 PDT by Anthony Jones (:kentuckyfriedtakahe, :k17e)
Modified: 2012-11-27 09:03 PST (History)
15 users (show)
ryanvm: in‑testsuite-
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
Fixed gfxPlatform::GetSourceSurfaceForSurface() to avoid converting from xlib to buffered image. (1.28 KB, patch)
2012-08-09 20:55 PDT, Anthony Jones (:kentuckyfriedtakahe, :k17e)
matt.woodrow: review+
Details | Diff | Splinter Review
Part 1: Remove xlib to buffered image conversion [v2] (1.53 KB, patch)
2012-08-21 21:22 PDT, Anthony Jones (:kentuckyfriedtakahe, :k17e)
bas: review+
Details | Diff | Splinter Review
Part 2: Pass text bounds to AdjustedTarget (2.02 KB, patch)
2012-08-21 21:24 PDT, Anthony Jones (:kentuckyfriedtakahe, :k17e)
bas: review+
Details | Diff | Splinter Review
Part 3: Shadow code clean up (3.29 KB, patch)
2012-08-21 21:26 PDT, Anthony Jones (:kentuckyfriedtakahe, :k17e)
jmuizelaar: review+
Details | Diff | Splinter Review
Part 4: Removed double padding with AlphaBoxBlur (5.64 KB, patch)
2012-08-21 21:27 PDT, Anthony Jones (:kentuckyfriedtakahe, :k17e)
roc: review+
Details | Diff | Splinter Review
Part 5: Avoid readback by teeing the drawing operations (7.95 KB, patch)
2012-08-21 21:28 PDT, Anthony Jones (:kentuckyfriedtakahe, :k17e)
no flags Details | Diff | Splinter Review
Part 6: Match thebes shadow image buffer size (1.32 KB, patch)
2012-08-21 21:31 PDT, Anthony Jones (:kentuckyfriedtakahe, :k17e)
roc: review+
Details | Diff | Splinter Review
Part 7: Keep unblurred patches in xlib (5.47 KB, patch)
2012-08-21 21:36 PDT, Anthony Jones (:kentuckyfriedtakahe, :k17e)
roc: review+
Details | Diff | Splinter Review
Part 8: Fixed empty shadows (1.55 KB, patch)
2012-08-23 22:13 PDT, Anthony Jones (:kentuckyfriedtakahe, :k17e)
bas: review+
Details | Diff | Splinter Review
Part 9: Clean up code based on review feedback (3.92 KB, patch)
2012-08-26 15:53 PDT, Anthony Jones (:kentuckyfriedtakahe, :k17e)
no flags Details | Diff | Splinter Review
Part 5: Avoid readback by teeing the drawing operations v2 (6.95 KB, patch)
2012-08-26 20:34 PDT, Anthony Jones (:kentuckyfriedtakahe, :k17e)
roc: review+
Details | Diff | Splinter Review
Part 7: Keep unblurred patches in xlib v2 (6.44 KB, patch)
2012-08-26 20:35 PDT, Anthony Jones (:kentuckyfriedtakahe, :k17e)
roc: review+
Details | Diff | Splinter Review
Part 9: Fixed failed assertion in reftest test_canvas.html (1.56 KB, patch)
2012-08-27 02:48 PDT, Anthony Jones (:kentuckyfriedtakahe, :k17e)
cajbir.bugzilla: review+
Details | Diff | Splinter Review
Part 4: Removed double padding with AlphaBoxBlur v2 (5.64 KB, patch)
2012-08-27 18:09 PDT, Anthony Jones (:kentuckyfriedtakahe, :k17e)
roc: review+
Details | Diff | Splinter Review
Part 1: Remove xlib to buffered image conversion [v3] (1.61 KB, patch)
2012-08-29 16:11 PDT, Anthony Jones (:kentuckyfriedtakahe, :k17e)
matt.woodrow: review+
Details | Diff | Splinter Review

Description Anthony Jones (:kentuckyfriedtakahe, :k17e) 2012-08-09 20:52:11 PDT
Canvas DrawImage performance 40 times slower on Azure Cairo compared to Thebes.
Comment 1 Anthony Jones (:kentuckyfriedtakahe, :k17e) 2012-08-09 20:55:07 PDT
Created attachment 650773 [details] [diff] [review]
Fixed gfxPlatform::GetSourceSurfaceForSurface() to avoid converting from xlib to buffered image.
Comment 2 Anthony Jones (:kentuckyfriedtakahe, :k17e) 2012-08-21 21:22:17 PDT
Created attachment 654074 [details] [diff] [review]
Part 1: Remove xlib to buffered image conversion [v2]
Comment 3 Anthony Jones (:kentuckyfriedtakahe, :k17e) 2012-08-21 21:24:27 PDT
Created attachment 654075 [details] [diff] [review]
Part 2: Pass text bounds to AdjustedTarget
Comment 4 Anthony Jones (:kentuckyfriedtakahe, :k17e) 2012-08-21 21:26:02 PDT
Created attachment 654076 [details] [diff] [review]
Part 3: Shadow code clean up
Comment 5 Anthony Jones (:kentuckyfriedtakahe, :k17e) 2012-08-21 21:27:36 PDT
Created attachment 654078 [details] [diff] [review]
Part 4: Removed double padding with AlphaBoxBlur
Comment 6 Anthony Jones (:kentuckyfriedtakahe, :k17e) 2012-08-21 21:28:59 PDT
Created attachment 654080 [details] [diff] [review]
Part 5: Avoid readback by teeing the drawing operations
Comment 7 Anthony Jones (:kentuckyfriedtakahe, :k17e) 2012-08-21 21:31:39 PDT
Created attachment 654081 [details] [diff] [review]
Part 6: Match thebes shadow image buffer size
Comment 8 Anthony Jones (:kentuckyfriedtakahe, :k17e) 2012-08-21 21:36:49 PDT
Created attachment 654082 [details] [diff] [review]
Part 7: Keep unblurred patches in xlib
Comment 9 Bas Schouten (:bas.schouten) 2012-08-22 00:07:23 PDT
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 2012-08-22 01:45:25 PDT
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 Mozilla RelEng Bot 2012-08-22 15:15:25 PDT
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 Mozilla RelEng Bot 2012-08-22 22:30:24 PDT
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
Comment 13 Jeff Muizelaar [:jrmuizel] 2012-08-23 06:28:01 PDT
(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 (:kentuckyfriedtakahe, :k17e) 2012-08-23 16:00:01 PDT
[: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 (:kentuckyfriedtakahe, :k17e) 2012-08-23 22:13:36 PDT
Created attachment 654919 [details] [diff] [review]
Part 8: Fixed empty shadows
Comment 16 Jeff Muizelaar [:jrmuizel] 2012-08-23 22:22:35 PDT
(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 (:kentuckyfriedtakahe, :k17e) 2012-08-23 23:00:10 PDT
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) 2012-08-24 00:03:38 PDT
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 (:kentuckyfriedtakahe, :k17e) 2012-08-24 11:24:49 PDT
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) 2012-08-24 20:50:03 PDT
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 (:kentuckyfriedtakahe, :k17e) 2012-08-26 15:53:20 PDT
Created attachment 655467 [details] [diff] [review]
Part 9: Clean up code based on review feedback
Comment 22 Robert O'Callahan (:roc) (email my personal email if necessary) 2012-08-26 16:43:05 PDT
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) 2012-08-26 16:44:23 PDT
Actually, you should fold part 9 in part 5 after all.
Comment 24 Anthony Jones (:kentuckyfriedtakahe, :k17e) 2012-08-26 20:34:27 PDT
Created attachment 655494 [details] [diff] [review]
Part 5: Avoid readback by teeing the drawing operations v2
Comment 25 Anthony Jones (:kentuckyfriedtakahe, :k17e) 2012-08-26 20:35:37 PDT
Created attachment 655495 [details] [diff] [review]
Part 7: Keep unblurred patches in xlib v2
Comment 26 Mozilla RelEng Bot 2012-08-26 21:45:24 PDT
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
Comment 27 Anthony Jones (:kentuckyfriedtakahe, :k17e) 2012-08-26 23:12:32 PDT
https://tbpl.mozilla.org/?tree=Try&rev=f4c7e27c718e
Comment 29 Anthony Jones (:kentuckyfriedtakahe, :k17e) 2012-08-27 02:48:36 PDT
Created attachment 655550 [details] [diff] [review]
Part 9: Fixed failed assertion in reftest test_canvas.html
Comment 30 cajbir (:cajbir) 2012-08-27 03:01:04 PDT
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 32 :Ms2ger (⌚ UTC+1/+2) 2012-08-27 03:29:45 PDT
Backed out

https://hg.mozilla.org/integration/mozilla-inbound/rev/a8f183cf4b38
Comment 33 Mozilla RelEng Bot 2012-08-27 03:45:24 PDT
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
Comment 34 Anthony Jones (:kentuckyfriedtakahe, :k17e) 2012-08-27 18:09:46 PDT
Created attachment 655844 [details] [diff] [review]
Part 4: Removed double padding with AlphaBoxBlur v2
Comment 35 Anthony Jones (:kentuckyfriedtakahe, :k17e) 2012-08-28 13:58:36 PDT
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 (:kentuckyfriedtakahe, :k17e) 2012-08-28 13:59:21 PDT
Comment on attachment 655844 [details] [diff] [review]
Part 4: Removed double padding with AlphaBoxBlur v2

Changed int8_t aStride to int32_t aStride.
Comment 38 Ryan VanderMeulen [:RyanVM] 2012-08-29 15:44:52 PDT
Backed out again for OSX bustage.
https://hg.mozilla.org/integration/mozilla-inbound/rev/d9a7f7115ec6
Comment 39 Anthony Jones (:kentuckyfriedtakahe, :k17e) 2012-08-29 16:11:00 PDT
Created attachment 656653 [details] [diff] [review]
Part 1: Remove xlib to buffered image conversion [v3]

Needs a check for aTarget->GetType() == BACKEND_CAIRO
Comment 40 Mozilla RelEng Bot 2012-08-29 16:20:48 PDT
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) 2012-08-30 10:59:22 PDT
fwiw, autoland doesn't work any more :( http://lukasblakk.com/why-isnt-autoland-working/
Comment 42 Mozilla RelEng Bot 2012-08-31 04:15:25 PDT
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
Comment 43 Ryan VanderMeulen [:RyanVM] 2012-09-02 13:49:45 PDT
Part 1 hasn't received r+ from Bas yet.
Comment 46 Mozilla RelEng Bot 2012-10-03 07:39:24 PDT
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.

Note You need to log in before you can comment on or make changes to this bug.