(Marco Bonardo [::mak] from bug 1353563 comment #12) > r=me with at least a bug filed to add a test measuring flushes for a simple > location bar search. It should be trivial using > promiseAutocompleteResultPopup

ah there is one pitfall here though, I think Mike said we can measure only synchronous operations, but most of the awesomebar is async :\ Maybe we could replace the awesomebar search component with a custom synchronous component.

Comment on attachment 8884937 [details] Bug 1356271 - add a test to measure how many layout flushes it takes for a simple location bar search. https://reviewboard.mozilla.org/r/155778/#review161604 Looks good, but I have 2 concerns here: - should we add the letters one by one using EventUtils.synthesizeKey(letter, {}); to simulate a user typing? I assume that would cause a lot more sync reflows. If you considered it but decided we should start doing that only once the existing whitelist has been reduced, that's fine. - this seems to rely on the default places content. I'm afraid that'll be different on different channels (eg. on beta or release I expect us to set different default bookmarks than on Nightly). I think this is likely to change the set of reflow stacks we are seeing (or most likely just change the count of the various stacks). ::: browser/base/content/test/performance/browser_urlbar_search_reflows.js:126 (Diff revision 2) > + "_reuseAcItem@chrome://global/content/bindings/autocomplete.xml", > + "_appendCurrentResult@chrome://global/content/bindings/autocomplete.xml", > + "_invalidate@chrome://global/content/bindings/autocomplete.xml", > + "invalidate@chrome://global/content/bindings/autocomplete.xml" > + ], > + times: 222, // This number should only ever go down - never up. :-o ::: browser/base/content/test/performance/browser_urlbar_search_reflows.js:139 (Diff revision 2) > + * @param win (browser window) > + * The window to do the search in. > + * @returns Promise > + */ > +async function promiseAutocompleteResultPopup(win) { > + win.gURLBar.controller.startSearch(win.gURLBar.value); nit: win.gURLBar is accessed 5 times in this function, I would put it in a variable. ::: browser/base/content/test/performance/browser_urlbar_search_reflows.js:161 (Diff revision 2) > + await ensureNoPreloadedBrowser(win); > + > + win.gURLBar.focus(); > + win.gURLBar.value = SEARCH_TERM; > + await withReflowObserver(async function(dirtyFrameFn) { > + let oldInvalidate = win.gURLBar.popup.invalidate.bind(win.gURLBar.popup); nit: let popup = win.gURLBar.popup; ::: browser/base/content/test/performance/browser_urlbar_search_reflows.js:164 (Diff revision 2) > + win.gURLBar.value = SEARCH_TERM; > + await withReflowObserver(async function(dirtyFrameFn) { > + let oldInvalidate = win.gURLBar.popup.invalidate.bind(win.gURLBar.popup); > + let oldResultsAdded = win.gURLBar.popup.onResultsAdded.bind(win.gURLBar.popup); > + > + win.gURLBar.popup.invalidate = (reason) => { why do we need to dirty the frame tree outside of the normal mechanism? (this should likely be a comment in the test)

Comment on attachment 8885307 [details] Bug 1356271 - Make it so that it's easier to define repeating reflows for reflow tests. https://reviewboard.mozilla.org/r/156164/#review161610 ::: browser/base/content/test/performance/browser_tabopen_reflows.js:19 (Diff revision 1) > */ > const EXPECTED_REFLOWS = [ > // selection change notification may cause querying the focused editor content > // by IME and that will cause reflow. > - [ > + { > + stack: [ is eslint happy with the multiple spaces between ':' and '[' here? Seems to be an accident. ::: browser/base/content/test/performance/browser_tabopen_squeeze_reflows.js:14 (Diff revision 1) > * See https://developer.mozilla.org/en-US/Firefox/Performance_best_practices_for_Firefox_fe_engineers > * for tips on how to do that. > */ > const EXPECTED_REFLOWS = [ > - [ > + { > + stack: [ same here ::: browser/base/content/test/performance/browser_windowopen_reflows.js:17 (Diff revision 1) > * See https://developer.mozilla.org/en-US/Firefox/Performance_best_practices_for_Firefox_fe_engineers > * for tips on how to do that. > */ > const EXPECTED_REFLOWS = [ > - [ > + { > + stack: [ and here ::: browser/base/content/test/performance/head.js:64 (Diff revision 1) > + * "_fillTrailingGap@chrome://browser/content/tabbrowser.xml", > + * "_handleNewTab@chrome://browser/content/tabbrowser.xml", > + * "onxbltransitionend@chrome://browser/content/tabbrowser.xml", > + * ], > + * // We expect this reflow to only happen once > + * times: 1, I would make 1 the default value and skip this line in tests for all the cases where a reflow happens only once.

Comment on attachment 8884937 [details] Bug 1356271 - add a test to measure how many layout flushes it takes for a simple location bar search. https://reviewboard.mozilla.org/r/155778/#review161604 > why do we need to dirty the frame tree outside of the normal mechanism? (this should likely be a comment in the test) This is because results are added to the popup via an XPCOM search observer callback that occurs outside of normal JS events (which is what `withReflowObserver` uses to dirty the frame tree). I'll add a comment.

Comment on attachment 8884937 [details] Bug 1356271 - add a test to measure how many layout flushes it takes for a simple location bar search. https://reviewboard.mozilla.org/r/155778/#review161604 I'll write a new test for the first case that you describe. As you say, I do expect this to result in a heck of a lot of reflows. For the second - that's a great catch. I'll insert some custom history entries instead.

(In reply to Mike Conley (:mconley) - Digging out of needinfo / review backlog. from comment #8) > For the second - that's a great catch. I'll insert some custom history > entries instead. I wonder if the reflows are the same for history entries, bookmark entries, and search suggestions. All of these may be possible to fake. Starting with only history entries sounds fine, and we can add more variations in follow-ups if needed.

Comment on attachment 8886256 [details] Bug 1356271 - Clean up comment about reflow test helper now that we're using a better mechanism to dirty the frame tree. https://reviewboard.mozilla.org/r/157028/#review162234

Comment on attachment 8885307 [details] Bug 1356271 - Make it so that it's easier to define repeating reflows for reflow tests. https://reviewboard.mozilla.org/r/156164/#review162238 ::: browser/base/content/test/performance/head.js:67 (Diff revisions 1 - 2) > > // We're going to remove the reflows one by one as we see them so that > // we can check for expected, unseen reflows, so let's clone the array. > - expectedReflows = expectedReflows.slice(0); > + // While we're at it, for reflows that omit the "times" property, default > + // it to 1. > + expectedReflows = expectedReflows.slice(0).map(reflow => { That's fine I guess, but a .map function that modifies its parameter makes me a bit uncomfortable. I would prefer: expectedReflows.forEach(r => { r.times = r.times || 1; })

Comment on attachment 8884937 [details] Bug 1356271 - add a test to measure how many layout flushes it takes for a simple location bar search. https://reviewboard.mozilla.org/r/155778/#review162244 I have much more confidence in this new version :-). ::: browser/base/content/test/performance/browser_urlbar_search_reflows.js:163 (Diff revision 3) > + const DOMAIN = "http://example.com"; > + let visits = []; > + > + for (let i = 0; i < NUM_VISITS; ++i) { > + visits.push({ > + uri: `${DOMAIN}/urlbar-reflows-${i}`, Is there a reason for making DOMAIN a const rather than inlining it here? ::: browser/base/content/test/performance/browser_urlbar_search_reflows.js:209 (Diff revision 3) > + oldResultsAdded(); > + }; > + > + await promiseAutocompleteResultPopup(win); > + }, EXPECTED_REFLOWS, win); > + await BrowserTestUtils.closeWindow(win); I don't feel strongly so feel free to leave this as is, but if I was the one writing this, I would inline this function so that the test code can be read sequentially from top to bottom. Do you expect to be able to re-use this function to expand the test later? I don't think its current implementation will be helpful for a test trying to synthesize key events.

Comment on attachment 8884937 [details] Bug 1356271 - add a test to measure how many layout flushes it takes for a simple location bar search. https://reviewboard.mozilla.org/r/155778/#review162294 Thinking about this some more, I think this test does a good job of preventing regressions, but doesn't really help us to prioritize our efforts toward fixing what matters the most for users. This is using a new window, and so catches all the reflows we do when first opening the awesomebar. I don't think fixing all of this by 57 is achievable. And if one sync reflow happens when opening the panel, that's probably OKish, as long as we don't cause several in a row by dirtying the frame tree and causing another reflow... and this is something the test doesn't test because we purposefully dirty the frame tree all the time here. Should this test (or a variation of it) not dirty the frame tree right after a reflow happened, so that we have a chance of counting how many sync reflows would actually be experienced by the user? The sync reflows that I think we really need to eliminate for 57 are those that happen while the user is typing, especially while the panel is already open. And even more specifically, the reflows that happen once per item we display. Could we add soon another test that starts with the awesomebar panel already opened, and counts only the reflows happening when the user causes the display to refresh by typing more characters? (in case it isn't clear; none of this should block landing, I just figured I would dump my thoughts somewhere) ::: browser/base/content/test/performance/browser_urlbar_search_reflows.js:157 (Diff revision 3) > +} > + > +const SEARCH_TERM = "urlbar-reflows"; > + > +add_task(async function setup() { > + const NUM_VISITS = 6; How did you pick this constant? (may be worth explaining in a comment above it) Should it match the max number of results that will be displayed in the awesomebar (10 currently)? If so, should we get this value from somewhere else instead of copying the constant in the test?

(In reply to Florian Quèze [:florian] [:flo] from comment #15) > ::: > browser/base/content/test/performance/browser_urlbar_search_reflows.js:209 > (Diff revision 3) > > + oldResultsAdded(); > > + }; > > + > > + await promiseAutocompleteResultPopup(win); > > + }, EXPECTED_REFLOWS, win); > > + await BrowserTestUtils.closeWindow(win); > > I don't feel strongly so feel free to leave this as is, but if I was the one > writing this, I would inline this function so that the test code can be read > sequentially from top to bottom. > > Do you expect to be able to re-use this function to expand the test later? I > don't think its current implementation will be helpful for a test trying to > synthesize key events. This comment was meant to be attached to the promiseAutocompleteResultPopup call, not the BrowserTestUtils.closeWindow one.

While reviewing bug 1356532, I updated this test to verify that these patches had the intended effect. I think it would make sense to open the awesomebar a second time during this test, and whitelist separately the reflows that happen only the first time and the reflows that happen everytime. This made me observe that the one-off buttons are rebuilt the 2 first times rather than only the first time. I filed bug 1383749 on this. The list of reflow stacks in the test after applying this patch reflect the situation after fixing bug 1356532 and bug 1383749. Feel free to either r+ this patch or adopt it.

Comment on attachment 8889467 [details] [diff] [review] check reflows the second time the panel opens Thanks, looks good! I'll file a bug for that one reflow in EXPECTED_REFLOWS and update the comment, and then just fold this in.

Comment on attachment 8889467 [details] [diff] [review] check reflows the second time the panel opens Review of attachment 8889467 [details] [diff] [review]: ----------------------------------------------------------------- ::: browser/base/content/test/performance/browser_urlbar_search_reflows.js @@ -48,5 @@ > ], > times: 2, // This number should only ever go down - never up. > }, > - > - { Pretty surprising these went away somehow... uh, let me just run this locally to check.

Comment on attachment 8889467 [details] [diff] [review] check reflows the second time the panel opens I have folded this patch in and updated it to reflect current reality in terms of reflow stacks. This meant getting rid of the shared EXPECTED_REFLOWS array, and having two distinct arrays.

Comment on attachment 8884937 [details] Bug 1356271 - add a test to measure how many layout flushes it takes for a simple location bar search. https://reviewboard.mozilla.org/r/155778/#review162244 > Is there a reason for making DOMAIN a const rather than inlining it here? I think I was originally using it elsewhere, but I dropped that part. I'll inline it. > I don't feel strongly so feel free to leave this as is, but if I was the one writing this, I would inline this function so that the test code can be read sequentially from top to bottom. > > Do you expect to be able to re-use this function to expand the test later? I don't think its current implementation will be helpful for a test trying to synthesize key events. Yeah, I think I'm probably going to end up re-using it for the typing case, but it will require slight modification. I'm going to leave this as is for now.

Comment on attachment 8884937 [details] Bug 1356271 - add a test to measure how many layout flushes it takes for a simple location bar search. https://reviewboard.mozilla.org/r/155778/#review162294 > How did you pick this constant? (may be worth explaining in a comment above it) > > Should it match the max number of results that will be displayed in the awesomebar (10 currently)? If so, should we get this value from somewhere else instead of copying the constant in the test? Okay, will switch this to 10 and update reflow counts accordingly.

(In reply to Mike Conley (:mconley) - Buried in needinfo / review backlog, eating my way out from comment #20) > Comment on attachment 8889467 [details] [diff] [review] > check reflows the second time the panel opens > > Review of attachment 8889467 [details] [diff] [review]: > ----------------------------------------------------------------- > > ::: browser/base/content/test/performance/browser_urlbar_search_reflows.js > @@ -48,5 @@ > > ], > > times: 2, // This number should only ever go down - never up. > > }, > > - > > - { > > Pretty surprising these went away somehow... uh, let me just run this > locally to check. As I said in comment 18, that's when testing with the patches from bug 1356532 applied. (In reply to Mike Conley (:mconley) - Buried in needinfo / review backlog, eating my way out from comment #21) > I have folded this patch in and updated it to reflect current reality in > terms of reflow stacks. This meant getting rid of the shared > EXPECTED_REFLOWS array, and having two distinct arrays. Why does it get rid of the shared array?

(In reply to Florian Quèze [:florian] [:flo] (away until August 7th) from comment #24) > > As I said in comment 18, that's when testing with the patches from bug > 1356532 applied. Ah, missed that part. Thanks. > > Why does it get rid of the shared array? Mainly because the stacks and numbers are different enough (at least at this time, without bug 1356532 fixed) that that simplification seems overcomplicated, imo. I currently have some matching stacks between the two arrays that have different "times" counts, and I didn't want to write more than necessary to reduce it all.

Pushed by mconley@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/f8860e47d9c3 Clean up comment about reflow test helper now that we're using a better mechanism to dirty the frame tree. r=florian https://hg.mozilla.org/integration/autoland/rev/cebc6bb19d83 Make it so that it's easier to define repeating reflows for reflow tests. r=florian https://hg.mozilla.org/integration/autoland/rev/cfb6f5820871 add a test to measure how many layout flushes it takes for a simple location bar search. r=florian

Backed out for test failures like https://treeherder.mozilla.org/logviewer.html#?job_id=118106291&repo=autoland https://hg.mozilla.org/integration/autoland/rev/4df14467e14cd45297ff6fb096f9f4340e79144a

Comment on attachment 8884937 [details] Bug 1356271 - add a test to measure how many layout flushes it takes for a simple location bar search. https://reviewboard.mozilla.org/r/155778/#review166960 ::: browser/base/content/test/performance/browser_urlbar_search_reflows.js:118 (Diff revision 4) > + }, > +]; > + > +/* These reflows happen everytime the awesomebar panel opens. */ > +const EXPECTED_REFLOWS_SECOND_OPEN = [ > + // Bug 1357054 That was actually bug 1383749, which is already fixed. I guess that's the other part of comment 18 that you missed :-/.

Pushed by mconley@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/33172a0a496a Clean up comment about reflow test helper now that we're using a better mechanism to dirty the frame tree. r=florian https://hg.mozilla.org/integration/autoland/rev/8ec898cb33d5 Make it so that it's easier to define repeating reflows for reflow tests. r=florian https://hg.mozilla.org/integration/autoland/rev/54b35cdc1f84 add a test to measure how many layout flushes it takes for a simple location bar search. r=florian

https://hg.mozilla.org/mozilla-central/rev/33172a0a496a https://hg.mozilla.org/mozilla-central/rev/8ec898cb33d5 https://hg.mozilla.org/mozilla-central/rev/54b35cdc1f84

Backout by cbook@mozilla.com: https://hg.mozilla.org/mozilla-central/rev/127982eac3f4 Backed out changeset 54b35cdc1f84 https://hg.mozilla.org/mozilla-central/rev/0ec51e44ccd1 Backed out changeset 8ec898cb33d5 https://hg.mozilla.org/mozilla-central/rev/5845151f1a2c Backed out changeset 33172a0a496a for broken tests after the backout of bug 1351148 on try

had to back this out in https://treeherder.mozilla.org/#/jobs?repo=mozilla-central&revision=5845151f1a2cd00957fdd48e204542ccbdfaba1e because of the backout of bug 1351148 and try tests from stone showed perma failures like https://treeherder.mozilla.org/logviewer.html#?job_id=118840729&repo=try when bug 1351148 was backedout on try

I'm guessing that, especially on slower debug builds, the sheer number of reflows being processed causes this test to time out, and bug 1351148 just changed the timing a bit. I'll update the test to request a longer period to run, and re-land.

Pushed by mconley@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/4047a260173d Clean up comment about reflow test helper now that we're using a better mechanism to dirty the frame tree. r=florian https://hg.mozilla.org/integration/autoland/rev/742643bf1bcb Make it so that it's easier to define repeating reflows for reflow tests. r=florian https://hg.mozilla.org/integration/autoland/rev/cf8d9788723a add a test to measure how many layout flushes it takes for a simple location bar search. r=florian

Backout by archaeopteryx@coole-files.de: https://hg.mozilla.org/integration/autoland/rev/d389494af12c Backed out changeset cf8d9788723a https://hg.mozilla.org/integration/autoland/rev/843bb2fdec5d Backed out changeset 742643bf1bcb https://hg.mozilla.org/integration/autoland/rev/58b2f8b17e37 Backed out changeset 4047a260173d for failing browser_urlbar_search_reflows.js on Linux. r=backout

Backed out for failing browser_urlbar_search_reflows.js on Linux: https://hg.mozilla.org/integration/autoland/rev/58b2f8b17e37c9760145d4cf8f9837e6ae79876f https://hg.mozilla.org/integration/autoland/rev/843bb2fdec5d0181a03b60e42d8cfdd56a561fee https://hg.mozilla.org/integration/autoland/rev/d389494af12c7f107efd218b0a96bec1ef6ae518 A push with failures: https://treeherder.mozilla.org/#/jobs?repo=autoland&revision=b9abf4de15297fa10e1ee09c99713a2c6286fd69&filter-resultStatus=testfailed&filter-resultStatus=busted&filter-resultStatus=exception&filter-resultStatus=retry&filter-resultStatus=usercancel&filter-resultStatus=runnable Failure log: https://treeherder.mozilla.org/logviewer.html#?job_id=119210956&repo=autoland [task 2017-07-29T08:42:23.872871Z] 08:42:23 INFO - TEST-PASS | browser/base/content/test/performance/browser_urlbar_search_reflows.js | expected uninterruptible reflow: '[ [task 2017-07-29T08:42:23.882081Z] 08:42:23 INFO - "adjustHeight@chrome://global/content/bindings/autocomplete.xml:1289:31", [task 2017-07-29T08:42:23.886937Z] 08:42:23 INFO - "_invalidate/this._adjustHeightTimeout<@chrome://global/content/bindings/autocomplete.xml:1186:58", [task 2017-07-29T08:42:23.889758Z] 08:42:23 INFO - "setTimeout handler*_invalidate@chrome://global/content/bindings/autocomplete.xml:1186:41", [task 2017-07-29T08:42:23.894308Z] 08:42:23 INFO - "invalidate@chrome://global/content/bindings/autocomplete.xml:1164:11", [task 2017-07-29T08:42:23.897918Z] 08:42:23 INFO - "testFn/popup.invalidate@chrome://mochitests/content/browser/browser/base/content/test/performance/browser_urlbar_search_reflows.js:252:7", [task 2017-07-29T08:42:23.900459Z] 08:42:23 INFO - "testFn/popup.invalidate@chrome://mochitests/content/browser/browser/base/content/test/performance/browser_urlbar_search_reflows.js:252:7", [task 2017-07-29T08:42:23.905805Z] 08:42:23 INFO - "notifyResults@jar:file:///home/worker/workspace/build/application/firefox/omni.ja!/components/UnifiedComplete.js:2214:5", [task 2017-07-29T08:42:23.911044Z] 08:42:23 INFO - "finishSearch@jar:file:///home/worker/workspace/build/application/firefox/omni.ja!/components/UnifiedComplete.js:2387:5", [task 2017-07-29T08:42:23.914704Z] 08:42:23 INFO - "startSearch/<@jar:file:///home/worker/workspace/build/application/firefox/omni.ja!/components/UnifiedComplete.js:2344:33", [task 2017-07-29T08:42:23.918540Z] 08:42:23 INFO - "promise callback*startSearch@jar:file:///home/worker/workspace/build/application/firefox/omni.ja!/components/UnifiedComplete.js:2337:5", [task 2017-07-29T08:42:23.924218Z] 08:42:23 INFO - "promiseAutocompleteResultPopup@chrome://mochitests/content/browser/browser/base/content/test/performance/browser_urlbar_search_reflows.js:183:3", [task 2017-07-29T08:42:23.928962Z] 08:42:23 INFO - "async*testFn@chrome://mochitests/content/browser/browser/base/content/test/performance/browser_urlbar_search_reflows.js:260:11", [task 2017-07-29T08:42:23.932770Z] 08:42:23 INFO - "async*withReflowObserver@chrome://mochitests/content/browser/browser/base/content/test/performance/head.js:128:11", [task 2017-07-29T08:42:23.936535Z] 08:42:23 INFO - "async*@chrome://mochitests/content/browser/browser/base/content/test/performance/browser_urlbar_search_reflows.js:265:9", [task 2017-07-29T08:42:23.940733Z] 08:42:23 INFO - "Async*Tester_execTest/<@chrome://mochikit/content/browser-test.js:798:21", [task 2017-07-29T08:42:23.943111Z] 08:42:23 INFO - "TaskImpl_run@resource://gre/modules/Task.jsm:331:42", [task 2017-07-29T08:42:23.945363Z] 08:42:23 INFO - "promise callback*TaskImpl_handleResultValue@resource://gre/modules/Task.jsm:408:7", [task 2017-07-29T08:42:23.947606Z] 08:42:23 INFO - "TaskImpl_run@resource://gre/modules/Task.jsm:339:15", [task 2017-07-29T08:42:23.951429Z] 08:42:23 INFO - "TaskImpl@resource://gre/modules/Task.jsm:280:3", [task 2017-07-29T08:42:23.954225Z] 08:42:23 INFO - "asyncFunction@resource://gre/modules/Task.jsm:252:14", [task 2017-07-29T08:42:23.956744Z] 08:42:23 INFO - "Task_spawn@resource://gre/modules/Task.jsm:166:12", [task 2017-07-29T08:42:23.963291Z] 08:42:23 INFO - "Tester_execTest@chrome://mochikit/content/browser-test.js:789:9", [task 2017-07-29T08:42:23.965208Z] 08:42:23 INFO - "Tester.prototype.nextTest</<@chrome://mochikit/content/browser-test.js:689:7", [task 2017-07-29T08:42:23.967344Z] 08:42:23 INFO - "SimpleTest.waitForFocus/waitForFocusInner/focusedOrLoaded/<@chrome://mochikit/content/tests/SimpleTest/SimpleTest.js:795:59", [task 2017-07-29T08:42:23.969289Z] 08:42:23 INFO - "" [task 2017-07-29T08:42:23.971270Z] 08:42:23 INFO - ]' - true == true - [task 2017-07-29T08:42:23.973443Z] 08:42:23 INFO - Buffered messages finished [task 2017-07-29T08:42:23.975390Z] 08:42:23 INFO - TEST-UNEXPECTED-FAIL | browser/base/content/test/performance/browser_urlbar_search_reflows.js | Test timed out - [task 2017-07-29T08:42:23.977495Z] 08:42:23 INFO - Leaving test bound

Pushed by mconley@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/8536b787a5c7 Clean up comment about reflow test helper now that we're using a better mechanism to dirty the frame tree. r=florian https://hg.mozilla.org/integration/autoland/rev/79ac0025e9e8 Make it so that it's easier to define repeating reflows for reflow tests. r=florian https://hg.mozilla.org/integration/autoland/rev/1c901b2b5971 add a test to measure how many layout flushes it takes for a simple location bar search. r=florian

https://hg.mozilla.org/mozilla-central/rev/8536b787a5c7 https://hg.mozilla.org/mozilla-central/rev/79ac0025e9e8 https://hg.mozilla.org/mozilla-central/rev/1c901b2b5971