spend too much time snapping scroll areas to layer pixels

RESOLVED FIXED in Firefox 57

Status

()

enhancement
P3
normal
RESOLVED FIXED
2 years ago
2 years ago

People

(Reporter: dbaron, Assigned: bas.schouten)

Tracking

({perf})

Trunk
mozilla57
x86
Windows 10
Points:
---

Firefox Tracking Flags

(firefox57 fixed)

Details

(Whiteboard: [qf:p1])

Attachments

(2 attachments)

I just took a profile of scrolling my facebook timeline.  I noticed that about 0.1% of the total time in the profile (including idle) was a separate stack that was also (like bug 1363919) a result of bug 1012752:

nsStyleContext::DoGetStyleDisplay
nsLayoutUtils::GetReferenceFrame
ScrollFrameHelper::GetScrolledRect

Is this something we can avoid?
Flags: needinfo?(mstange)
Whiteboard: [qf] → [qf:investigate:p1]
I'm not sure how. We need to snap the scroll area the same way we'd snap backgrounds during painting, and the latter are snapped with respect to an anchor position that is defined by the reference frame. So we need to know the reference frame during scroll area snapping. Otherwise it will produce bad results in some cases.
Flags: needinfo?(mstange)
Priority: -- → P3
(Assignee)

Comment 2

2 years ago
I see nsLayoutUtils::GetReferenceFrame showing up as over 10% of the time in displaylist building on the youtube front page (DisplayList building itself is 50% of the total time spent, and about 20ms per frame on my machine). As far as I can tell nsLayoutUtils::GetReferenceFrame is called many times sequentially with the same parameter and returning the same reference frame, shouldn't it be possible to cache this?
Flags: needinfo?(mstange)
Flags: needinfo?(bugmail)
It's hard to know where to put the cache or where to invalidate it. I suppose you could invalidate it at the start of painting...
The display list builder has a cache at http://searchfox.org/mozilla-central/rev/bd39b6170f04afeefc751a23bb04e18bbd10352b/layout/painting/nsDisplayList.cpp#1436 but we need this information during reflow, not during painting, so we don't have a display list builder.
Flags: needinfo?(mstange)
(In reply to Markus Stange [:mstange] from comment #3)
> It's hard to know where to put the cache or where to invalidate it. I
> suppose you could invalidate it at the start of painting...

You could cache it using static variables inside GetReferenceFrame itself. A cache of size one should get most of the win here. It looks like nsLayoutUtils::GetReferenceFrame is called in a few places, such as from nsImageFrame::MaybeDecodeForPredictedSize. It might be safer to only enable the cache with a RAII helper that wraps displaylist building. That way we can opt in to the caching behaviour in codepaths that we know are hot and during which the cache will not get invalidated.
Flags: needinfo?(bugmail)
(Assignee)

Comment 5

2 years ago
(In reply to Kartikaya Gupta (email:kats@mozilla.com) from comment #4)
> (In reply to Markus Stange [:mstange] from comment #3)
> > It's hard to know where to put the cache or where to invalidate it. I
> > suppose you could invalidate it at the start of painting...
> 
> You could cache it using static variables inside GetReferenceFrame itself. A
> cache of size one should get most of the win here. It looks like
> nsLayoutUtils::GetReferenceFrame is called in a few places, such as from
> nsImageFrame::MaybeDecodeForPredictedSize. It might be safer to only enable
> the cache with a RAII helper that wraps displaylist building. That way we
> can opt in to the caching behaviour in codepaths that we know are hot and
> during which the cache will not get invalidated.

Hrm, so this was my idea as well, and I've experimented with this by now, but it seems this doesn't work well with the calls from displaylist building (through ScrollFrameHelper::WantAsyncScroll), a lot of calls are made (from nsScrollFrameHelper::GetScrolledRect) that are made subsequently with different arguments passed to GetReferenceFrame, although they all end up with the same result (they all go up to the same nsViewportFrame), each individual call ends up iterating upwards over the frame tree until it hits the nsViewportFrame.

So to reply to Markus comment, the important cost here as far as I can tell from my profiles -is- during display list building.

This appears to me to be a fundamental design flaw, it seems the reference frame if it is determined by upwardly iterating over the frame tree, when required while downwardly iterating over the Frame Tree, could be more efficiently passed -downward- and only updated when running into a frame that IsTransformed || IsPreserve3DLeaf || IsPopup.

Let me know what you think.
Flags: needinfo?(mstange)
Flags: needinfo?(bugmail)
(Assignee)

Comment 6

2 years ago
Hrm, it does seem the reference frame at this point is already known to the DisplayListBuilder, we should be able to use the cache Markus pointed out in his comment.
(Assignee)

Comment 8

2 years ago
This patch improves small paint time on YouTube by about 5%. And will likely help in other places. I feel like the helper I added for nulling out mDLBuilder must exist somewhere else in the tree but I couldn't find it, let me know if you know of a place.
Flags: needinfo?(mstange)
Flags: needinfo?(bugmail)

Comment 9

2 years ago
mozreview-review
Comment on attachment 8895412 [details]
Bug 1363922 - Part 1: Remember about the reference frame during BuildDisplayList for ScrollFrameHelper so GetScrolledRect can use it.

https://reviewboard.mozilla.org/r/166588/#review171744

This looks ok to me, so r+, but I'd like markus to also take a look. Your suggestion of propagating the reference frame downwards rather than walking up the tree to get it might be a better approach from a high-level view.

::: commit-message-6a3a3:1
(Diff revision 1)
> +Bug 1363922 - Part 1: Remember about the DisplayListBuilder during BuildDisplayList for so GetScrolledRect can use its reference frame cache. r=kats

Are there going to be more parts? If not, drop "Part 1" from the commit message

::: layout/generic/nsGfxScrollFrame.cpp:2002
(Diff revision 1)
>  // but it doesn't have that reference so instead we use a static global to
>  // ensure the new one gets a fresh value.
>  static uint32_t sScrollGenerationCounter = 0;
>  
>  ScrollFrameHelper::ScrollFrameHelper(nsContainerFrame* aOuter,
> -                                             bool aIsRoot)
> +                                               bool aIsRoot)

spurious indent?

::: layout/generic/nsGfxScrollFrame.cpp:3253
(Diff revision 1)
> +template<typename T>
> +class SetAndNullOnExit

If you're going to the trouble of templating this to make it reusable, you should put it somewhere more accessible, like nsLayoutUtils. Otherwise you might as well drop the template and just hard-code this to nsDisplayListBuilder. Regardless, add "MOZ_RAII" between "class" and "SetAndNullOnExit".
Attachment #8895412 - Flags: review?(bugmail) → review+
(Assignee)

Comment 10

2 years ago
Converting this bug to the generic cost associated with GetScrolledRect, one more patch incoming that should completely tackle this problem (including the issue the bug was initially opened for).
Summary: spend too much time in GetStyleDisplay() in nsLayoutUtils::GetReferenceFrame in order to snap scroll areas to layer pixels → spend too much time snapping scroll areas to layer pixels
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)

Comment 14

2 years ago
mozreview-review
Comment on attachment 8895561 [details]
Bug 1363922 - Part 2: Estimate scale of frames using their transformation relative to the root frame.

https://reviewboard.mozilla.org/r/166774/#review172376

::: layout/generic/nsGfxScrollFrame.cpp:5887
(Diff revision 1)
>    if (scrollPort.Overflows() || scrolledRect.Overflows()) {
>      return result;
>    }
>  
>    // Now, snap the bottom right corner of both of these rects.
>    // We snap to layer pixels, so we need to respect the layer's scale.

This comment needs to be adjusted. How about:

We ignore CSS transforms here because we don't have a cheap way of accessing the layer resolution of the layer that this frame is going to be painted to, and respecting the presshell resolution is enough for most cases.
Attachment #8895561 - Flags: review?(mstange) → review+

Comment 15

2 years ago
mozreview-review
Comment on attachment 8895412 [details]
Bug 1363922 - Part 1: Remember about the reference frame during BuildDisplayList for ScrollFrameHelper so GetScrolledRect can use it.

https://reviewboard.mozilla.org/r/166588/#review171910

I think this patch can be simplified a lot.

You store the display list builder on the scroll frame so that you can call FindReferenceFrameFor(mOuter) on it. mOuter isn't going to change during painting. So you might as well store the result of that call.
And since ScrollFrameHelper::BuildDisplayList is called while mOuter is the builder's current frame, you can just call aBuilder->GetCurrentReferenceFrame(), and store that, instead of storing the display list builder. I'd call the storage field "mReferenceFrameDuringPainting".

I don't have an opinion on where to put SetAndNullOnExit or whether to keep using templates on it.

::: layout/generic/nsGfxScrollFrame.cpp:5887
(Diff revisions 1 - 2)
>    if (scrollPort.Overflows() || scrolledRect.Overflows()) {
>      return result;
>    }
>  
>    // Now, snap the bottom right corner of both of these rects.
>    // We snap to layer pixels, so we need to respect the layer's scale.
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)

Comment 18

2 years ago
mozreview-review
Comment on attachment 8895412 [details]
Bug 1363922 - Part 1: Remember about the reference frame during BuildDisplayList for ScrollFrameHelper so GetScrolledRect can use it.

https://reviewboard.mozilla.org/r/166588/#review174244
Attachment #8895412 - Flags: review?(mstange) → review+
Assignee: nobody → bas

Comment 19

2 years ago
Pushed by bschouten@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/5ea221b039d4
Part 1: Remember about the reference frame during BuildDisplayList for ScrollFrameHelper so GetScrolledRect can use it. r=mstange
https://hg.mozilla.org/integration/mozilla-inbound/rev/189abf9f8d6d
Part 2: Estimate scale of frames based on their PresShell resolution. r=mstange
(Assignee)

Updated

2 years ago
Whiteboard: [qf:investigate:p1] → [qf:p1]

Comment 21

2 years ago
Pushed by bschouten@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/2f49d825c651
Part 1: Remember about the reference frame during BuildDisplayList for ScrollFrameHelper so GetScrolledRect can use it. r=mstange
https://hg.mozilla.org/integration/mozilla-inbound/rev/3241eaaa608d
Part 2: Estimate scale of frames based on their PresShell resolution. r=mstange
Push with failures: https://treeherder.mozilla.org/#/jobs?repo=mozilla-inbound&revision=3241eaaa608dcce34e44818f7e71ef0eeee06feb&filter-resultStatus=testfailed&filter-resultStatus=busted&filter-resultStatus=exception&filter-resultStatus=retry&filter-resultStatus=usercancel&filter-resultStatus=runnable

Failure log Windows 7 debug: https://treeherder.mozilla.org/logviewer.html#?job_id=124392183&repo=mozilla-inbound
15:10:07     INFO -  2813 INFO None2814 INFO TEST-START | layout/base/tests/test_transformed_scrolling_repaints_3.html
15:10:07     INFO -  GECKO(1456) | ++DOMWINDOW == 21 (0FD64000) [pid = 1456] [serial = 790] [outer = 1D4D5000]
15:10:07     INFO -  GECKO(1456) | ++DOCSHELL 0FDDA400 == 7 [pid = 1456] [id = {76a590f0-5614-4e77-a7d8-6c01c130d5de}]
15:10:07     INFO -  GECKO(1456) | ++DOMWINDOW == 22 (0FDDA800) [pid = 1456] [serial = 791] [outer = 00000000]
15:10:07     INFO -  GECKO(1456) | ++DOMWINDOW == 23 (0FDDB000) [pid = 1456] [serial = 792] [outer = 0FDDA800]
15:10:07     INFO -  GECKO(1456) | ++DOCSHELL 0FDE2400 == 8 [pid = 1456] [id = {53f7f6aa-a96f-45b9-9683-5c7eb417564b}]
15:10:07     INFO -  GECKO(1456) | ++DOMWINDOW == 24 (0FDE2C00) [pid = 1456] [serial = 793] [outer = 00000000]
15:10:07     INFO -  GECKO(1456) | ++DOCSHELL 100D0800 == 9 [pid = 1456] [id = {9f876eb7-3203-4eea-8154-60be790bb72e}]
15:10:07     INFO -  GECKO(1456) | ++DOMWINDOW == 25 (100D1000) [pid = 1456] [serial = 794] [outer = 00000000]
15:10:07     INFO -  GECKO(1456) | ++DOMWINDOW == 26 (10104400) [pid = 1456] [serial = 795] [outer = 100D1000]
15:10:07     INFO -  GECKO(1456) | ++DOMWINDOW == 27 (10173800) [pid = 1456] [serial = 796] [outer = 0FDE2C00]
15:10:07     INFO -  GECKO(1456) | ++DOMWINDOW == 28 (10105000) [pid = 1456] [serial = 797] [outer = 100D1000]
15:10:07     INFO -  GECKO(1456) | ++DOMWINDOW == 29 (0BD7A000) [pid = 1456] [serial = 798] [outer = 100D1000]
15:10:08     INFO -  GECKO(1456) | ++DOCSHELL 13007400 == 10 [pid = 1456] [id = {c700f8e3-91eb-4f06-a351-db347a40b0c9}]
15:10:08     INFO -  GECKO(1456) | ++DOMWINDOW == 30 (13008C00) [pid = 1456] [serial = 799] [outer = 00000000]
15:10:08     INFO -  GECKO(1456) | [Parent 1456] WARNING: No inner window available!: file z:/build/build/src/dom/base/nsGlobalWindow.cpp, line 10412
15:10:08     INFO -  GECKO(1456) | ++DOMWINDOW == 31 (0BEE8000) [pid = 1456] [serial = 800] [outer = 13008C00]
15:10:08     INFO -  TEST-INFO | started process screenshot
15:10:08     INFO -  TEST-INFO | screenshot: exit 0
15:10:08    ERROR -  2815 INFO TEST-UNEXPECTED-FAIL | layout/base/tests/test_transformed_scrolling_repaints_3.html | Fully-visible scrolled element should not have been painted - got true, expected false
15:10:08     INFO -      SimpleTest.is@SimpleTest/SimpleTest.js:312:5
15:10:08     INFO -      nextIteration@layout/base/tests/transformed_scrolling_repaints_3_window.html:46:3
15:10:08     INFO -      waitForPaints@SimpleTest/paint_listener.js:77:5
15:10:08     INFO -      waitForPaints/<@SimpleTest/paint_listener.js:65:22
15:10:08     INFO -      setTimeout handler*paintListener@SimpleTest/paint_listener.js:32:7
15:10:08     INFO -      EventListener.handleEvent*@SimpleTest/paint_listener.js:35:3
15:10:08     INFO -      @SimpleTest/paint_listener.js:1:2

Failure log Android: https://treeherder.mozilla.org/logviewer.html#?job_id=124391916&repo=mozilla-inbound
[task 2017-08-20T16:37:10.819091Z] 16:37:10     INFO -  121 INFO TEST-PASS | docshell/test/test_bug590573.html | test 9
[task 2017-08-20T16:37:10.819256Z] 16:37:10     INFO -  122 INFO TEST-PASS | docshell/test/test_bug590573.html | test 10
[task 2017-08-20T16:37:10.819620Z] 16:37:10     INFO -  Buffered messages finished
[task 2017-08-20T16:37:10.819847Z] 16:37:10     INFO -  123 INFO TEST-UNEXPECTED-FAIL | docshell/test/test_bug590573.html | test 11, got 300.1333312988281 for popup.scrollY instead of 299|300 - got 1, expected +0
[task 2017-08-20T16:37:10.820007Z] 16:37:10     INFO -      SimpleTest.is@SimpleTest/SimpleTest.js:312:5
[task 2017-08-20T16:37:10.820193Z] 16:37:10     INFO -      testBody@docshell/test/test_bug590573.html:225:5
[task 2017-08-20T16:37:10.820353Z] 16:37:10     INFO -      pageLoad@docshell/test/test_bug590573.html:138:13
[task 2017-08-20T16:37:10.820525Z] 16:37:10     INFO -      page2PageShow@docshell/test/test_bug590573.html:90:5
[task 2017-08-20T16:37:10.820665Z] 16:37:10     INFO -      onpageshow@docshell/test/file_bug590573_2.html:1:1
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
(Assignee)

Comment 26

2 years ago
Comment on attachment 8895561 [details]
Bug 1363922 - Part 2: Estimate scale of frames using their transformation relative to the root frame.

Markus, be so kind to re-review :) This seems to pass all tests!
Flags: needinfo?(bas)
Attachment #8895561 - Flags: review+ → review?(mstange)

Comment 27

2 years ago
mozreview-review
Comment on attachment 8895561 [details]
Bug 1363922 - Part 2: Estimate scale of frames using their transformation relative to the root frame.

https://reviewboard.mozilla.org/r/166774/#review177516

::: layout/painting/FrameLayerBuilder.cpp:5878
(Diff revision 3)
> -      // the transform from some content inside the popup to some content
> -      // which is an ancestor of the popup.
> -      break;
> -    }
>  
> -    const SmallPointerArray<DisplayItemData>& array = aFrame->DisplayItemData();
> +  nsIFrame* root = aFrame->PresContext()->GetRootPresContext()->PresShell()->GetRootFrame();

GetRootPresContext() seems to have the capability of returning null. So we should probably have a null check here.

::: layout/painting/FrameLayerBuilder.cpp:5883
(Diff revision 3)
> -    const SmallPointerArray<DisplayItemData>& array = aFrame->DisplayItemData();
> +  nsIFrame* root = aFrame->PresContext()->GetRootPresContext()->PresShell()->GetRootFrame();
>  
> -    for (uint32_t i = 0; i < array.Length(); i++) {
> -      Layer* layer = DisplayItemData::AssertDisplayItemData(array.ElementAt(i))->mLayer;
> -      ContainerLayer* container = layer->AsContainerLayer();
> -      if (!container ||
> +  MOZ_ASSERT(root);
> +
> +  Matrix4x4 transform = Matrix4x4::Scaling(root->PresContext()->PresShell()->GetResolution(),
> +                                           root->PresContext()->PresShell()->GetResolution(), 1.0);

Let's store the resolution in a local variable.
Attachment #8895561 - Flags: review?(mstange) → review+

Comment 28

2 years ago
Pushed by bschouten@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/6db9db55ab37
Part 1: Remember about the reference frame during BuildDisplayList for ScrollFrameHelper so GetScrolledRect can use it. r=mstange
https://hg.mozilla.org/integration/mozilla-inbound/rev/6c9006de0463
Part 2: Estimate scale of frames using their transformation relative to the root frame. r=mstange
https://hg.mozilla.org/mozilla-central/rev/6db9db55ab37
https://hg.mozilla.org/mozilla-central/rev/6c9006de0463
Status: NEW → RESOLVED
Last Resolved: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla57
You need to log in before you can comment on or make changes to this bug.