928123 - SyncFrontBufferToBackBuffer hits PushGroup slowpath in moz2d Cairo backend

Status: RESOLVED FIXED

(Core :: Graphics, defect)

Description

[Benoit Girard (:BenWa)]

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.

Comment 1

[Benoit Girard (:BenWa)]

Attachment: 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.

Comment 2

[Benoit Girard (:BenWa)]

Attachment: 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.

Comment 3

[Benoit Girard (:BenWa)]

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

Comment 4

[Bas Schouten (:bas.schouten)]

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)

Comment 5

[Benoit Girard (:BenWa)]

Attachment: patch v1.1

Comment 6

[Benoit Girard (:BenWa)]

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

Comment 7

[Benoit Girard (:BenWa)]

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 <[e-mail]> - Bug 928123 - Avoid PushGroup during simple FillRect. r=Bas
    : http://bugzilla.mozilla.org/show_bug.cgi?id=928123

Comment 8

[Benoit Girard (:BenWa)]

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.

Comment 9

[Mike Conley (:mconley) (:⚙️)]

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.

Comment 10

[Benoit Girard (:BenWa)]

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

Comment 11

[Mike Conley (:mconley) (:⚙️)]

Ok, well, I guess we'll see when this lands on m-c and merges into UX. :)

Comment 12

[Phil Ringnalda (:philor)]

https://hg.mozilla.org/mozilla-central/rev/fc7cc3c1dccf

Comment 13

[Mike Conley (:mconley) (:⚙️)]

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

Comment 14

[Benoit Girard (:BenWa)]

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.

Comment 15

[Jonathan Watt [:jwatt]]

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()...

Comment 16

[Jonathan Watt [:jwatt]]

If it is needed for some reason, a specific example that would break without it would be helpful.

Comment 17

[Bas Schouten (:bas.schouten)]

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