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)
Core
DOM: Core & HTML
Tracking
()
People
(Reporter: rjward0, Assigned: bzbarsky)
References
Details
(Whiteboard: [ohnoreflow] [reserve-photon-performance])
Attachments
(5 files, 2 obsolete files)
1.06 KB,
patch
|
ehsan.akhgari
:
review+
|
Details | Diff | Splinter Review |
3.09 KB,
patch
|
ehsan.akhgari
:
review+
|
Details | Diff | Splinter Review |
2.30 KB,
patch
|
ehsan.akhgari
:
review+
|
Details | Diff | Splinter Review |
5.36 KB,
patch
|
ehsan.akhgari
:
review+
|
Details | Diff | Splinter Review |
8.60 KB,
patch
|
Details | Diff | Splinter Review |
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;
Updated•8 years ago
|
Flags: qe-verify?
Priority: -- → P2
Comment 1•8 years ago
|
||
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
Comment 2•8 years ago
|
||
(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.
Updated•8 years ago
|
Flags: qe-verify?
Priority: P2 → --
Whiteboard: [ohnoreflow][qf][photon-performance] → [ohnoreflow][qf]
Comment 3•8 years ago
|
||
(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.
Comment 4•8 years ago
|
||
(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
Comment 5•8 years ago
|
||
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
Comment 6•8 years ago
|
||
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.
Comment 7•8 years ago
|
||
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]
Updated•8 years ago
|
Status: REOPENED → NEW
Updated•8 years ago
|
Flags: qe-verify? → qe-verify-
Comment 8•8 years ago
|
||
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.
Assignee | ||
Comment 9•8 years ago
|
||
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.
Comment 10•8 years ago
|
||
I think ehsan added a fast path for window.scrollTo(0, 0) in bug 1365830. Can we use that here instead?
Comment 11•8 years ago
|
||
(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]
Updated•8 years ago
|
Priority: P2 → P3
Whiteboard: [ohnoreflow][qf:p1] [photon-performance] → [ohnoreflow][qf:p1] [reserve-photon-performance]
Assignee | ||
Comment 12•8 years ago
|
||
Attachment #8871032 -
Flags: review?(ehsan)
Assignee | ||
Updated•8 years ago
|
Assignee: nobody → bzbarsky
Status: NEW → ASSIGNED
Assignee | ||
Comment 13•8 years ago
|
||
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)
Assignee | ||
Comment 14•8 years ago
|
||
Attachment #8871035 -
Flags: review?(ehsan)
Assignee | ||
Comment 15•8 years ago
|
||
Attachment #8871036 -
Flags: review?(ehsan)
Updated•8 years ago
|
Priority: P3 → P1
Updated•8 years ago
|
Attachment #8867543 -
Attachment is obsolete: true
Comment 16•8 years ago
|
||
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 17•8 years ago
|
||
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+
Updated•8 years ago
|
Attachment #8871035 -
Flags: review?(ehsan) → review+
Comment 18•8 years ago
|
||
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+
Assignee | ||
Comment 19•8 years ago
|
||
Assignee | ||
Comment 20•8 years ago
|
||
Assignee | ||
Updated•8 years ago
|
Attachment #8871032 -
Attachment is obsolete: true
Attachment #8871332 -
Flags: review?(ehsan)
Comment 21•8 years ago
|
||
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+
Comment 22•8 years ago
|
||
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
Comment 23•8 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/3c315bd6cf00
https://hg.mozilla.org/mozilla-central/rev/e670c0dcaa9f
https://hg.mozilla.org/mozilla-central/rev/dd1c5aecc373
Status: ASSIGNED → RESOLVED
Closed: 8 years ago → 8 years ago
status-firefox55:
--- → fixed
Flags: in-testsuite+
Resolution: --- → FIXED
Target Milestone: --- → mozilla55
Updated•8 years ago
|
Iteration: --- → 55.6 - May 29
Updated•7 years ago
|
Depends on: CVE-2018-5100
Updated•6 years ago
|
Component: DOM → DOM: Core & HTML
Updated•3 years ago
|
Updated•3 years ago
|
Performance Impact: --- → P1
Whiteboard: [ohnoreflow][qf:p1] [reserve-photon-performance] → [ohnoreflow] [reserve-photon-performance]
You need to log in
before you can comment on or make changes to this bug.
Description
•