Closed Bug 1364360 Opened 8 years ago Closed 8 years ago

0.96ms uninterruptible reflow at gotoPref@chrome://browser/content/preferences/in-content/preferences.js:195:3

Categories

(Core :: DOM: Core & HTML, enhancement, P1)

enhancement

Tracking

()

RESOLVED FIXED
mozilla55
Iteration:
55.6 - May 29
Performance Impact high
Tracking Status
firefox55 --- fixed

People

(Reporter: rjward0, Assigned: bzbarsky)

References

Details

(Whiteboard: [ohnoreflow] [reserve-photon-performance])

Attachments

(5 files, 2 obsolete files)

Here's the stack: gotoPref@chrome://browser/content/preferences/in-content/preferences.js:195:3 init_all/<@chrome://browser/content/preferences/in-content/preferences.js:67:50 _fireOnSelect@chrome://global/content/bindings/richlistbox.xml:78:13 selectItem@chrome://global/content/bindings/listbox.xml:255:11 onxblmousedown@chrome://global/content/bindings/listbox.xml:1033:13 bug 750519 related? preferences.js:67: categories.addEventListener("select", event => gotoPref(event.target.value)); preferences.js:195: mainContent.scrollTop = 0;
Flags: qe-verify?
Priority: -- → P2
It's not worth doing anything about this since the reflow is 1) cheap and 2) happening inside the preferences window rather than the browser chrome window.
Status: NEW → RESOLVED
Closed: 8 years ago
Resolution: --- → WONTFIX
(In reply to Dão Gottwald [::dao] from comment #1) > and 2) > happening inside the preferences window rather than the browser chrome > window. I don't think I understand this point - about:preferences runs by default in the parent process, and so a reflow like this will also affect the browser chrome, despite not technically being contained within it.
Flags: qe-verify?
Priority: P2 → --
Whiteboard: [ohnoreflow][qf][photon-performance] → [ohnoreflow][qf]
(In reply to Mike Conley (:mconley) - At work week, slow to respond from comment #2) > (In reply to Dão Gottwald [::dao] from comment #1) > > and 2) > > happening inside the preferences window rather than the browser chrome > > window. > > I don't think I understand this point - about:preferences runs by default in > the parent process, and so a reflow like this will also affect the browser > chrome, despite not technically being contained within it. It's related to (1) as the preferences document is simpler than the browser document, so reflows will be cheaper. It's true that about:preferences can block the browser UI, but that's doesn't mean we can't do any work in this document. In all likelihood, it's already doing way more expensive things than this reflow. I'm assuming that the reflow happens while the user is interacting with about:preferences. If this happens at random times even if about:preferences is not the foreground tab, that would probably be something we should fix.
(In reply to Dão Gottwald [::dao] from comment #1) > It's not worth doing anything about this since the reflow is 1) cheap I'm not sure this point is correct, as the time in ms isn't very relevant without knowing how fast the machine was. I would also really be curious to know why setting scrollTop to 0 causes a sync reflow; is that a platform bug? (I'll try to profile this to find a stack of the actual reflow)
Component: Untriaged → Preferences
Here is the relevant platform part of the stack from the profiler: PresShell::Flush Layout nsDocument::FlushPendingNotifications(mozilla::FlushType) mozilla::dom::Element::GetScrollFrame(nsIFrame**, bool) mozilla::dom::Element::SetScrollTop(int) mozilla::dom::ElementBinding::set_scrollTop mozilla::dom::GenericBindingSetter(JSContext*, unsigned int, JS::Value*) js::InternalCallOrConstruct(JSContext*, JS::CallArgs const&, js::MaybeConstruct) js::CallSetter(JSContext*, JS::Handle<JS::Value>, JS::Handle<JS::Value>, JS::Handle<JS::Value>) js::NativeSetProperty(JSContext*, JS::Handle<js::NativeObject*>, JS::Handle<jsid>, JS::Handle<JS::Value>, JS::Handle<JS::Value>, js::QualifiedBool, JS::ObjectOpResult&) Interpret js::RunScript(JSContext*, js::RunState&) gotoPref
Attached patch Possible patch (obsolete) — Splinter Review
Maybe it's as simple as not flushing in the special case where this is set to 0? What I would really like is if someone who understands this code could review all the GetScrollFrame calls in that file to verify if there are not other cases where we should skip flushing. And btw, I agree that the preferences window case itself may not be worth a lot of effort, but web content may also use the scrollTop setter, so this may still be worth optimizing on the platform side.
Discussed in Performance Team meeting and bug has been reopened.
No longer blocks: photon-performance-triage
Status: RESOLVED → REOPENED
Flags: qe-verify?
Priority: -- → P2
Resolution: WONTFIX → ---
Whiteboard: [ohnoreflow][qf] → [ohnoreflow][qf] [photon-performance]
Status: REOPENED → NEW
Flags: qe-verify? → qe-verify-
Are there some edge cases where flushing is needed even with value 0? I'm thinking a case where the element doesn't yet have nsIFrame, and flushing would create it and then .scrollTop = 0 would scroll to 0. But if we don't flush, there won't be nsIFrame at all, and scrolling wouldn't happen, and when we later flush, we scroll down. But I can't now think of way to scroll down when frame is created.
Right, if a new frame is created it will be scrolled to top, so as if scrollTop got set to 0 anyway. There's an edge case here which can go wrong, though. Say this is a quirks mode document, the element is the <body>, there is content in the <html> that is _not_ inside the <body>, the <body> is currently display:none, and someone first sets the <body> to display:block and then sets scrollTop on it to 0. In quirks mode, if the body doesn't have a scrollframe of its own, this needs to scroll the _viewport_. With the patch as attached it won't scroll anything, because Element::GetScrollFrame is arguably buggy: it returns immediately if !frame instead of doing viewport stuff. But even if we fixed that, it would not help much because in the example above the frame would not be around for us to do checks on. So what I think we should probably do is: 1) Change Element::GetScrollFrame to press on to the "is this the root thing?" if !frame. 2) Change Element::GetScrollFrame to actually use IsPotentiallyScrollable on the <body> to get the right behavior around this stuff. 3) We should also check why nsIDocument::GetScrollingElement does a layout, not frame, flush; that part seems wrong. 4) We should change Element::GetScrollFrame to take a FlushType, not a boolean. Callers that currently pass true can pass FlushType::Layout. Callers that currently pass false can pass FlushType::None. The caller we're looking at can pass FlushType::Frames when setting scrollTop to 0, and FlushType::Layout otherwise, with a comment explaining why.
I think ehsan added a fast path for window.scrollTo(0, 0) in bug 1365830. Can we use that here instead?
(In reply to Mike Conley (:mconley) from comment #10) > I think ehsan added a fast path for window.scrollTo(0, 0) in bug 1365830. > Can we use that here instead? Apparently not. And apparently the bug is in DOM and can affect not just about:preferences but actual websites.
Component: Preferences → DOM
Product: Firefox → Core
Whiteboard: [ohnoreflow][qf] [photon-performance] → [ohnoreflow][qf:p1] [photon-performance]
Priority: P2 → P3
Whiteboard: [ohnoreflow][qf:p1] [photon-performance] → [ohnoreflow][qf:p1] [reserve-photon-performance]
Assignee: nobody → bzbarsky
Status: NEW → ASSIGNED
We care about the state of frames (and styles, but flushing frames of course requires flushing styles) to determine IsPotentiallyScrollable, but we don't care about layout information.
Attachment #8871033 - Flags: review?(ehsan)
Priority: P3 → P1
Attachment #8867543 - Attachment is obsolete: true
Comment on attachment 8871033 [details] [diff] [review] part 2. Change GetScrollingElement to just flush frames, not layout Review of attachment 8871033 [details] [diff] [review]: ----------------------------------------------------------------- ::: dom/base/nsDocument.cpp @@ +10609,2 @@ > HTMLBodyElement* body = GetBodyElement(); > if (body && !IsPotentiallyScrollable(body)) { We can still avoid this flush if there is no body in this case, and it seems like we shouldn't leave this optimization opportunity on the table here.
Attachment #8871033 - Flags: review?(ehsan) → review+
Comment on attachment 8871032 [details] [diff] [review] part 1. Make Element::GetScrollFrame follow the spec more closely in the quirks mode case Review of attachment 8871032 [details] [diff] [review]: ----------------------------------------------------------------- ::: dom/base/Element.cpp @@ +700,4 @@ > } > > nsIDocument* doc = OwnerDoc(); > + if (this == OwnerDoc()->GetScrollingElement()) { Nit: it may not be apparent to the reader of this code that there is a flush happening in the callee here, do you mind adding a comment about that here?
Attachment #8871032 - Flags: review?(ehsan) → review+
Attachment #8871035 - Flags: review?(ehsan) → review+
Comment on attachment 8871036 [details] [diff] [review] part 4. Don't flush layout when setting scrollTop to 0 Review of attachment 8871036 [details] [diff] [review]: ----------------------------------------------------------------- Thanks a lot!
Attachment #8871036 - Flags: review?(ehsan) → review+
Attachment #8871032 - Attachment is obsolete: true
Attachment #8871332 - Flags: review?(ehsan)
Comment on attachment 8871332 [details] [diff] [review] Interdiff for part 1 per our discussion and the review comments Review of attachment 8871332 [details] [diff] [review]: ----------------------------------------------------------------- Thank you!
Attachment #8871332 - Flags: review?(ehsan) → review+
Pushed by bzbarsky@mozilla.com: https://hg.mozilla.org/integration/mozilla-inbound/rev/3c315bd6cf00 part 1. Make Element::GetScrollFrame follow the spec more closely in the quirks mode case. r=ehsan https://hg.mozilla.org/integration/mozilla-inbound/rev/e670c0dcaa9f part 2. Make Element::GetScrollFrame take a flush type, not a "should I flush?" boolean. r=ehsan https://hg.mozilla.org/integration/mozilla-inbound/rev/dd1c5aecc373 part 3. Don't flush layout when setting scrollTop to 0. r=ehsan
Status: ASSIGNED → RESOLVED
Closed: 8 years ago8 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Target Milestone: --- → mozilla55
Iteration: --- → 55.6 - May 29
Depends on: 1370072
Depends on: 1411138
Depends on: 1415373
Depends on: 1398500
Depends on: 1425107
Component: DOM → DOM: Core & HTML
No longer depends on: 1370072
Regressions: 1370072
Performance Impact: --- → P1
Whiteboard: [ohnoreflow][qf:p1] [reserve-photon-performance] → [ohnoreflow] [reserve-photon-performance]
Blocks: 1154193
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: