1253412 - APZ causes 4% - 12% TART regression - Event Region Regression

Status: RESOLVED WONTFIX

(Core :: Panning and Zooming, defect)

Description

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

Spinning this out from bug 1251699. We noticed that when we disabled APZ, TART scores improved quite a bit:

https://treeherder.mozilla.org/perf.html#/compare?originalProject=mozilla-central&originalRevision=a4929411c0aa&newProject=try&newRevision=850e39c8f668&framework=1&filter=TART&showOnlyImportant=0

Reversing the above for ease of reading:

linux64      5.01 ± 0.54%    	> 	5.69 ± 0.62%  	+11.87% 	
osx-10-10    9.64 ± 1.71%    	> 	10.06 ± 2.06% 	+4.18%
windows7-32  5.34 ± 0.91%   	> 	5.96 ± 0.54% 	+10.46%
windows8-64  4.92 ± 0.31%   	> 	5.44 ± 0.47% 	+9.56%

Comment 1

[Kartikaya Gupta (email:[e-mail])]

The TART regression from APZ was previously investigated in bug 1210376. The conclusion at https://bugzilla.mozilla.org/show_bug.cgi?id=1210376#c22 was that the bulk of the regression was caused by the event regions code, which is a requirement for APZ. In that bug we decided to live with the regression.

Comment 2

[Kartikaya Gupta (email:[e-mail])]

I verified using a try push that most of this regression is still due to event regions.

https://treeherder.mozilla.org/perf.html#/compare?originalProject=mozilla-central&originalRevision=a4929411c0aa&newProject=try&newRevision=a5f7e2c7dbff&framework=1

^ That uses the same base revisionas the one in comment 0, but compares it to APZ disabled and event regions enabled. There's some thing we can do to improve this (which I'm working on) but we'll likely still have to accept some regression here for APZ. Hopefully we can get it down to < 5%.

Comment 3

[Benoit Girard (:BenWa)]

Changing the title to reflect the suspect cause since Comment #2 is pretty strong evidence.

Comment 4

[Benoit Girard (:BenWa)]

here's a profile of building the event region 5,000 times per frame to get more samples:
+      for (int i = 0; i < 5000; i++) {
+        nsDisplayLayerEventRegions* eventRegions =
+          new (aBuilder) nsDisplayLayerEventRegions(aBuilder, this);
+        eventRegions->AddFrame(aBuilder, this);
+      }

https://cleopatra.io/#report=967e511f12d9514ec580b3e5bdef77d352de128d

Comment 5

[Benoit Girard (:BenWa)]

Attachment: MozReview Request: Bug 1253412 - Add IsScrollFrameWithSnapping to speed up event regions. r=mstange

Review commit: https://reviewboard.mozilla.org/r/42081/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/42081/

Comment 6

[Benoit Girard (:BenWa)]

This patch is a partial help (big) if my approach in Comment #4 is sound and there's a long list of reasons why it might be wrong or incomplete. Probably a good start anyways.

Comment 7

[Botond Ballo [:botond]]

https://reviewboard.mozilla.org/r/42081/#review38591

Drive-by review

::: layout/base/nsLayoutUtils.cpp:8965
(Diff revision 1)
>  {
>    nsIScrollableFrame* sf = do_QueryFrame(aFrame);
>    if (!sf) {
>      return false;
>    }
> +  return sf->IsScrollFrameWithSnapping();

There are statements following this return.

::: layout/generic/nsGfxScrollFrame.cpp:3642
(Diff revision 1)
> +  if (!presContext->IsDynamic() &&
> +      !(mIsRoot && presContext->HasPaginatedScrolling())) {
> +    return false;
> +  }
> +
> +  if (!mIsRoot) {

This can be written a bit more concisely as:

const nsStyleDisplay& display = 
    mIsRoot
  ? presContext->GetViewportScrollbarStylesOverride()
  : \*mOuter->StyleDisplay();
return display.mScrollSnapTypeX != NS_STYLE_SCROLL_SNAP_TYPE_NONE ||
       display.mScrollSnapTypeY != NS_STYLE_SCROLL_SNAP_TYPE_NONE;

Comment 8

[Benoit Girard (:BenWa)]

(In reply to Botond Ballo [:botond] from comment #7)
> const nsStyleDisplay& display = 
>     mIsRoot
>   ? presContext->GetViewportScrollbarStylesOverride()
>   : \*mOuter->StyleDisplay();
> return display.mScrollSnapTypeX != NS_STYLE_SCROLL_SNAP_TYPE_NONE ||
>        display.mScrollSnapTypeY != NS_STYLE_SCROLL_SNAP_TYPE_NONE;

Thanks for the drive-by review. For this the types don't match and doing |ScrollbarStyles(disp)| is costly so I'd rather avoid it.

Moving the patch to bug 1259235.

Comment 9

[Botond Ballo [:botond]]

(In reply to Benoit Girard (:BenWa) from comment #8)
> (In reply to Botond Ballo [:botond] from comment #7)
> > const nsStyleDisplay& display = 
> >     mIsRoot
> >   ? presContext->GetViewportScrollbarStylesOverride()
> >   : \*mOuter->StyleDisplay();
> > return display.mScrollSnapTypeX != NS_STYLE_SCROLL_SNAP_TYPE_NONE ||
> >        display.mScrollSnapTypeY != NS_STYLE_SCROLL_SNAP_TYPE_NONE;
> 
> Thanks for the drive-by review. For this the types don't match and doing
> |ScrollbarStyles(disp)| is costly so I'd rather avoid it.

Oh whoops, I missed the different types! Never mind, then :)

Comment 10

[Benoit Girard (:BenWa)]

Bug 1259235 was the obvious bottleneck. I've spent a lot of time going through the profile and there's no clear win to be had by spending up the current code. It's a long tail of little things left. Things that could maybe done a bit better:

https://cleopatra.io/#report=70457baaf1e2d06a8f4e3a7395c2cb56e27e864e

1) PR_ArenaAllocate seems to be too expensive for what it does. Feels like we'd do better by just malloc'ing rather than spending so much time in that code.
2) We spent a lot of time in region32_init in 'BuildDisplayListForStackingContext' but not doing any operations on them. I think this might be init'ing empty regions. Perhaps we could save a few samples by improving this.
3) AddFrame() has a lot of time spent in the function, but it's a very complicated function. If we could reduce this we would see a win.
4) ApplyNonRoundedIntersection() is 9%, could perhaps be faster but it's not a bottleneck really.
5) There's bug 1259270 but it's not a clear win here and only accounts for 9% of the performance.

To get a good win here we'd have to fix 2 or more items above. I'm going to push to see where we stand now but it's probably pragramatic to accept this regression now that 1) we understand it, 2) it's a large % regression but it's 'just' a few ms, 3) we're expecting to be doing more work to composite this region.

Comment 11

[Benoit Girard (:BenWa)]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=7d931e50ec5d
https://treeherder.mozilla.org/#/jobs?repo=try&revision=88619ff4054e

Comment 12

[Benoit Girard (:BenWa)]

The numbers have not moves even with my patch from bug 1259270:
https://treeherder.mozilla.org/perf.html#/compare?originalProject=try&originalRevision=7d931e50ec5d&newProject=try&newRevision=88619ff4054e&framework=1&filter=TART&showOnlyImportant=0

Comment 13

[Benoit Girard (:BenWa)]

Here's another push forcing event region just to double check but I expect the results to just duplicate what Kats said Comment 1:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=7bcc12d3791c

Likely my experiment to run AddFrame in a loop isn't representative.

Comment 14

[Benoit Girard (:BenWa)]

Here's a push using TN' idea. Not 100% sure it's implemented properly:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=bb74328293ac

Comment 15

[Benoit Girard (:BenWa)]

The numbers in treeherder for Linux are really noisy. This is because it's pulling in data from talos-aws (spot instances) and mixing them in with talos (native) results. For now we should manually ignore these results. This is tracked in bug 1260926 but doesn't block this bug.

Comment 16

[Benoit Girard (:BenWa)]

Here are the relevant URLs as I wait for more re-triggers:

Without Event Region (Base) Vs With Event Region (new):
https://treeherder.mozilla.org/perf.html#/compare?originalProject=try&originalRevision=88619ff4054e&newProject=try&newRevision=7bcc12d3791c&framework=1&filter=TART&showOnlyImportant=0
Without Event Region (Base) Vs With :tn' optimized Event Region (new):
https://treeherder.mozilla.org/perf.html#/compare?originalProject=try&originalRevision=88619ff4054e&newProject=try&newRevision=bb74328293ac&framework=1&filter=TART&showOnlyImportant=0

Comment 17

[Benoit Girard (:BenWa)]

So far the numbers don't seem to be showing an improvement.

:tn does this patch look correct?
https://hg.mozilla.org/try/rev/3dfa7569de6c2c902df0130361971ec0f11b9ea1

I verified locally that it was being hit but I'm not seeing an improvement from it.

Comment 18

[Timothy Nikkel (:tnikkel)]

GetVisualOverflowRect is relative to |this|, so nix the line making borderBox relative to the reference frame, this line:

  borderBox += aBuilder->ToReferenceFrame(this)

Also, the idea behind this approach is that this optimization should take care of the regions where accumulating regions are a net-loss, so for all remaining layer event regions items accumulating regions should be a win (hopefully). Like in your analysis where you divided the regions into two types (ones that never expand after the first rect, and ones that expand for every rect added).

Comment 19

[Benoit Girard (:BenWa)]

Here's the results:
https://treeherder.mozilla.org/perf.html#/compare?originalProject=try&originalRevision=88619ff4054e&newProject=try&newRevision=8efadf5b6ccbf3a6088f58892d85c7317ae46a50&framework=1&filter=TART&showOnlyImportant=0

It might be slightly faster (would need more retiggers to say for sure) but hardly at all and this patch makes the code more complicated.

I'm leaning towards WONTFIX at this point. We knew there would be an added cost to constructing event regions and we have no tangible lead to improve it.

I'll leave the bug open for a bit in case someone disagrees.

Comment 20

[Kartikaya Gupta (email:[e-mail])]

I think we can close this as WONTFIX now based on comment 19 and the investigation that has been done so far.