Closed Bug 1363922 Opened 8 years ago Closed 7 years ago

spend too much time snapping scroll areas to layer pixels

Categories

(Core :: Layout, enhancement, P3)

Product:
Core
Component:
Layout
Platform:
x86
Windows 10
Type:
enhancement

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla57
Project Flags:
Performance Impact high
Tracking Flags:
Tracking Status
firefox57 --- fixed

People

(Reporter: dbaron, Assigned: bas.schouten)

References

Details

(Keywords: perf)

Attachments

(2 files)

Bug 1363922 - Part 1: Remember about the reference frame during BuildDisplayList for ScrollFrameHelper so GetScrolledRect can use it.
59 bytes, text/x-review-board-request
mstange
: review+
Details
Bug 1363922 - Part 2: Estimate scale of frames using their transformation relative to the root frame.
59 bytes, text/x-review-board-request
mstange
: review+
Details
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
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)
(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)
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.
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 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+
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 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 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 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
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
Whiteboard: [qf:investigate:p1] → [qf:p1]
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 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 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+
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
Performance Impact: --- → P1
Whiteboard: [qf:p1]
See Also: → 1846559
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: