Closed Bug 1253412 Opened 6 years ago Closed 6 years ago

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

Categories

(Core :: Panning and Zooming, defect)

defect
Not set
normal

Tracking

()

RESOLVED WONTFIX

People

(Reporter: mconley, Unassigned)

References

(Depends on 2 open bugs)

Details

(Whiteboard: [gfx-noted])

Attachments

(1 obsolete file)

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%
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.
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%.
Whiteboard: [gfx-noted]
Changing the title to reflect the suspect cause since Comment #2 is pretty strong evidence.
Summary: APZ causes 4% - 12% TART regression → APZ causes 4% - 12% TART regression - Event Region Regression
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
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.
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;
Depends on: 1259235
(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.
(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 :)
Depends on: 1259270
Attachment #8734086 - Attachment is obsolete: true
Attachment #8734086 - Flags: review?(mstange)
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.
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.
Here's a push using TN' idea. Not 100% sure it's implemented properly:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=bb74328293ac
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.
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.
Flags: needinfo?(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).
Flags: needinfo?(tnikkel)
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.
I think we can close this as WONTFIX now based on comment 19 and the investigation that has been done so far.
Status: NEW → RESOLVED
Closed: 6 years ago
Resolution: --- → WONTFIX
You need to log in before you can comment on or make changes to this bug.