Marco Bonardo [:mak] Reporter
	Description • 7 years ago

We should try using getBoundsWithoutFlushing() and see if it break things.

	Marco Bonardo [:mak] Reporter
	Comment 2 • 7 years ago

(In reply to Marco Bonardo [::mak] from comment #0)
> We should try using getBoundsWithoutFlushing() and see if it break things.

The answer is yes, it breaks things.
Reducing the window size to make it easier to have overflow, and search in the urlbar.
The right side of the text should get ellipsis and not touch the border of the panel, it doesn't without a flush.

	Brian Carpenter [:geeknik]
	Comment 3 • 7 years ago

Is it helpful to post these stacks? If so, here is one that just popped up during testing:

_handleOverflow@chrome://global/content/bindings/autocomplete.xml:2391:26
handleOverUnderflow@chrome://global/content/bindings/autocomplete.xml:2463:11
_reuseAcItem@chrome://global/content/bindings/autocomplete.xml:2099:15
_appendCurrentResult@chrome://global/content/bindings/autocomplete.xml:1365:28
_invalidate@chrome://global/content/bindings/autocomplete.xml:1193:11
invalidate@chrome://global/content/bindings/autocomplete.xml:1162:11
notifyResults@resource://gre/components/UnifiedComplete.js:2114:5
finishSearch@resource://gre/components/UnifiedComplete.js:2287:5
startSearch/<@resource://gre/components/UnifiedComplete.js:2244:33
process@resource://gre/modules/Promise.jsm -> resource://gre/modules/Promise-backend.js:922:23
walkerLoop@resource://gre/modules/Promise.jsm -> resource://gre/modules/Promise-backend.js:806:7
Promise*scheduleWalkerLoop@resource://gre/modules/Promise.jsm -> resource://gre/modules/Promise-backend.js:739:11
schedulePromise@resource://gre/modules/Promise.jsm -> resource://gre/modules/Promise-backend.js:770:7
completePromise@resource://gre/modules/Promise.jsm -> resource://gre/modules/Promise-backend.js:707:7
TaskImpl_run@resource://gre/modules/Task.jsm:324:15
promise callback*TaskImpl_handleResultValue@resource://gre/modules/Task.jsm:396:7
TaskImpl_run@resource://gre/modules/Task.jsm:327:15
process@resource://gre/modules/Promise.jsm -> resource://gre/modules/Promise-backend.js:922:23
walkerLoop@resource://gre/modules/Promise.jsm -> resource://gre/modules/Promise-backend.js:806:7
Promise*scheduleWalkerLoop@resource://gre/modules/Promise.jsm -> resource://gre/modules/Promise-backend.js:739:11
schedulePromise@resource://gre/modules/Promise.jsm -> resource://gre/modules/Promise-backend.js:770:7
completePromise@resource://gre/modules/Promise.jsm -> resource://gre/modules/Promise-backend.js:707:7
TaskImpl_run@resource://gre/modules/Task.jsm:324:15
promise callback*TaskImpl_handleResultValue@resource://gre/modules/Task.jsm:396:7
TaskImpl_run@resource://gre/modules/Task.jsm:327:15
promise callback*TaskImpl_handleResultValue@resource://gre/modules/Task.jsm:396:7
TaskImpl_run@resource://gre/modules/Task.jsm:327:15
promise callback*TaskImpl_handleResultValue@resource://gre/modules/Task.jsm:396:7
TaskImpl_run@resource://gre/modules/Task.jsm:327:15
promise callback*TaskImpl_handleResultValue@resource://gre/modules/Task.jsm:396:7
TaskImpl_run@resource://gre/modules/Task.jsm:327:15
process@resource://gre/modules/Promise.jsm -> resource://gre/modules/Promise-backend.js:922:23
walkerLoop@resource://gre/modules/Promise.jsm -> resource://gre/modules/Promise-backend.js:806:7
Promise*scheduleWalkerLoop@resource://gre/modules/Promise.jsm -> resource://gre/modules/Promise-backend.js:739:11
schedulePromise@resource://gre/modules/Promise.jsm -> resource://gre/modules/Promise-backend.js:770:7
Promise.prototype.then@resource://gre/modules/Promise.jsm -> resource://gre/modules/Promise-backend.js:455:5
TaskImpl_handleResultValue@resource://gre/modules/Task.jsm:396:7
TaskImpl_run@resource://gre/modules/Task.jsm:327:15
promise callback*TaskImpl_handleResultValue@resource://gre/modules/Task.jsm:396:7
TaskImpl_run@resource://gre/modules/Task.jsm:327:15
process@resource://gre/modules/Promise.jsm -> resource://gre/modules/Promise-backend.js:922:23
walkerLoop@resource://gre/modules/Promise.jsm -> resource://gre/modules/Promise-backend.js:806:7
Promise*scheduleWalkerLoop@resource://gre/modules/Promise.jsm -> resource://gre/modules/Promise-backend.js:739:11
schedulePromise@resource://gre/modules/Promise.jsm -> resource://gre/modules/Promise-backend.js:770:7
Promise.prototype.then@resource://gre/modules/Promise.jsm -> resource://gre/modules/Promise-backend.js:455:5
startSearch@resource://gre/components/UnifiedComplete.js:2237:5
onInput@chrome://browser/content/urlbarBindings.xml:1125:17
onxblinput@chrome://global/content/bindings/autocomplete.xml:649:9

	Florian Quèze [:florian]
	Comment 4 • 7 years ago

Unsurprisingly, this is very visible in profiles of interacting with the awesomebar: https://perfht.ml/2t6DoAs

	Kris Maglione [:kmag]
	Comment 5 • 7 years ago

All of autocomplete.xml is a reflow nightmare... particularly when adjustHeight is called, and we go back and force between changing the DOM and querying layout multiple times, for every item...

	Marco Bonardo [:mak] Reporter
	Comment 6 • 7 years ago

The thing we should try to preserve is animations and delayed shrinking, we really don't want to go back to the previous status where the panel was resizing up and down like crazy at every keypress. We should also avoid locking up into a world where we assume all the richlistitems are the same height, cause even if it's true now, it won't be for long.
It's true that some parts are a reflow nightmare, but it gives quite more perception of speed than the previous state.
We should try to use RAF and simplify the logic a bit and check where we are then.

	Florian Quèze [:florian]
	Comment 7 • 7 years ago

(In reply to Marco Bonardo [::mak] from comment #6)

> It's true that some parts are a reflow nightmare, but it gives quite more
> perception of speed than the previous state.

With flickering and long pauses (I saw 500+ms pauses while typing in the urlbar on my fast macbook, due to _handleOverflow reflows), it's difficult to agree on the second half of your sentence.

> We should try to use RAF and simplify the logic a bit and check where we are
> then.

Eventually we should get rid of all these sync reflows in primary UI. If this isn't achievable in the 57 time frame, we should at the very least ensure that things in the various items are done at the same time, to avoid the current situation where we reflow several times per item and repeat for each item.

The test added in bug 1356271 should be very helpful to track our progress.

	Kris Maglione [:kmag]
	Comment 12 • 7 years ago

These patches seem to give smooth UI behavior without any uninterruptible overflows.

	Kris Maglione [:kmag]
	Comment 13 • 7 years ago

*reflows (during overflow :) )

	Florian Quèze [:florian]
	Comment 14 • 7 years ago
mozreview-review

Comment on attachment 8889104 [details]
Bug 1356532: Part 2 - Avoid layout flushes during adjustSiteIconStart().

https://reviewboard.mozilla.org/r/160130/#review165486

::: toolkit/content/widgets/autocomplete.xml:2391
(Diff revision 1)
>            let rect = utils.getBoundsWithoutFlushing(this._siteIcon);
>  
>            let dir = this.getAttribute("dir");
>            let delta = dir == "rtl" ? rect.right - newStart
>                                     : newStart - rect.left;
> +          (async () => {

Isn't there a () missing at the end of this async function to call it?

	Kris Maglione [:kmag]
	Comment 15 • 7 years ago
mozreview-review-reply

Comment on attachment 8889104 [details]
Bug 1356532: Part 2 - Avoid layout flushes during adjustSiteIconStart().

https://reviewboard.mozilla.org/r/160130/#review165486

> Isn't there a () missing at the end of this async function to call it?

Hm. Yes, I fixed that locally after I tested, but somehow it didn't make it into the commit.

	Marco Bonardo [:mak] Reporter
	Comment 20 • 7 years ago
mozreview-review

Comment on attachment 8889103 [details]
Bug 1356532: Part 1 - Add BrowserUtils.promiseAnimationFrame helper.

https://reviewboard.mozilla.org/r/160128/#review165636

::: toolkit/modules/PromiseUtils.jsm:144
(Diff revision 2)
> +   *
> +   * @param {Window} win
> +   * @param {function} callback
> +   * @returns {Promise}
> +   */
> +  animationFrame(win, callback) {

Doesn't look like this has anything to do with Promises, and and such this looks like the wrong place to put this helper.
I'd suggest BrowserUtils instead.

	Florian Quèze [:florian]
	Comment 21 • 7 years ago
mozreview-review

Comment on attachment 8889103 [details]
Bug 1356532: Part 1 - Add BrowserUtils.promiseAnimationFrame helper.

https://reviewboard.mozilla.org/r/160128/#review165676

Removing the review flag on this part for now. I'm not convinced we really need this helper, and if we do, it's probably not in the right file.

::: commit-message-ff979:5
(Diff revision 2)
> +Bug 1356532: Part 1 - Add PromiseUtils.animationFrame helper. r?florian
> +
> +It's important that requestAnimationFrame callbacks run synchronously, and
> +carefully control what code runs during them. At the same time, we often need
> +to UI code that runs before and after the DOM mutations in an animation frame

Looks like there's a missing word after 'to'.

::: commit-message-ff979:17
(Diff revision 2)
> +
> +
> +It's also important to note that, at the moment, the promise microtask queue
> +is not flushed at the end of the animation frame callback, which means we can
> +safely rely on promise resolution handlers automatically executing after frame
> +has been painted. After bug 1193394 is fixed, that will no longer be the case,

Should we leave a note about this either in bug 1193394 or in a code comment here?

	Florian Quèze [:florian]
	Comment 22 • 7 years ago
mozreview-review

Comment on attachment 8889104 [details]
Bug 1356532: Part 2 - Avoid layout flushes during adjustSiteIconStart().

https://reviewboard.mozilla.org/r/160130/#review165756

Looks good to me, thanks!

While testing the whole patchset, I noticed that whenever we open the awesomebar in a window for the first time, the icon positions are off for a split second. I see it very consistently now, probably because I'm paying more attention due to reviewing these patches. After unapplying the patches I can still reproduce, so it's not a new issue. I wonder if these patches could have made the time while the icon positions are off longer, eg. 2 frames instead of 1... or if it's just a side effect of me paying closer attention.

It looks like there's related code that still does sync reflows at http://searchfox.org/mozilla-central/rev/3a3af33f513071ea829debdfbc628caebcdf6996/browser/base/content/urlbarBindings.xml#1852 so it may be a good idea to fix these too before further investigating.

This should probably be a follow-up anyway, and you don't have to take it unless you want to :-).

	Florian Quèze [:florian]
	Comment 23 • 7 years ago
mozreview-review

Comment on attachment 8889105 [details]
Bug 1356532: Part 4 - Avoid layout flushes during adjustSiteIconStart() and _handleOverflow().

https://reviewboard.mozilla.org/r/160132/#review165672

::: toolkit/content/widgets/autocomplete.xml:2365
(Diff revision 2)
>        <!-- Sets the x-coordinate of the leading edge of the site icon (favicon)
>             relative the the leading edge of the window.
>             @param newStart The new x-coordinate, relative to the leading edge of
>                    the window.  Pass undefined to reset the icon's position to
>                    whatever is specified in CSS.
>             @return True if the icon's position changed, false if not. -->

Please remove this last line of the comment.

::: toolkit/content/widgets/autocomplete.xml:2381
(Diff revision 2)
>            }
>  
> -          let utils = window.QueryInterface(Ci.nsIInterfaceRequestor)
> -                            .getInterface(Ci.nsIDOMWindowUtils);
> -          let rect = utils.getBoundsWithoutFlushing(this._siteIcon);
> +          (async () => {
> +            let delta;
> +            let px = this._typeIcon.style.marginInlineStart;
> +            await PromiseUtils.layoutFlushed(document, "layout", () => {

Why is it better to wait for a layout flush in all cases instead of relying on getBoundsWithoutFlushing? Isn't there a risk of this causing one frame to appear with the icons in the wrong place?

::: toolkit/content/widgets/autocomplete.xml:2415
(Diff revision 2)
>          <body><![CDATA[
> -          let itemRect = this.parentNode.getBoundingClientRect();
> -          let titleRect = this._titleText.getBoundingClientRect();
> -          let tagsRect = this._tagsText.getBoundingClientRect();
> -          let separatorRect = this._separator.getBoundingClientRect();
> -          let urlRect = this._urlText.getBoundingClientRect();
> +          if (this._overflowPromise) {
> +            return;
> +          }
> +
> +          this._overflowPromise = (async () => {

It looks like this._overflowPromise is used as a boolean and nothing is waiting for the result of this promise (or handling rejections).

Should this just be:

this._handleOverflowPending = true;
(async () => { ... })();

?

::: toolkit/content/widgets/autocomplete.xml:2460
(Diff revision 2)
> -            let titleTagsMaxWidth = Math.max(
> +              let titleTagsMaxWidth = Math.max(
> -              titleTagsAvailable,
> +                titleTagsAvailable,
> -              itemWidth * titleTagsPct
> +                itemWidth * titleTagsPct
> -            );
> +              );
> +
> +              await PromiseUtils.animationFrame(window, () => {

Unless something is actually using the promise, I don't see the need for PromiseUtils.animationFrame here. Would we lose something if we simplified to:
requestAnimationFrame(() => {
?

	Florian Quèze [:florian]
	Comment 24 • 7 years ago
mozreview-review

Comment on attachment 8889106 [details]
Bug 1356532: Part 5 - Avoid layout flushes during adjustHeight().

https://reviewboard.mozilla.org/r/160134/#review165766

Looks good to me but I'm not very familiar with this awesomebar panel height code. I think ::mak would be a better reviewer for this part, but he currently seems a bit overloaded, so I'm going to r+ and needinfo him so that he can check if he would like to review this too.

	Florian Quèze [:florian]
	Comment 25 • 7 years ago

(In reply to Florian Quèze [:florian] [:flo] from comment #24)
> Comment on attachment 8889106 [details]
> Bug 1356532: Part 4 - Avoid layout flushes during adjustHeight().
> 
> https://reviewboard.mozilla.org/r/160134/#review165766
> 
> Looks good to me but I'm not very familiar with this awesomebar panel height
> code. I think ::mak would be a better reviewer for this part, but he
> currently seems a bit overloaded, so I'm going to r+ and needinfo him so
> that he can check if he would like to review this too.

	Kris Maglione [:kmag]
	Comment 26 • 7 years ago
mozreview-review-reply

Comment on attachment 8889103 [details]
Bug 1356532: Part 1 - Add BrowserUtils.promiseAnimationFrame helper.

https://reviewboard.mozilla.org/r/160128/#review165636

> Doesn't look like this has anything to do with Promises, and and such this looks like the wrong place to put this helper.
> I'd suggest BrowserUtils instead.

I've been thinking about adding other helpers to PromiseUtils, for things like observers and DOM events, that this seemed to fit well with.

I'm not really opposed to using BrowserUtils, but PromiseUtils seems like a better place to group those kinds of helpers.

	Marco Bonardo [:mak] Reporter
	Comment 27 • 7 years ago

I disagree, if everyone starts adding anything to PromiseUtils just because it returns a promise, it'll soon be unmanageable.
PromiseUtils exists to keep utils that are strictly related to Promises (and also PromiseUtils.defer is sort-of moving towards deprecation so the whole module may go away).

	Florian Quèze [:florian]
	Comment 28 • 7 years ago

(In reply to Kris Maglione [:kmag] from comment #26)

> I'm not really opposed to using BrowserUtils, but PromiseUtils seems like a
> better place to group those kinds of helpers.

The way I see it, PromiseUtils is more for things related to promises that work in any JS context, and BrowserUtils is more for stuff that is meant to be used in a (browser) window context.

	Marco Bonardo [:mak] Reporter
	Comment 29 • 7 years ago

Also, looks like some other changeset in your queue is adding a layoutFlushed to PromiseUtils, that should also be moved elsewhere.

	Florian Quèze [:florian]
	Comment 30 • 7 years ago

(In reply to Marco Bonardo [::mak] from comment #29)
> Also, looks like some other changeset in your queue is adding a
> layoutFlushed to PromiseUtils, that should also be moved elsewhere.

I already mentioned that in bug 1383367 comment 6.

	Dão Gottwald [:dao]
	Comment 31 • 7 years ago

(In reply to Florian Quèze [:florian] [:flo] (away until August 7th) from comment #22)
> While testing the whole patchset, I noticed that whenever we open the
> awesomebar in a window for the first time, the icon positions are off for a
> split second. I see it very consistently now, probably because I'm paying
> more attention due to reviewing these patches. After unapplying the patches
> I can still reproduce, so it's not a new issue. I wonder if these patches
> could have made the time while the icon positions are off longer, eg. 2
> frames instead of 1...

Considering bug 1385729, that seems likely.

	Mike Conley (:mconley) (:⚙️)
	Comment 32 • 7 years ago

Hey kmag, anything left to do here, or are these ready to land?

	:Paolo Amadini
	Comment 33 • 7 years ago

I didn't realize there was already a patch for this, but I think the alternative solution for Part 2 in bug 1358792 should be less prone to flickering. The later patches in this bug are still needed to fix everything.

	Kris Maglione [:kmag]
	Comment 39 • 7 years ago

Comment on attachment 8889105 [details]
Bug 1356532: Part 4 - Avoid layout flushes during adjustSiteIconStart() and _handleOverflow().

This could use another review. I rolled the handling of adjustSiteIconStart into _handleOverflow to avoid flakiness or the need to force additional reflows.

	Marco Bonardo [:mak] Reporter
	Comment 40 • 7 years ago

didn't paolo already take care of adjustSiteIconStart in bug 1358792? what is this doing?

	Kris Maglione [:kmag]
	Comment 41 • 7 years ago

(In reply to Marco Bonardo [::mak] from comment #40)
> didn't paolo already take care of adjustSiteIconStart in bug 1358792? what
> is this doing?

Sort of. That solution only worked because of the obscene number of forced reflows in the existing code. When I fixed those, the layout wound up blowing up after the patches in bug 1358792, because getBoundsWithoutFlushing returned a null rect, and adjustSiteIconStart computed the new width as a delta from the existing width, and adjustSiteIconStart wound up being called multiple times between flushes.

	Marco Bonardo [:mak] Reporter
	Comment 42 • 7 years ago

Something interesting happened, the patch in bug 1388331 seems somehow to completely remove the most common _handleOverflow reflow :/
I'm still not sure what's up, but could be interesting to do a check if that's true or the test is somehow bogus.

	Marco Bonardo [:mak] Reporter
	Comment 43 • 7 years ago

Looks like it was just a reduction in the number. "Unused expected reflow" is a quite confusing failure message, unused looked like the reflow went away completely.

	Florian Quèze [:florian]
	Comment 44 • 7 years ago
mozreview-review

Comment on attachment 8889103 [details]
Bug 1356532: Part 1 - Add BrowserUtils.promiseAnimationFrame helper.

https://reviewboard.mozilla.org/r/160128/#review179510

::: commit-message-ff979:5
(Diff revision 3)
> +Bug 1356532: Part 1 - Add BrowserUtils.promiseAnimationFrame helper. r?florian
> +
> +It's important that requestAnimationFrame callbacks run synchronously, and
> +carefully control what code runs during them. At the same time, we often need
> +to UI code that runs before and after the DOM mutations in an animation frame

Still looks like there's a missing word after 'to'.

	Florian Quèze [:florian]
	Comment 45 • 7 years ago
mozreview-review

Comment on attachment 8901501 [details]
Bug 1356532: Part 2 - Add helper for performing Promise microtask checkpoint from JS.

https://reviewboard.mozilla.org/r/172954/#review179512

Looks good to me, but I'm not a reviewer for dom/

::: dom/base/nsDOMWindowUtils.cpp:1888
(Diff revision 1)
>  }
>  
>  NS_IMETHODIMP
> +nsDOMWindowUtils::PerformMicroTaskCheckpoint()
> +{
> +  Promise::PerformMicroTaskCheckpoint();

Promise::PerformMicroTaskCheckpoint() returns a boolean, should we make performMicroTaskCheckpoint return this value, even if we don't need it for the current use case?

	Florian Quèze [:florian]
	Comment 46 • 7 years ago
mozreview-review

Comment on attachment 8901502 [details]
Bug 1356532: Part 3 - Add layout query helper that forces a single reflow before paint.

https://reviewboard.mozilla.org/r/172956/#review179516

Thanks for the updated patch. It's unfortunate we have to force this reflow, but a huge improvement over the current situation :-)

::: toolkit/modules/BrowserUtils.jsm:39
(Diff revision 1)
> +  this._idleRequest = null;
> +  this._reflowRequested = false;
>  }
>  
> -ReflowObserver.prototype = {
> +ReflowScheduler.get = function(doc) {
> +  let observer = reflowSchedulers.get(doc);

Is the 'observer' variable name here a leftover from a previous version of the patch?

::: toolkit/modules/BrowserUtils.jsm:141
(Diff revision 1)
> +  addSlowReflowCallback(callback) {
> +    this.addReflowCallback(callback);
> +
> +    // Add both idle and animation frame callbacks. If the idle callback
> +    // fires first, we force a reflow immediately, and do nothing
> +    // special during the animation frame callback. If the callback is

"do nothing special during the animation frame callback" seems to actually be "cancel the animation frame callback" in the implementation.

::: toolkit/modules/BrowserUtils.jsm:837
(Diff revision 1)
> +   * pending promise handlers will likewise run before the paint. When
> +   * combined with `promiseAnimationFrame, this allows multiple
> +   * operations that depend on querying the layout state before paint to
> +   * be grouped so that only a single extra layout flush is required.
> +   *
> +   * Please *USE THIS ONLY AS A LAST RESORT*. If you need to query they

they -> the

::: toolkit/modules/BrowserUtils.jsm:878
(Diff revision 1)
> -   * The callback function may alter the DOM freely, but *must not query the document's style or
> -   * layout state*.
> +   * The callback function may alter the DOM freely, but *must not query
> +   * the document's style or layout state*.
> +   *
> +   * When called due to a forced reflow from `promiseSlowLayoutFlush`
> +   * that happens during an animation frame, this animation frame
> +   * callback is guaranteed to run during the same animation frame.

I had to read this comment 3 times to understand it, and I know what we are talking about... so I'm afraid this is a bit obscure.
Possible tweaks to the wording:
- simplify "during the same animation frame" to "during the same frame"
or
- replace "during the same animation frame" with "before the frame is painted".

	Florian Quèze [:florian]
	Comment 47 • 7 years ago
mozreview-review

Comment on attachment 8889105 [details]
Bug 1356532: Part 4 - Avoid layout flushes during adjustSiteIconStart() and _handleOverflow().

https://reviewboard.mozilla.org/r/160132/#review179550

r- because the positions are off. I'll attach a screenshot.

Please update the list of expected reflows in mconley's browser/base/content/test/performance/browser_urlbar_*_reflows.js tests.

	Kris Maglione [:kmag]
	Comment 49 • 7 years ago

(In reply to Florian Quèze [:florian] [:flo] (away until August 28th) from comment #47)
> r- because the positions are off. I'll attach a screenshot.

Hm. Interesting. I didn't have this issue when I first tested, but I do now.

	Kris Maglione [:kmag]
	Comment 50 • 7 years ago
mozreview-review-reply

Comment on attachment 8901502 [details]
Bug 1356532: Part 3 - Add layout query helper that forces a single reflow before paint.

https://reviewboard.mozilla.org/r/172956/#review179516

> I had to read this comment 3 times to understand it, and I know what we are talking about... so I'm afraid this is a bit obscure.
> Possible tweaks to the wording:
> - simplify "during the same animation frame" to "during the same frame"
> or
> - replace "during the same animation frame" with "before the frame is painted".

Heh... yeah, I struggled to find a sensible way to explain this and still came up short...

Updated to:

    * This has an important difference from `window.requestAnimationFrame`
    * when used alongside `promiseSlowLayoutFlush`:
    *
    * Sometimes `promiseSlowLayoutFlush` needs to force a layout flush
    * from an animation frame callback before calling its callbacks. If
    * those callbacks call `window.requestAnimationFrame` in that
    * circumstance, those callbacks will run in the *next* animation
    * frame, after the current frame is painted. `promiseAnimationFrame`
    * callbacks, however, will run in the *same* animation frame, after
    * all layout flush callbacks have been called and their pending
    * promise microtasks processed.

(with some additional copy-editing that I can't be bothered to copy back into the mozreview field and reformat...)

	Kris Maglione [:kmag]
	Comment 51 • 7 years ago
mozreview-review-reply

Comment on attachment 8889103 [details]
Bug 1356532: Part 1 - Add BrowserUtils.promiseAnimationFrame helper.

https://reviewboard.mozilla.org/r/160128/#review179510

> Still looks like there's a missing word after 'to'.

Updating commit messages is always the last thing I do before I land :)

	Florian Quèze [:florian]
	Comment 57 • 7 years ago

(In reply to Kris Maglione [:kmag] from comment #50)

> Heh... yeah, I struggled to find a sensible way to explain this and still
> came up short...
> 
> Updated to:
[...]

Much clearer, thanks! :-)

	Florian Quèze [:florian]
	Comment 58 • 7 years ago
mozreview-review

Comment on attachment 8889105 [details]
Bug 1356532: Part 4 - Avoid layout flushes during adjustSiteIconStart() and _handleOverflow().

https://reviewboard.mozilla.org/r/160132/#review179948

The browser/base/content/test/performance/browser_urlbar_keyed_search_reflows.js test also needs to be updated.

The positions are still off for a few frames before getting fixed, I'll attach a screen cast.

::: toolkit/content/widgets/autocomplete.xml:2390
(Diff revisions 3 - 4)
>            this._overflowPromise = (async () => {
>              if (typeof this._siteIconStart != "number") {
>                this.typeIconSpacer.style.removeProperty("width");
>              }
>  
> -            let [itemRect, iconSpacerRect, titleRect, tagsRect, separatorRect, urlRect, actionRect] =
> +            let [itemRect, iconSpacerRect, siteIconRect, titleRect, tagsRect, separatorRect, urlRect, actionRect] =

nit: This line is too long, please wrap it after "tagsRect,".

::: browser/base/content/test/performance/browser_urlbar_search_reflows.js:41
(Diff revision 4)
> -    stack: [
> -      "adjustHeight@chrome://global/content/bindings/autocomplete.xml",
> -      "_invalidate/this._adjustHeightTimeout<@chrome://global/content/bindings/autocomplete.xml",
>      ],
>      times: 3, // This number should only ever go down - never up.
> +    minTimes: 2,

What's causing this non determinism? Is it because we sometimes have 3 frames and sometimes only 2 before we are done displaying the panel? (this could do with a comment here in the test)

Is there anything we can do about it to have always the same number of reflows here?

I almost wonder if BrowserUtils.forceReflow should be handled by head.js, with a verification that it happens at most once per requestAnimationFrame, and only a boolean in the test file to say that the code being tested is expected to use BrowserUtils.forceReflow. Maybe this is overkill though.

::: browser/base/content/test/performance/browser_urlbar_search_reflows.js:95
(Diff revision 4)
> +  {
> +    stack: [
> +      "forceReflow@resource://gre/modules/BrowserUtils.jsm",
> +    ],
> +    times: 3, // This number should only ever go down - never up.
> +    minTimes: 1,

Same here.

::: browser/base/content/test/performance/head.js:54
(Diff revision 4)
>   *              "get_scrollPosition@chrome://global/content/bindings/scrollbox.xml",
>   *              "_fillTrailingGap@chrome://browser/content/tabbrowser.xml",
>   *              "_handleNewTab@chrome://browser/content/tabbrowser.xml",
>   *              "onxbltransitionend@chrome://browser/content/tabbrowser.xml",
>   *            ],
>   *          }

If we do want to have support for "minTimes", we should document it in this comment with an example.

::: browser/base/content/test/performance/head.js:66
(Diff revision 4)
>   *        will cause an assertion failure. When this argument is not passed,
>   *        it defaults to the empty Array, meaning no reflows are expected.
>   * @param window (browser window, optional)
>   *        The browser window to monitor. Defaults to the current window.
>   */
> +let times = {};

This doesn't seem to be used, please remove it. And if it was used, a global variable shared with all test files of the folder should have a longer name to avoid collisions.

::: browser/base/content/test/performance/head.js:126
(Diff revision 4)
>        let index = expectedReflows.findIndex(reflow => path.startsWith(reflow.stack.join("|")));
>  
>        if (index != -1) {
>          Assert.ok(true, "expected uninterruptible reflow: '" +
>                    JSON.stringify(pathWithLineNumbers, null, "\t") + "'");
> +        --expectedReflows[index].minTimes;

While this works (and results in NaN), -- on something that's undefined makes me a bit uncomfortable.

::: browser/base/content/test/performance/head.js:161
(Diff revision 4)
>    } finally {
>      for (let remainder of expectedReflows) {
> -      Assert.ok(false,
> +      Assert.ok(remainder.minTimes <= 0,
>                  `Unused expected reflow: ${JSON.stringify(remainder.stack, null, "\t")}\n` +
> -                `This reflow was supposed to be hit ${remainder.times} more time(s).\n` +
> +                `This reflow was supposed to be hit ${remainder.minTimes || remainder.times} ` +
> +                `more time(s).\n` +

This line is not a template string and could use normal double quotes.

::: toolkit/modules/BrowserUtils.jsm:50
(Diff revision 4)
>  }
>  
>  ReflowScheduler.prototype = {
>    QueryInterface: XPCOMUtils.generateQI(["nsIReflowObserver", "nsISupportsWeakReference"]),
>  
> +  // This is overridden by mochitests to allow us skip dirtying the

to allow us _to_ skip

	Florian Quèze [:florian]
	Comment 60 • 7 years ago

When running the browser_urlbar_search_reflows.js test after applying the patches, I see lots of errors like this in the output:

19 INFO Console message: [JavaScript Warning: "Error in parsing value for ‘max-width’.  Declaration dropped." {file: "chrome://browser/content/browser.xul" line: 0 column: 0 source: "-6.534000000000001px"}]
20 INFO Console message: [JavaScript Warning: "Error in parsing value for ‘max-width’.  Declaration dropped." {file: "chrome://browser/content/browser.xul" line: 0 column: 0 source: "-13.265999999999998px"}]
21 INFO Console message: [JavaScript Warning: "Error in parsing value for ‘max-width’.  Declaration dropped." {file: "chrome://browser/content/browser.xul" line: 0 column: 0 source: "-10.2px"}]
22 INFO Console message: [JavaScript Warning: "Error in parsing value for ‘max-width’.  Declaration dropped." {file: "chrome://browser/content/browser.xul" line: 0 column: 0 source: "-10.2px"}]

These errors aren't here if I unapply the patches. This may give a hint about what's causing the flicker in my screencast.

	Kris Maglione [:kmag]
	Comment 61 • 7 years ago
mozreview-review-reply

Comment on attachment 8889105 [details]
Bug 1356532: Part 4 - Avoid layout flushes during adjustSiteIconStart() and _handleOverflow().

https://reviewboard.mozilla.org/r/160132/#review179948

Huh. Interesting. I had to single-step through the video to see this. I can only reproduce it locally the first time the drop-down opens, so it presumably has something to do with the initial offset value.

> What's causing this non determinism? Is it because we sometimes have 3 frames and sometimes only 2 before we are done displaying the panel? (this could do with a comment here in the test)
> 
> Is there anything we can do about it to have always the same number of reflows here?
> 
> I almost wonder if BrowserUtils.forceReflow should be handled by head.js, with a verification that it happens at most once per requestAnimationFrame, and only a boolean in the test file to say that the code being tested is expected to use BrowserUtils.forceReflow. Maybe this is overkill though.

It's timing-related, and depends on how many times we're asked to update per-frame, and when exactly in the frame it happens. So, for example, either of the following sequences are possible when we update twice during a frame:

1) - We're asked to add a new item to the list, and request a layout flush.
   - There's enough time for an idle callback before the next animation frame, so we force a reflow during an idle slice and do layout calculations.
   - A new result comes in, so we add it to the list and request a layout flush.
   - There's not enough time for an idle callback, so we force a layout flush during the next animation frame.

2) - We're asked to add a new frame to the list, and request a layout flush.
   - A new result comes in, so we add it to the list, with a layout flush still pending.
   - There's not enough time for an idle callback, so we force a layout flush during the next animation frame.

It's hard to make the above deterministic. We could remove the idle callback logic, which would reduce the number of reflows between frames, but at the expense of pushing back the next animation frame more often.

Even if we did, the `_handleOverflow` function currently tries to avoid multiple operations within the same frame by storing the promise for an existing calculation. Which means that two updates within the same frame would still produce fewer flushes than two updates in consecutive frames, and I don't think we want to change that.

> This doesn't seem to be used, please remove it. And if it was used, a global variable shared with all test files of the folder should have a longer name to avoid collisions.

Oops. I was using it to dump the existing flushes when I was updating the tests. I meant to remove it.

> This line is not a template string and could use normal double quotes.

I generally try to use the same string types when concatenating like this, even if not all of them use template interpolation. Partly for style reasons, partly because it's easy to add new template interpolation to one of the double quoted strings without noticing. But I don't have a super strong opinion on it.

	Florian Quèze [:florian]
	Comment 62 • 7 years ago

Is there any chance this could still ship with 57? Should we accept the initial flicker for 57 (as the current implementation flickers anyway) to get the improvements while typing/using the awesomebar?

	Marco Bonardo [:mak] Reporter
	Comment 63 • 7 years ago

I have a local patch that by avoiding completely the call to handleOverUnderflow in certain cases reduces its reflows in browser_urlbar_search_reflows by 654 and in browser_urlbar_keyed_search_reflows by 1284. I'll try to land that as part of bug 1391293, so this specific fix may be a bit less critical to have in 57 and could be delayed to 58, if we're scared about regressions or we want to address the remaining issues.

	Marco Bonardo [:mak] Reporter
	Comment 65 • 7 years ago

Note: I'm also copying over the minTimes idea in bug 1391293.

	Florian Quèze [:florian]
	Comment 66 • 6 years ago

In bug 1414126 I disabled the browser_urlbar_search_reflows.js test on Windows/Linux debug because it intermittently timed out (and always took ages to run). We should consider re-enabling it once this bug is fixed.

	Marco Bonardo [:mak] Reporter
	Comment 67 • 6 years ago

This is sort-of blocked by Bug 1434376.

	Florian Quèze [:florian]
	Comment 68 • 6 years ago

Now that bug 1434376 is fixed, maybe we can get this moving again? It's the most painful performance bug related to the urlbar panel.

	:Gijs (he/him)
	Comment 69 • 6 years ago

Stealing this per perf team discussions - Kris, I know you're busy and I don't see any activity here for the last few months, but let me know if you were about to revive this yourself! :-)

	:Gijs (he/him)
	Comment 70 • 6 years ago

What with the plans for the new architecture for the address bar that would come to replace the current autocomplete-based one, it's not clear that trying to fix this bug in the current architecture is a good use of time. As a result, I'm going to focus on other things at the moment. I'll unassign so that if anyone else wants to pick this back up, they can do so.

	Marco Bonardo [:mak] Reporter
	Comment 71 • 6 years ago

The re-architecture will take 6 months for an MVP, I still think this is worth it, so keeping it as a P1, I think we could at least fix the low hanging fruits here; I suspect with a better use of requestAnimationFrame and promiseDocumentFlushed we could likely obtain decent results without large changes.

	Mike Conley (:mconley) (:⚙️)
	Comment 72 • 6 years ago

(In reply to Marco Bonardo [::mak] from comment #71)
> I suspect with a better use of requestAnimationFrame
> and promiseDocumentFlushed we could likely obtain decent results without
> large changes.

Do you think we can accomplish this without flicker? As I recall, this was the primary concern when we started evaluating how promiseDocumentFlushed could help here.

	Marco Bonardo [:mak] Reporter
	Comment 73 • 6 years ago

Honestly it's hard to tell without trying, but it should be feasible at least for AdjustHeight. The overflow problem is a bit more complex.

	Mike Conley (:mconley) (:⚙️)
	Comment 74 • 6 years ago

(In reply to Marco Bonardo [::mak] from comment #73)
> Honestly it's hard to tell without trying, but it should be feasible at
> least for AdjustHeight.

Okay, that's bug 1358443. I guess let's discuss it over there.

	Marco Bonardo [:mak] Reporter
	Comment 75 • 6 years ago

This is complex to achieve without flickering and likely we'd better prioritize the new framework, lowering prio. We'll pick bug 1358443 instead.