SyncFrontBufferToBackBuffer hits PushGroup slowpath in moz2d Cairo backend

RESOLVED FIXED in mozilla27

Status

()

RESOLVED FIXED
5 years ago
4 years ago

People

(Reporter: BenWa, Assigned: BenWa)

Tracking

(Blocks: 1 bug)

Trunk
mozilla27
x86
Mac OS X
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(2 attachments, 1 obsolete attachment)

(Assignee)

Description

5 years ago
SyncFrontBufferToBackBuffer slowness is responsible for some of our b2g frame misses. Its hitting a PushGroup path instead of something like a memcpy or equivalent.

We enter here: http://mxr.mozilla.org/mozilla-central/source/gfx/2d/DrawTargetCairo.cpp#611
because we have OP_SOURCE.
(Assignee)

Comment 1

5 years ago
Created attachment 818758 [details]
Patch profile

Working on a patch with some tips from Bas. Looks to avoid the push group. Now we don't get any large blocks from SyncFrontBufferToBackBuffer which is enough to avoid these frame skips. Trying to validate the results before I post the patch.
(Assignee)

Comment 2

5 years ago
Created attachment 818765 [details] [diff] [review]
patch v1

Ran some more tests and it confirms that this patch makes a good improvements by itself. I haven't confirmed that we're hitting the right fast path yet however so I'll dig more tomorrow and likely file another bug.
Assignee: nobody → bgirard
Status: NEW → ASSIGNED
Attachment #818765 - Flags: review?(bas)
Comment on attachment 818765 [details] [diff] [review]
patch v1

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

::: gfx/2d/DrawTargetCairo.cpp
@@ +608,4 @@
>    cairo_set_antialias(mContext, GfxAntialiasToCairoAntialias(aOptions.mAntialiasMode));
>  
>    if (NeedIntermediateSurface(aPattern, aOptions) ||
> +      !IsOperatorBoundByMask(aOptions.mCompositionOp) && !aPathBoundsClip) {

nit: Just for clarity make this (!IsOperatorBoundByMask(aOptions.mCompositionOp) && !aPathBoundsClip)
Attachment #818765 - Flags: review?(bas) → review+
(Assignee)

Updated

5 years ago
blocking-b2g: --- → koi?
(Assignee)

Comment 5

5 years ago
Created attachment 819171 [details] [diff] [review]
patch v1.1
Attachment #818765 - Attachment is obsolete: true
Attachment #819171 - Flags: review+
(Assignee)

Comment 7

5 years ago
Looks like this patch is responsible for a TResize improvement:

Improvement: Mozilla-Inbound-Non-PGO - TResize - WINNT 5.1 (ix) - 9.57% decrease
--------------------------------------------------------------------------------
    Previous: avg 10.369 stddev 0.066 of 12 runs up to revision 5861a2de43d3
    New     : avg 9.377 stddev 0.091 of 12 runs since revision a46ff1a56160
    Change  : -0.992 (9.57% / z=14.968)
    Graph   : http://mzl.la/15RUh33

Changeset range: http://hg.mozilla.org/integration/mozilla-inbound/pushloghtml?fromchange=5861a2de43d3&tochange=a46ff1a56160

Changesets:
  * http://hg.mozilla.org/integration/mozilla-inbound/rev/fc7cc3c1dccf
    : Benoit Girard <b56girard@gmail.com> - Bug 928123 - Avoid PushGroup during simple FillRect. r=Bas
    : http://bugzilla.mozilla.org/show_bug.cgi?id=928123
(Assignee)

Comment 8

5 years ago
looks like more improvements are coming:
Improvement: Mozilla-Inbound-Non-PGO - tscroll-ASAP - Ubuntu HW 12.04 - 5.28% decrease
--------------------------------------------------------------------------------------
    Previous: avg 10.132 stddev 0.022 of 12 runs up to revision 5861a2de43d3
    New     : avg 9.597 stddev 0.024 of 12 runs since revision fbbdf3bb140c
    Change  : -0.535 (5.28% / z=24.642)
    Graph   : http://mzl.la/1awb5uQ

mconley just a ping that it will be worth investigating the results this has for the UX branch.
The comparison profiles we have for the TART tests for Windows XP do not show SyncFrontBufferToBackBuffer as one of the expensive operations that we care about, so I do not expect this patch to give us any win - but we'll keep an eye on it, regardless.
(Assignee)

Comment 10

5 years ago
(In reply to Mike Conley (:mconley) from comment #9)
> The comparison profiles we have for the TART tests for Windows XP do not
> show SyncFrontBufferToBackBuffer as one of the expensive operations that we
> care about, so I do not expect this patch to give us any win - but we'll
> keep an eye on it, regardless.

The fix was made in azure, not in layers, so it likely affects many calls to FillRect.
Ok, well, I guess we'll see when this lands on m-c and merges into UX. :)
https://hg.mozilla.org/mozilla-central/rev/fc7cc3c1dccf
Status: ASSIGNED → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla27
The patch merged into UX this morning, and while I'm seeing some improvement for both UX and m-c on tscrollx on the Linux platforms, I'm not seeing any movement for TART, tpaint, or ts_paint for any platform for either branch.

But thanks for thinking of us. :)

Updated

5 years ago
Depends on: 928758

Updated

5 years ago
Blocks: 920921

Updated

5 years ago
Depends on: 928727
Blocks: 916034
(Assignee)

Comment 14

5 years ago
Alright we don't need koi for this because moz2d isn't enabled there. This means that koi isn't impacted by this bug so it doesn't matter.
blocking-b2g: koi? → ---

Updated

5 years ago
Depends on: 976322
Why do we have the |(!IsOperatorBoundByMask(aOptions.mCompositionOp) && !aPathBoundsClip)| check? I don't see the point in creating a group in that case. We're not calling cairo_mask()...
Flags: needinfo?(bas)
If it is needed for some reason, a specific example that would break without it would be helpful.
(In reply to Jonathan Watt [:jwatt] from comment #15)
> Why do we have the |(!IsOperatorBoundByMask(aOptions.mCompositionOp) &&
> !aPathBoundsClip)| check? I don't see the point in creating a group in that
> case. We're not calling cairo_mask()...

So IsOperatorBoundByMask, is for the mask, referring to the mask of the operation, i.e. for a fill that is the path currently set on the context.
Flags: needinfo?(bas)
You need to log in before you can comment on or make changes to this bug.