Closed Bug 1354194 Opened 8 years ago Closed 8 years ago

Make it easy to write synchronous reflow tests for key interactions

Categories

(Firefox :: General, enhancement, P1)

Product:
Firefox
Component:
General
Type:
enhancement

Tracking

()

Status:
RESOLVED FIXED
Milestone:
Firefox 55
Iteration:
55.6 - May 29
Project Flags:
Performance Impact none
Tracking Flags:
Tracking Status
firefox55 --- fixed

People

(Reporter: mconley, Assigned: mconley)

References

(Depends on 1 open bug, Blocks 1 open bug)

Details

(Whiteboard: [photon-performance])

Attachments

(4 files, 1 obsolete file)

Bug 1354194 - Add utility method for more easily writing reflow tests.
59 bytes, text/x-review-board-request
florian
: review+
Details
Bug 1354194 - Make browser_tabopen_reflows.js use the new reflow test helper, and make the whitelist reflect reality.
59 bytes, text/x-review-board-request
florian
: review+
Details
Bug 1354194 - Make browser_windowopen_reflows.js use the reflow test helper and make the whitelist reflect reality.
59 bytes, text/x-review-board-request
florian
: review+
Details
Bug 1354194 - Add reflow regression tests for tab closing.
59 bytes, text/x-review-board-request
florian
: review+
Details
There are key user interactions in the browser that we want to always be smooth. Synchronous reflow and style flushes are performance cliffs that can work against us. We should add tests for the key interactions to record how many sync reflows and style flushes we already do, and then try to drive that number to 0 (and ensure that number never grows). We have tests for the tab open and window open cases in browser/base/content/test/general/browser_tabopen_reflows.js and browser/base/content/test/general/browser_windowopen_reflows.js That's a great start. sfoster is working on a sync style flush test for window focus switching in bug 1334642. That's a great help too. This bug is about writing tests like this for all[1] of our key interactions. [1]: Where practical, naturally.
Our key interactions are: Tabs: * Open tab * Open tab causing tab squeeze * Open tab in an overflowed tab bar * Close tab * Close tab causing tab un-squeeze * Close tab in an overflowed tab bar * Switch tab * Entering the tab overflow state * Exiting the tab overflow state Windows: * Opening a window * Closing a window * Opening the first browser window While we're at it, there are probably a few others we should try to get in as well: * Focusing the URL bar * Typing in the URL bar * Opening the default menus (hamburger, downloads, bookmarks, Pocket, etc)
While we should absolutely do this, test issues are not gonna be tracked for qf.
Whiteboard: [qf] → [qf-]
Whiteboard: [qf-] → [photon-performance][qf-]
Flags: qe-verify?
Priority: -- → P2
Depends on: 1356271
Flags: qe-verify? → qe-verify-
Depends on: 1356684
See Also: 1334642 →
Assignee: nobody → mconley
Status: NEW → ASSIGNED
Iteration: --- → 55.5 - May 15
Priority: P2 → P1
Attachment #8863910 - Attachment is obsolete: true
Attachment #8863911 - Flags: review?(mconley)
Attachment #8863911 - Flags: review?(mconley)
I think I'm going to scope this bug down now to the following things: 1) Make it easier to write layout flush tests with a helper function that is more strict[1] than the current approach 2) Consolidate our current style and layout flush tests into the same folder 3) Re-write our current tests to use the helper function 4) Add one new test (window opening) that uses the helper function [1]: By "more strict" I mean, do our darndest to make the reflows that occur during the test to be as deterministic as possible. This means instrumenting the browser to dirty the layout tree as often as possible (ie, the worst-case-scenario). By being this strict, we can have a strict set of reflows that we expect, so that if one goes away unexpectedly, the test will fail (with a message saying that the stack should be removed from the whitelist). I'll file a follow-up bug to write more reflow tests, and then look into making style flush testing easier.
Summary: Write synchronous reflow and synchronous style flush tests for key interactions → Make it easy to write synchronous reflow tests for key interactions
Blocks: 1363505
Blocks: 1363507
No longer depends on: 1356271
Comment on attachment 8865856 [details] Bug 1354194 - Make browser_windowopen_reflows.js use the reflow test helper and make the whitelist reflect reality. https://reviewboard.mozilla.org/r/137462/#review140754 ::: browser/base/content/test/performance/browser_windowopen_reflows.js:3 (Diff revision 3) > -/* Any copyright is dedicated to the Public Domain. > - http://creativecommons.org/publicdomain/zero/1.0/ */ > - > "use strict"; > > +/** Note to self - we can probably re-enable this test on OS X now (since it's currently disabled in browser.ini).
Comment on attachment 8865856 [details] Bug 1354194 - Make browser_windowopen_reflows.js use the reflow test helper and make the whitelist reflect reality. https://reviewboard.mozilla.org/r/137462/#review140760 ::: browser/base/content/test/performance/browser_windowopen_reflows.js (Diff revision 3) > - // Focusing the content area causes a reflow. > - "_delayedStartup@chrome://browser/content/browser.js|", > - > - // Sometimes sessionstore collects data during this test, which causes a sync reflow > - // (https://bugzilla.mozilla.org/show_bug.cgi?id=892154 will fix this) > - "ssi_getWindowDimension@resource:///modules/sessionstore/SessionStore.jsm", I took care of this one in https://hg.mozilla.org/integration/mozilla-inbound/rev/aa210d7d1b43
Comment on attachment 8865856 [details] Bug 1354194 - Make browser_windowopen_reflows.js use the reflow test helper and make the whitelist reflect reality. https://reviewboard.mozilla.org/r/137462/#review140760 > I took care of this one in https://hg.mozilla.org/integration/mozilla-inbound/rev/aa210d7d1b43 Nicely done! :D
Comment on attachment 8865856 [details] Bug 1354194 - Make browser_windowopen_reflows.js use the reflow test helper and make the whitelist reflect reality. https://reviewboard.mozilla.org/r/137462/#review140776 ::: browser/base/content/test/performance/browser_windowopen_reflows.js:56 (Diff revision 3) > /* > * This test ensures that there are no unexpected > * uninterruptible reflows when opening new windows. > */ > -function test() { > - waitForExplicitFinish(); > +add_task(function*() { > + // Add a reflow observer and open a new tab. Nit: probably can remove this comment.
(In reply to Mike Conley (:mconley) - At work week, slow to respond from comment #23) > Comment on attachment 8865856 [details] > Bug 1354194 - Make browser_windowopen_reflows.js use the reflow test helper > and make the whitelist reflect reality. > > https://reviewboard.mozilla.org/r/137462/#review140760 > > > I took care of this one in https://hg.mozilla.org/integration/mozilla-inbound/rev/aa210d7d1b43 > > Nicely done! :D In case you want to rebase, this is now on central.
(In reply to Dão Gottwald [::dao] from comment #25) > In case you want to rebase, this is now on central. Not sure I understand - in the patch, I _removed_ the ssi_getWindowDimension reflow from the whitelist, since it's not being hit anymore. Or am I missing something?
Flags: needinfo?(dao+bmo)
(In reply to Mike Conley (:mconley) - At work week, slow to respond from comment #26) > (In reply to Dão Gottwald [::dao] from comment #25) > > In case you want to rebase, this is now on central. > > Not sure I understand - in the patch, I _removed_ the ssi_getWindowDimension > reflow from the whitelist, since it's not being hit anymore. > > Or am I missing something? I already removed it from the whitelist earlier today.
Flags: needinfo?(dao+bmo)
Comment on attachment 8863911 [details] Bug 1354194 - Add utility method for more easily writing reflow tests. https://reviewboard.mozilla.org/r/135638/#review142246 (I didn't get time to do a full review before my flight started boarding, but publishing now as I figured you may be interested in the comments I already had) ::: browser/base/content/test/performance/head.js:78 (Diff revision 5) > + * is passed) > + * Callers that pass dirtyFrameFn must pass this clean up function to > + * return the DOM back to its normal state so that future tests don't > + * inherit a strangely styled browser. > + */ > +async function withReflowObserver(window, expectedStacks, testFn, dirtyFrameFn, dirtyFrameCleanUp) { I think head.js runs in the JS context of the browser window, so 'window' is already defined; do you really want to shadow it? Feel free to ignore that comment if it's causing a lot of work to address: I think testFn is the only non-optional parameter here, so should it be first? I guess 'window' should default to err... window? nit: wrap this long line? ::: browser/base/content/test/performance/head.js:87 (Diff revision 5) > + } > + > + if (!dirtyFrameFn) { > + // The default dirty frame function adds an increasing margin > + // to the first element child of the window document to a maximum > + // of 15px, and then goes back to 0. You have "% 4" in the code, so I guess the maximum is 3px rather than 15. ::: browser/base/content/test/performance/head.js:105 (Diff revision 5) > + let els = Cc["@mozilla.org/eventlistenerservice;1"] > + .getService(Ci.nsIEventListenerService); > + > + // We're going to remove the stacks one by one as we see them so that > + // we can check for expected, unseen reflows, so let's clone the array. > + expectedStacks = expectedStacks.slice(0); Shouldn't expectedStacks default to an empty array?
Comment on attachment 8865856 [details] Bug 1354194 - Make browser_windowopen_reflows.js use the reflow test helper and make the whitelist reflect reality. https://reviewboard.mozilla.org/r/137462/#review142250 ::: browser/base/content/test/performance/browser_windowopen_reflows.js (Diff revision 4) > -/* Any copyright is dedicated to the Public Domain. > - http://creativecommons.org/publicdomain/zero/1.0/ */ Last time we discussed whether tests should have public domain headers, Gerv said having them was preferable, so let's not remove them. ::: browser/base/content/test/performance/browser_windowopen_reflows.js:55 (Diff revision 4) > > /* > * This test ensures that there are no unexpected > * uninterruptible reflows when opening new windows. > */ > -function test() { > +add_task(function*() { At this point we shouldn't introduce new generator functions as task in browser/. Please switch to async/await, and sorry I didn't get to this review early enough to avoid bitrot :(.
Iteration: 55.5 - May 15 → 55.6 - May 29
Attachment #8863911 - Flags: review?(florian)
Attachment #8863912 - Flags: review?(florian)
Attachment #8865856 - Flags: review?(florian)
Attachment #8865857 - Flags: review?(florian)
Comment on attachment 8863911 [details] Bug 1354194 - Add utility method for more easily writing reflow tests. https://reviewboard.mozilla.org/r/135638/#review143558 ::: browser/base/content/test/performance/head.js:9 (Diff revision 7) > + * > + * The helper works by running a JS function before each event is > + * dispatched that attempts to dirty the layout tree - the idea being > + * that this puts us in the "worst case scenario" so that any JS > + * that attempts to query for layout or style information will cause > + * a reflow to fire. We also dirty the layout tree after each reflow Is this enough to catch sync reflows by code running from timers/requestIdleCallback/dispatchToMainThread? ::: browser/base/content/test/performance/head.js:74 (Diff revision 7) > + * Callers can provide a custom DOM node to change some layout style > + * on in the event that the action being tested is occurring within > + * a scrollable frame. Otherwise, withReflowObserver defaults to dirtying > + * the first element child of the window. > + */ > +async function withReflowObserver(testFn, expectedStacks = [], win, elemToDirty) { I think you missed " = window" after "win". ::: browser/base/content/test/performance/head.js:102 (Diff revision 7) > + }).join("|"); > + > + let pathWithLineNumbers = (new Error().stack).split("\n").slice(1); > + > + // Just in case, dirty the frame now that we've reflowed. > + elemToDirty.style.margin = ""; I think this should be dirtyFrameFn();, or you won't get any result when the margin was already 0. ::: browser/base/content/test/performance/head.js:105 (Diff revision 7) > + > + // Just in case, dirty the frame now that we've reflowed. > + elemToDirty.style.margin = ""; > + > + // Stack trace is empty. Reflow was triggered by native code, which > + // we ignore. Is this the right thing to do? If some native code was to introduce an uninterruptible reflow within one of the interactions we care about, we would want to know (and get that patch backed out), right? Could be material for a follow-up. ::: browser/base/content/test/performance/head.js:149 (Diff revision 7) > + "This is probably a good thing - just remove it from the " + > + "expected list."); > + } > + > + els.removeListenerForAllEvents(win, dirtyFrameFn, true); > + docShell.removeWeakReflowObserver(observer); Should we return elemToDirty.style.margin to its original value here?
Attachment #8863911 - Flags: review?(florian) → review+
Comment on attachment 8863912 [details] Bug 1354194 - Make browser_tabopen_reflows.js use the new reflow test helper, and make the whitelist reflect reality. https://reviewboard.mozilla.org/r/135640/#review143566 ::: browser/base/content/test/performance/browser_tabopen_reflows.js:111 (Diff revision 8) > - > -function waitForTransitionEnd() { > - return new Promise(resolve => { > - let tab = gBrowser.selectedTab; > + let tab = gBrowser.selectedTab; > - tab.addEventListener("transitionend", function onEnd(event) { > - if (event.propertyName === "max-width") { > + await BrowserTestUtils.removeTab(gBrowser.selectedTab, { animate: true }); > + await BrowserTestUtils.waitForCondition(() => tab.parentNode == null); What is this code doing? Why do we need anything more than just removing the tab we added?
Attachment #8863912 - Flags: review?(florian) → review+
Comment on attachment 8865856 [details] Bug 1354194 - Make browser_windowopen_reflows.js use the reflow test helper and make the whitelist reflect reality. https://reviewboard.mozilla.org/r/137462/#review143594
Attachment #8865856 - Flags: review?(florian) → review+
Comment on attachment 8865857 [details] Bug 1354194 - Add reflow regression tests for tab closing. https://reviewboard.mozilla.org/r/137464/#review143570 ::: browser/base/content/test/performance/browser_tabclose_reflows.js:31 (Diff revision 7) > +/* > + * This test ensures that there are no unexpected > + * uninterruptible reflows when closing new tabs. > + */ > +add_task(async function() { > + // If we've got a preloaded browser, get rid of it so that it Feel free to ignore this comment, but I wonder if this workaround related to aboutnewtab should go to head.js I guess it depends if you expect to need it for more tests soon.
Attachment #8865857 - Flags: review?(florian) → review+
Comment on attachment 8863911 [details] Bug 1354194 - Add utility method for more easily writing reflow tests. https://reviewboard.mozilla.org/r/135638/#review143558 > Is this enough to catch sync reflows by code running from timers/requestIdleCallback/dispatchToMainThread? I'm afraid not. :( Perhaps good fodder for a follow-up. I'll add a TODO to file one once this bug lands. > Is this the right thing to do? If some native code was to introduce an uninterruptible reflow within one of the interactions we care about, we would want to know (and get that patch backed out), right? > > Could be material for a follow-up. If there's no JS on the stack, I'm pretty sure that means that the cause of the reflow was a refresh driver tick, unless there's another case here that you're thinking of?
Comment on attachment 8863912 [details] Bug 1354194 - Make browser_tabopen_reflows.js use the new reflow test helper, and make the whitelist reflect reality. https://reviewboard.mozilla.org/r/135640/#review143566 > What is this code doing? Why do we need anything more than just removing the tab we added? Ah, this is a leftover from when I was running this test on repeat with --run-until-failure. There are cases where after calling removeTab, the tab will continue to exist in the DOM even after the removeTab Promise resolves, which can mess up the next run of the test. Doesn't seem to affect things when the tests are run in sequence, which is how it's done in automation. I'll file a follow-up to change BrowserTestUtils.removeTab to ensure that the DOM node is removed.
ni? to florian for comment 49.
Flags: needinfo?(florian)
(In reply to Mike Conley (:mconley) from comment #49) > If there's no JS on the stack, I'm pretty sure that means that the cause of > the reflow was a refresh driver tick, unless there's another case here that > you're thinking of? I was somehow assuming that reflow observers were not notified for reflows caused by the refresh driver tick. I don't have a specific case in mind, I was just thinking that cases like webNavigation.stop() (which could be called from C++) or moving the focus (eg. from activating the window) should be reported by the tests. (In reply to Mike Conley (:mconley) from comment #50) > I'll file a follow-up to change BrowserTestUtils.removeTab to ensure that > the DOM node is removed. Exactly what I was going to suggest :-).
Flags: needinfo?(florian)
(In reply to Florian Quèze [:florian] [:flo] from comment #56) > (In reply to Mike Conley (:mconley) from comment #49) > > > If there's no JS on the stack, I'm pretty sure that means that the cause of > > the reflow was a refresh driver tick, unless there's another case here that > > you're thinking of? > > I was somehow assuming that reflow observers were not notified for reflows > caused by the refresh driver tick. > > I don't have a specific case in mind, I was just thinking that cases like > webNavigation.stop() (which could be called from C++) or moving the focus > (eg. from activating the window) should be reported by the tests. > Ah, those. :( I wonder if there's a way to supply a "reason" to the nsIReflowObserver. That will be the follow-up bug.
Pushed by mconley@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/0428e82ccdaa Add utility method for more easily writing reflow tests. r=florian https://hg.mozilla.org/integration/autoland/rev/a6d09aa52cb5 Make browser_tabopen_reflows.js use the new reflow test helper, and make the whitelist reflect reality. r=florian https://hg.mozilla.org/integration/autoland/rev/d537626e1a10 Make browser_windowopen_reflows.js use the reflow test helper and make the whitelist reflect reality. r=florian https://hg.mozilla.org/integration/autoland/rev/cea53d87b4d5 Add reflow regression tests for tab closing. r=florian
Depends on: 1367941
Blocks: 1367797
Filed bug 1368132 to deal with the timer/requestIdleCallback/dispatchToMainThread case. Filed bug 1368131 to check that the tab is fully removed in BrowserTestUtils.removeTab.
Depends on: 1368132
Blocks: 1369491
Performance Impact: --- → -
Whiteboard: [photon-performance][qf-] → [photon-performance]
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: