1356532 - Try avoiding layout flushes in autocomplete::_handleOverflow

Status: NEW

(Toolkit :: Autocomplete, enhancement, P3)

Description

[Marco Bonardo [:mak]]

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

Comment 2

[Marco Bonardo [:mak]]

(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.

Comment 3

[Brian Carpenter [:geeknik]]

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

Comment 4

[Florian Quèze [:florian]]

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

Comment 5

[Kris Maglione [:kmag]]

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...

Comment 6

[Marco Bonardo [:mak]]

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.

Comment 7

[Florian Quèze [:florian]]

(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.

Comment 8

[Kris Maglione [:kmag]]

Attachment: Bug 1356532: Part 1 - Add BrowserUtils.promiseAnimationFrame helper.

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
callback.

This helper makes it possible to run DOM mutation code during an animation
frame in the course of an async function, and then resume execution after the
animation frame callback has been executed, and without danger of blocking the
frame from being painted.


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,
and we'll need to resolve the promise from a task dispatched to the event
loop.

Review commit: https://reviewboard.mozilla.org/r/160128/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/160128/

Comment 9

[Kris Maglione [:kmag]]

Attachment: Bug 1356532: Part 2 - Avoid layout flushes during adjustSiteIconStart().

Review commit: https://reviewboard.mozilla.org/r/160130/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/160130/

Comment 10

[Kris Maglione [:kmag]]

Attachment: Bug 1356532: Part 4 - Avoid layout flushes during adjustSiteIconStart() and _handleOverflow().

Review commit: https://reviewboard.mozilla.org/r/160132/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/160132/

Comment 11

[Kris Maglione [:kmag]]

Attachment: Bug 1356532: Part 5 - Avoid layout flushes during adjustHeight().

Review commit: https://reviewboard.mozilla.org/r/160134/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/160134/

Comment 12

[Kris Maglione [:kmag]]

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

Comment 13

[Kris Maglione [:kmag]]

*reflows (during overflow :) )

Comment 14

[Florian Quèze [:florian]]

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?

Comment 15

[Kris Maglione [:kmag]]

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.

Comment 16

[Kris Maglione [:kmag]]

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

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/160128/diff/1-2/

Comment 17

[Kris Maglione [:kmag]]

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

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/160130/diff/1-2/

Comment 18

[Kris Maglione [:kmag]]

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

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/160132/diff/1-2/

Comment 19

[Kris Maglione [:kmag]]

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

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/160134/diff/1-2/

Comment 20

[Marco Bonardo [:mak]]

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.

Comment 21

[Florian Quèze [:florian]]

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?

Comment 22

[Florian Quèze [:florian]]

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 :-).

Comment 23

[Florian Quèze [:florian]]

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(() => {
?

Comment 24

[Florian Quèze [:florian]]

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.

Comment 25

[Florian Quèze [:florian]]

(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.

Comment 26

[Kris Maglione [:kmag]]

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.

Comment 27

[Marco Bonardo [:mak]]

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).

Comment 28

[Florian Quèze [:florian]]

(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.

Comment 29

[Marco Bonardo [:mak]]

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

Comment 30

[Florian Quèze [:florian]]

(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.

Comment 31

[Dão Gottwald [:dao]]

(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.

Comment 32

[Mike Conley (:mconley) (:⚙️)]

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

Comment 33

[:Paolo Amadini]

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.

Comment 34

[Kris Maglione [:kmag]]

Attachment: Bug 1356532: Part 2 - Add helper for performing Promise microtask checkpoint from JS.

Review commit: https://reviewboard.mozilla.org/r/172954/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/172954/

Comment 35

[Kris Maglione [:kmag]]

Attachment: Bug 1356532: Part 3 - Add layout query helper that forces a single reflow before paint.

This is mostly a stopgap until code that relies on synchronous layout flushes
can be rewriteen to rely on the layout engine instead. But it allows existing
code to be reworked so that only one synchronous layout flush is required per
frame, and tries to perform them during idle slices when possible.

Review commit: https://reviewboard.mozilla.org/r/172956/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/172956/

Comment 36

[Kris Maglione [:kmag]]

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

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/160128/diff/2-3/

Comment 37

[Kris Maglione [:kmag]]

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

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/160132/diff/2-3/

Comment 38

[Kris Maglione [:kmag]]

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

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/160134/diff/2-3/

Comment 39

[Kris Maglione [:kmag]]

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.

Comment 40

[Marco Bonardo [:mak]]

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

Comment 41

[Kris Maglione [:kmag]]

(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.

Comment 42

[Marco Bonardo [:mak]]

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.

Comment 43

[Marco Bonardo [:mak]]

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.

Comment 44

[Florian Quèze [:florian]]

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'.

Comment 45

[Florian Quèze [:florian]]

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?

Comment 46

[Florian Quèze [:florian]]

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".

Comment 47

[Florian Quèze [:florian]]

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.

Comment 48

[Florian Quèze [:florian]]

Attachment: Screenshot

Comment 49

[Kris Maglione [:kmag]]

(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.

Comment 50

[Kris Maglione [:kmag]]

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...)

Comment 51

[Kris Maglione [:kmag]]

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 :)

Comment 52

[Kris Maglione [:kmag]]

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

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/160128/diff/3-4/

Comment 53

[Kris Maglione [:kmag]]

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

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/172954/diff/1-2/

Comment 54

[Kris Maglione [:kmag]]

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

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/172956/diff/1-2/

Comment 55

[Kris Maglione [:kmag]]

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

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/160132/diff/3-4/

Comment 56

[Kris Maglione [:kmag]]

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

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/160134/diff/3-4/

Comment 57

[Florian Quèze [:florian]]

(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! :-)

Comment 58

[Florian Quèze [:florian]]

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

Comment 59

[Florian Quèze [:florian]]

Attachment: Screencast

Comment 60

[Florian Quèze [:florian]]

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.

Comment 61

[Kris Maglione [:kmag]]

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.

Comment 62

[Florian Quèze [:florian]]

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?

Comment 63

[Marco Bonardo [:mak]]

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.

Comment 65

[Marco Bonardo [:mak]]

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

Comment 66

[Florian Quèze [:florian]]

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.

Comment 67

[Marco Bonardo [:mak]]

This is sort-of blocked by Bug 1434376.

Comment 68

[Florian Quèze [:florian]]

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.

Comment 69

[:Gijs (out for now; he/him)]

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! :-)

Comment 70

[:Gijs (out for now; he/him)]

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.

Comment 71

[Marco Bonardo [:mak]]

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.

Comment 72

[Mike Conley (:mconley) (:⚙️)]

(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.

Comment 73

[Marco Bonardo [:mak]]

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.

Comment 74

[Mike Conley (:mconley) (:⚙️)]

(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.

Comment 75

[Marco Bonardo [:mak]]

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

Comment 76

[Florian Quèze [:florian]]

The code still exists: https://searchfox.org/mozilla-central/rev/a06d5a8871b1796f2dbd588ab518eaa98507e018/toolkit/content/widgets/autocomplete-richlistitem.js#462-465