Closed Bug 1488871 Opened 6 years ago Closed 5 years ago

AsyncScrollPortEvent as an WillPaintObserver causes extra layout flushes during page load

Categories

(Core :: Layout, enhancement)

Product:
Core
Component:
Layout
Type:
enhancement
Priority:
Not set
Severity:
normal

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla69
Project Flags:
Performance Impact high
Tracking Flags:
Tracking Status
firefox69 --- fixed

People

(Reporter: smaug, Assigned: emilio)

References

(Blocks 2 open bugs)

Details

(Keywords: perf:pageload)

Attachments

(4 files)

Bug 1488871 - Don't flush layout in AsyncScrollPortEvent::Run. r=mats
(deleted), text/x-phabricator-request
MatsPalmgren_bugz
: review+
Details
Bug 1488871 - Make browser_windowopen.js workaround account for the toolbar background. r=mconley,florian
(deleted), text/x-phabricator-request
florian
: review+
mconley
: review+
Details
Bug 1488871 - Add an explicit style flush to make sure the added textbox and popupset is styled before proceeding tests in browser_UrlbarInput_unit.js. r?adw
(deleted), text/x-phabricator-request
Details
Bug 1488871 - Add an exception for the placeholder in the urlbar flickers to browser_windowopen.js. r?mconley
(deleted), text/x-phabricator-request
Details
I was profiling https://www.nationalgeographic.com/ page load and saw odd looking layout flush coming from RunWillPaintObservers. Only user of that API is https://searchfox.org/mozilla-central/rev/5a18fb5aeeec99f1ca1c36a697082c221189a3b9/layout/generic/nsGfxScrollFrame.cpp#5273
Whiteboard: [qf]
Blocks: 696248
Other browsers don't seem to support the scrollport events. Can we make them chromeonly / remove them?
Flags: needinfo?(bugs)
That I don't know. They are a layout feature. Do we use them somewhere, especially, do we use them in child process?
Flags: needinfo?(bugs)
Looks like devtools uses it, so should be feasible to remove... That being said, that flush there looks a bit hackish. It's needed so the scrollport size and such is the correct one at the time Run() runs, but actually the rect we should be interested in is the one that we had after reflow from PostOverflowEvent (and we should always fire the event I think, unconditionally on what the current state of the frame is). I could write a patch for that tomorrow, I think.
Flags: needinfo?(emilio)
Asynchronousness and flushing is there to avoid extra events, no?
(In reply to Olli Pettay [:smaug] from comment #4) > Asynchronousness and flushing is there to avoid extra events, no? Maybe? Oh, I guess we reflow the scroll frame way too often to check for different scrollbar combinations... But probably reflowing is not needed if we stash the relevant rects in the AsyncScrollPortEvent.
This is the overflow/underflow events, right? WebKit used to support an "overflowchanged" event - did they remove that? If not, then I suspect removing our events is likely to cause web-compat issues.
(In reply to Mats Palmgren (:mats) from comment #6) > This is the overflow/underflow events, right? > WebKit used to support an "overflowchanged" event - did they remove that? Blink did: https://bugs.chromium.org/p/chromium/issues/detail?id=460822
I'm pleasantly surprised, that's great! Then I think we can remove ours too (assuming Blink haven't introduced some new feature for this). The dev guides I found on the issue, like: http://www.backalleycoder.com/2013/03/14/oft-overlooked-overflow-and-underflow-events/ relies on overflowchanged *or* overflow/underflow so if the former is now broken then it seems the web-compat risk is low. ("intranet apps" being the dark horse as usual) We should announce an intent-to-remove on this though.
I'd claim that we don't need it because, in order to enqueue the event, we already need to have overflowed the event in a normal reflow. For now this should not break anything (or anything that wasn't already racy depending on when we paint). The only reason the flush is there is according to roc is to decide whether to fire the event, and because it needs the layout information: https://bugzilla.mozilla.org/show_bug.cgi?id=771822#c4 In practice, however, all the layout information we need we have already computed by the time we post the event. We don't expose the rects via the event details, which is what could get out-of-date, so this patch could only mean that we fire the event slightly more often in cases where people remove stuff from the DOM, right after we do layout and the content has overflowed. But that's actually pretty unlikely. This event in general is pretty problematic because it exposes when we do layout and when we paint, which is not great. Its test coverage is also pretty low (test_overflow_event.html, which of course still passes without this). I still want to do this change first since it's trivial to back out if needed. Then I'd want to change how it fires to match the scrolled area change event (which would allow us to remove the WillPaintObserver stuff), after verifying that chrome consumers are still fine with that, and then put behind a pref and hide it from content, while we leave time for chrome consumers to migrate away from it, and allow us to revert if something breaks. Does this sound reasonable? If so, I'll file bugs for that with patches.
Assignee: nobody → emilio
Flags: needinfo?(emilio)
Comment on attachment 9006702 [details] Bug 1488871 - Don't flush layout in AsyncScrollPortEvent::Run. r=mats Mats Palmgren (:mats) has approved the revision.
Attachment #9006702 - Flags: review+
Attachment #9006702 - Attachment description: Remove the flush. → Don't flush layout in AsyncScrollPortEvent::Run.
Pushed by emilio@crisal.io: https://hg.mozilla.org/integration/autoland/rev/fc5ee8a04645 Don't flush layout in AsyncScrollPortEvent::Run. r=mats
Blocks: 1488953
Blocks: 1488955
https://hg.mozilla.org/mozilla-central/rev/fc5ee8a04645
Status: NEW → RESOLVED
Closed: 6 years ago
status-firefox64: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla64
Backout by ebalazs@mozilla.com: https://hg.mozilla.org/mozilla-central/rev/1552d735c49f Backed out changeset fc5ee8a04645 for causing perma bc failures in browser/base/content/test/performance/browser_windowopen.js. a=backout
(In reply to Pulsebot from comment #13) > Backout by ebalazs@mozilla.com: > https://hg.mozilla.org/mozilla-central/rev/1552d735c49f > Backed out changeset fc5ee8a04645 for causing perma bc failures in > browser/base/content/test/performance/browser_windowopen.js. a=backout Would've been nice to reopen the bug. Do you have a link to the failure?
Status: RESOLVED → REOPENED
Flags: needinfo?(ebalazs)
Resolution: FIXED → ---
Flags: needinfo?(emilio)
With the other patch in this bug, we get two rects, not > 5. See: https://treeherder.mozilla.org/#/jobs?repo=autoland&revision=fc5ee8a046456b4d2affd2142954ea8e3b685322&filter-resultStatus=testfailed&filter-resultStatus=busted&filter-resultStatus=exception&filter-classifiedState=unclassified&selectedJob=197787808 It's not clear if this workaround is getting hit on other builds other than OSX debug, probably I could try to make this more specific... I'll fire a couple try runs with the condition changed.
Flags: needinfo?(emilio)
Comment on attachment 9007152 [details] Bug 1488871 - Make browser_windowopen.js workaround account for the toolbar background. r=mconley,florian Florian Quèze [:florian] has approved the revision.
Attachment #9007152 - Flags: review+
Comment on attachment 9007152 [details] Bug 1488871 - Make browser_windowopen.js workaround account for the toolbar background. r=mconley,florian Mike Conley (:mconley) (:⚙️) has approved the revision.
Attachment #9007152 - Flags: review+
Pushed by emilio@crisal.io: https://hg.mozilla.org/integration/autoland/rev/39b3a22e2552 Don't flush layout in AsyncScrollPortEvent::Run. r=mats https://hg.mozilla.org/integration/autoland/rev/078929426a6e Make browser_windowopen.js workaround account for the toolbar background. r=mconley,florian
https://hg.mozilla.org/mozilla-central/rev/39b3a22e2552 https://hg.mozilla.org/mozilla-central/rev/078929426a6e
Status: REOPENED → RESOLVED
Closed: 6 years ago → 6 years ago
status-firefox64: --- → fixed
Resolution: --- → FIXED
Backed out for failures on browser_windowopen.js backout: https://hg.mozilla.org/integration/autoland/rev/c7209352c57a979b096a9e1b08d74372c490cd57 push with failures: https://treeherder.mozilla.org/#/jobs?repo=autoland&revision=078929426a6e1f65e0526b526ceb2bb8fcdc7124&filter-resultStatus=testfailed&filter-resultStatus=busted&filter-resultStatus=exception&filter-classifiedState=unclassified&selectedJob=198129458 failure log: https://treeherder.mozilla.org/logviewer.html#?job_id=198129461&repo=autoland&lineNumber=1924 13:33:39 INFO - Console message: OpenGL compositor Initialized Succesfully. 13:33:39 INFO - Version: 2.1 INTEL-10.6.33 13:33:39 INFO - Vendor: Intel Inc. 13:33:39 INFO - Renderer: Intel Iris OpenGL Engine 13:33:39 INFO - FBO Texture Target: TEXTURE_2D 13:33:39 INFO - Buffered messages logged at 13:33:39 13:33:39 INFO - TEST-FAIL | browser/base/content/test/performance/browser_windowopen.js | known reflow at rect@chrome://browser/content/browser-tabsintitlebar.js was encountered 4 times - 13:33:39 INFO - Full stack: 13:33:39 INFO - rect@chrome://browser/content/browser-tabsintitlebar.js:173:23 13:33:39 INFO - _layOutTitlebar@chrome://browser/content/browser-tabsintitlebar.js:209:26 13:33:39 INFO - update@chrome://browser/content/browser-tabsintitlebar.js:162:5 13:33:39 INFO - whenWindowLayoutReady@chrome://browser/content/browser-tabsintitlebar.js:46:5 13:33:39 INFO - onDOMContentLoaded@chrome://browser/content/browser.js:1285:5 13:33:39 INFO - EventListener.handleEvent*@chrome://browser/content/browser.xul:114:3 13:33:39 INFO - 13:33:39 INFO - Full stack: 13:33:39 INFO - rect@chrome://browser/content/browser-tabsintitlebar.js:173:23 13:33:39 INFO - _layOutTitlebar@chrome://browser/content/browser-tabsintitlebar.js:212:34 13:33:39 INFO - update@chrome://browser/content/browser-tabsintitlebar.js:162:5 13:33:39 INFO - whenWindowLayoutReady@chrome://browser/content/browser-tabsintitlebar.js:46:5 13:33:39 INFO - onDOMContentLoaded@chrome://browser/content/browser.js:1285:5 13:33:39 INFO - EventListener.handleEvent*@chrome://browser/content/browser.xul:114:3 13:33:39 INFO - 13:33:39 INFO - Full stack: 13:33:39 INFO - rect@chrome://browser/content/browser-tabsintitlebar.js:173:23 13:33:39 INFO - _layOutTitlebar@chrome://browser/content/browser-tabsintitlebar.js:216:31 13:33:39 INFO - update@chrome://browser/content/browser-tabsintitlebar.js:162:5 13:33:39 INFO - whenWindowLayoutReady@chrome://browser/content/browser-tabsintitlebar.js:46:5 13:33:39 INFO - onDOMContentLoaded@chrome://browser/content/browser.js:1285:5 13:33:39 INFO - EventListener.handleEvent*@chrome://browser/content/browser.xul:114:3 13:33:39 INFO - 13:33:39 INFO - Full stack: 13:33:39 INFO - rect@chrome://browser/content/browser-tabsintitlebar.js:173:23 13:33:39 INFO - _layOutTitlebar@chrome://browser/content/browser-tabsintitlebar.js:228:33 13:33:39 INFO - update@chrome://browser/content/browser-tabsintitlebar.js:162:5 13:33:39 INFO - whenWindowLayoutReady@chrome://browser/content/browser-tabsintitlebar.js:46:5 13:33:39 INFO - onDOMContentLoaded@chrome://browser/content/browser.js:1285:5 13:33:39 INFO - EventListener.handleEvent*@chrome://browser/content/browser.xul:114:3 13:33:39 INFO - 13:33:39 INFO - TEST-PASS | browser/base/content/test/performance/browser_windowopen.js | 0 unexpected reflows - true == true - 13:33:39 INFO - comparing 3 frames 13:33:39 INFO - Buffered messages finished 13:33:39 INFO - TEST-UNEXPECTED-FAIL | browser/base/content/test/performance/browser_windowopen.js | unexpected changed rect: ({x1:0, x2:105, y1:0, y2:32, w:106, h:33}), window width: 1280 - 13:33:39 INFO - Stack trace: 13:33:39 INFO - chrome://mochitests/content/browser/browser/base/content/test/performance/head.js:reportUnexpectedFlicker/rects<:573 13:33:39 INFO - chrome://mochitests/content/browser/browser/base/content/test/performance/head.js:reportUnexpectedFlicker:564 13:33:39 INFO - chrome://mochitests/content/browser/browser/base/content/test/performance/head.js:withPerfObserver:632 13:33:39 INFO - chrome://mochitests/content/browser/browser/base/content/test/performance/browser_windowopen.js:null:109 13:33:39 INFO - chrome://mochikit/content/browser-test.js:Tester_execTest/<:1102 13:33:39 INFO - chrome://mochikit/content/browser-test.js:Tester_execTest:1093 13:33:39 INFO - chrome://mochikit/content/browser-test.js:nextTest/<:995 13:33:39 INFO - chrome://mochikit/content/tests/SimpleTest/SimpleTest.js:SimpleTest.waitForFocus/waitForFocusInner/focusedOrLoaded/<:795 13:33:39 INFO - Not taking screenshot here: see the one that was previously logged
Flags: needinfo?(emilio)
Looks like the test failure was resolved.
Flags: needinfo?(emilio)
Nope, this was backed out but somehow not reopened... I still need to land this.
Status: RESOLVED → REOPENED
Flags: needinfo?(emilio)
Resolution: FIXED → ---
status-firefox64: fixed → ---
Target Milestone: mozilla64 → ---
Whiteboard: [qf] → [qf:p1:f65]

Hi Emilio! Just want to be sure this doesn't fall too far off your radar -- is this likely as simple as just re-landing (per comment 22)?

(I guess some rebasing may be needed, but hopefully this is set to go otherwise?)

Whiteboard: [qf:p1:f65] → [qf:p1:pageload]

Here is a try run based on the latest trunk with the patch for bug 1488953.
https://treeherder.mozilla.org/#/jobs?repo=try&revision=8028e81e5c707b4383705e4b59086a43704afdec

It's mostly green on most platforms, but there are three failures, performance/browser_windowopen.js is timed out on Win10 x64 ASAN, browser_UrlbarInput_unit.js fails on Windows 7 and performance/browser_tabswitch.js fails on Windows 7. So I suspect there is something Windows 7 specific issue, but I no longer have any Windows 7 machines.

browser_UrlbarInput_unit.js creates a mock of the UrlbarInput, and it seems that it relies on the flush we are going to drop in this bug. Adding an explicit flush fixed the failure.

Drew, does adding the explicit flush sound reasonable?

Flags: needinfo?(adw)

Yes, it doesn't seem like that's related to what the test is testing, so that should be OK.

Flags: needinfo?(adw)

Oh could you include this bug number in the code comment please? Makes it a little easier to understand why it's there than searching through blame.

Thanks Drew! I will add some comments there and send a review request to you soon.

(Though in this bug there are other issues need to be solved)

Maybe the test failures will be disappeared by bug 1513325. CCing :ntim.

Doubtful, the urlbar already uses <html:input>

I did push a new try;
https://treeherder.mozilla.org/#/jobs?repo=try&revision=73561caefc2529469e9f314acaaa814bcfa983ca&selectedJob=248154218

The failure on Windows 7 have been passed for some reasons, instead browser_windowopen.js failed both on Windows 10/7 opt builds.

After poking around relevant places, I noticed that withPerfObserver doesn't work as expected, doesn't work as what browser normally does, because recordFrames calls drawWindow to take a screenshot and the drawWindow ends up calling FlushWillPaintObservers, thus it calls AsyncScrollPortEvent::Run then it calls FlushPendingNotifications which is exactly what this bug is going to drop. So, we inadvertently invokes an additional flush in withPerfObserver and the flush has been wallpapering some flickers which have been appeared (but unnoticeable I guess) on normal browsing.

As a proof of the wallpapering, I did push another try with skipping the FlushWillPaintObversers calls (and without any changes for this bug) when the snapshot is taken in recordFrames.
https://treeherder.mozilla.org/#/jobs?repo=try&revision=4f955c05730c934016f5dae4da642bc8a6a18136&selectedJob=248918813

browser_windowopen.js actually failed and the result is pretty much the same as the failure with the changes for this bug.

mconley, does this sound a reasonable reason to add an exception in browser_windowopen.js test? (And I suppose the placeholder text movement is caused by the same root cause)

A note for Emilio : I didn't include the patch for bug 1488953 in tries since it causes another failure I am going to tackle the failures one by one.

Flags: needinfo?(mconley)

If I understand correctly, we were always shipping this flicker, and for the reasons you identified, we weren't recording it properly for this test.

If that's the case, then yes, I think it makes sense to add an exception.

Flags: needinfo?(mconley)

Thank you! I filed bug 1555842 for further investigation what the root cause of the urlbar flicker.

recordFrames has been wallpapering these flickers because the function ends up
calling FlushPendingNotifications in AsyncScrollPortEvent::Run() and we hadn't
noticed the wallpaper until we tried to remove the FlushPendingNotifications in
AsyncScrollPortEvent::Run().

The browser_windowopen.js failure appeared on MacOSX and Linux opt builds too ([1] for example), and oddly it appeared on Linux ASAN build. (I am wondering why it doesn't fail on any debug builds?) Anyways, I am going to apply the exception in D33226 to those platforms.

[1] https://treeherder.mozilla.org/#/jobs?repo=try&resultStatus=testfailed%2Cbusted%2Cexception%2Cretry%2Cusercancel%2Crunning%2Cpending%2Crunnable&revision=41c7e1814ebdefe7546e6e588f54363c6a559d82&selectedJob=249505514

Attachment #9007152 - Attachment description: Fix browser_windowopen.js workaround. → Bug 1488871 - Make browser_windowopen.js workaround account for the toolbar background. r=mconley,florian
Attachment #9006702 - Attachment description: Don't flush layout in AsyncScrollPortEvent::Run. → Bug 1488871 - Don't flush layout in AsyncScrollPortEvent::Run. r=mats
Pushed by hikezoe@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/5f35e6fc8f91 Make browser_windowopen.js workaround account for the toolbar background. r=florian,mconley https://hg.mozilla.org/integration/autoland/rev/eddcd90c1ffa Add an exception for the placeholder in the urlbar flickers to browser_windowopen.js. r=mconley https://hg.mozilla.org/integration/autoland/rev/cfac49a18cf1 Don't flush layout in AsyncScrollPortEvent::Run. r=mats https://hg.mozilla.org/integration/autoland/rev/a132736ab17e Add an explicit style flush to make sure the added textbox and popupset is styled before proceeding tests in browser_UrlbarInput_unit.js. r=adw

Thanks hiro!

Flags: needinfo?(emilio)
Performance Impact: --- → P1
Keywords: perf:pageload
Whiteboard: [qf:p1:pageload]
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: