Closed Bug 928123 Opened 6 years ago Closed 6 years ago

SyncFrontBufferToBackBuffer hits PushGroup slowpath in moz2d Cairo backend

Categories

(Core :: Graphics, defect)

x86
macOS
defect
Not set

Tracking

()

RESOLVED FIXED
mozilla27

People

(Reporter: BenWa, Assigned: BenWa)

References

Details

Attachments

(2 files, 1 obsolete file)

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.
Attached image 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.
Attached patch patch v1 (obsolete) — Splinter Review
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+
blocking-b2g: --- → koi?
Attached patch patch v1.1Splinter Review
Attachment #818765 - Attachment is obsolete: true
Attachment #819171 - Flags: review+
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
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.
(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
Closed: 6 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. :)
Depends on: 928758
Blocks: 920921
Depends on: 928727
Blocks: 916034
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? → ---
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.