Closed Bug 1383367 Opened 7 years ago Closed 7 years ago

Add JS helper to determine if a layout flush is required

Categories

(Core :: DOM: Core & HTML, enhancement)

enhancement
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla56
Performance Impact high
Tracking Status
firefox56 --- fixed

People

(Reporter: kmag, Assigned: kmag)

References

(Blocks 4 open bugs)

Details

Attachments

(3 files, 2 obsolete files)

      No description provided.
Whiteboard: [qf]
Blocks: 1358712
Comment on attachment 8889013 [details]
Bug 1383367: Part 3 - Defer getComputedStyle call if a reflow is currently pending.

https://reviewboard.mozilla.org/r/160058/#review165452

This new helper is an exciting addition to our APIs, it should make it easy to fix sync reflows that were currently hardly possible to fix with front-end code only :-).

::: browser/base/content/browser.js:8708
(Diff revision 1)
>        // toolbars *should* all have ids, but guard anyway to avoid blowing up
>        let cacheKey = toolbar.id && toolbar.id + JSON.stringify(this._windowState);
>        // lookup cached luminance value for this toolbar in this window state
>        let luminance = cacheKey && cachedLuminances.get(cacheKey);
>        if (isNaN(luminance)) {
> +        await PromiseUtils.layoutFlushed(document, "style", () => {

Should we make PromiseUtils.layoutFlushed's callback parameter optional?
In this case, it looks like it would be easier to just write:
  await PromiseUtils.layoutFlushed(document, "style");
  /* do stuff using getComputedStyle */

::: browser/base/content/browser.js:8723
(Diff revision 1)
>  
>      for (let [toolbar, luminance] of luminances) {
>        if (luminance <= 110)
>          toolbar.removeAttribute("brighttext");
>        else
>          toolbar.setAttribute("brighttext", "true");

Isn't the dom attribute change here enough to require another style flush very soon? Should this go in a requestAnimationFrame?
Comment on attachment 8889013 [details]
Bug 1383367: Part 3 - Defer getComputedStyle call if a reflow is currently pending.

https://reviewboard.mozilla.org/r/160058/#review165452

> Should we make PromiseUtils.layoutFlushed's callback parameter optional?
> In this case, it looks like it would be easier to just write:
>   await PromiseUtils.layoutFlushed(document, "style");
>   /* do stuff using getComputedStyle */

Nope! In fact, I made it non-optional specifically to discourage people from doing things like that. :)

The key to this approach is that nothing must make any changes that would trigger a layout flush between the time the reflow finishes and the time any of the callbacks query the layout state. For callbacks, that's guaranteed, as long as all of the queries happen in the callback, and none of the callbacks change the DOM state.

But once we get into the promise microtask queue, all bets are off as to the order things run in, and what else runs before/interspersed with your code.

> Isn't the dom attribute change here enough to require another style flush very soon? Should this go in a requestAnimationFrame?

Not really. It only requires another layout flush the next time there's a refresh tick, or the next time someone tries to query layout information. Ideally, this probably should use requestAnimationFrame, but there are so many places in the browser frontend code where we currently change the DOM without RAF that it probably doesn't make a difference until we make a concerted effort to change most of them.

But I suppose there's probably no harm in fixing things as we come across them.
Blocks: 1358816
Blocks: 1356532
Comment on attachment 8889012 [details]
Bug 1383367: Part 2 - Add promise helpers to defer operations until after a reflow.

https://reviewboard.mozilla.org/r/160056/#review165640

::: toolkit/modules/PromiseUtils.jsm:24
(Diff revision 1)
> +  doc.docShell.addWeakReflowObserver(this);
> +  reflowObservers.set(this._doc, this);
> +
> +  this.callbacks = [];
> +
> +  this.promise = new Promise(resolve => {

I don't see anything using this promise.

::: toolkit/modules/PromiseUtils.jsm:37
(Diff revision 1)
> +  _resolve() {
> +    reflowObservers.delete(this._doc);
> +    this._doc.docShell.removeWeakReflowObserver(this);
> +
> +    for (let callback of this.callbacks) {
> +      try {

I think this try/catch is duplicated with the one in the reflowed method, and will eat exceptions that the second try/catch uses to reject promises.

::: toolkit/modules/PromiseUtils.jsm:95
(Diff revision 1)
> +    }
> +
> +    return new Promise((resolve, reject) => {
> +      observer.callbacks.push(() => {
> +        try {
> +          resolve(callback());

Should this be:

let result = callback();
observer.promise.then(resolve(result));

?

::: toolkit/modules/PromiseUtils.jsm:121
(Diff revision 1)
> +   *          - "layout"
> +   *          - "display"
> +   * @param {function} callback
> +   * @returns {Promise}
> +   */
> +  async layoutFlushed(doc, flushType, callback) {

Marco's bug 1356532 comment 20 applies here too, this would likely be better in BrowserUtils.
How do we avoid this leading to flashes of 'wrong' icons (plus first loading the wrong icons, then the right ones) ?
Depends on: 1334642
Comment on attachment 8889012 [details]
Bug 1383367: Part 2 - Add promise helpers to defer operations until after a reflow.

https://reviewboard.mozilla.org/r/160056/#review165684

::: toolkit/modules/PromiseUtils.jsm:78
(Diff revision 1)
> +  /**
> +   * Calls the given function when the given document has just reflowed,
> +   * and returns a promise which resolves after the function has been
> +   * called.
> +   *
> +   * The function *must not trigger any reflows*, or make any changes

Could we ensure this by making tests fail very clearly in debug builds when this requirement isn't respected?

I'm thinking that instead of running callback() directly, we could have a _runCallback method that in debug builds would add a reflow observer and assert that it's not getting called. Also assert that utils.needsFlush is still false after we are done running the callback.

::: toolkit/modules/PromiseUtils.jsm:85
(Diff revision 1)
> +   *
> +   * @param {Document} doc
> +   * @param {function} callback
> +   * @returns {Promise}
> +   */
> +  reflowed(doc, callback) {

This function needs to be whitelisted for the reflow observer used by our reflow tests.

Could be something as simple as adding:

if (path.includes("reflowed/</<@resource://gre/modules/PromiseUtils.jsm"))
  return;

at http://searchfox.org/mozilla-central/rev/3a3af33f513071ea829debdfbc628caebcdf6996/browser/base/content/test/performance/head.js#108
Comment on attachment 8889011 [details]
Bug 1383367: Part 1 - Add JS helper to determine if a layout flush is required.

https://reviewboard.mozilla.org/r/160054/#review165812

Yes!!! This was sorely needed. Thank you!
Attachment #8889011 - Flags: review?(mconley) → review+
Comment on attachment 8889013 [details]
Bug 1383367: Part 3 - Defer getComputedStyle call if a reflow is currently pending.

https://reviewboard.mozilla.org/r/160058/#review165814

Thanks!
Attachment #8889013 - Flags: review?(mconley) → review+
Comment on attachment 8889012 [details]
Bug 1383367: Part 2 - Add promise helpers to defer operations until after a reflow.

https://reviewboard.mozilla.org/r/160056/#review165836

This is really exciting stuff, kmag. Thanks so much for it.

A firefox-dev (and maybe even a dev-platform) post about this, when landed, would be excellent. The Photon Performance team will definitely socialize the hell out of this API.

::: toolkit/modules/PromiseUtils.jsm:32
(Diff revision 1)
> +}
> +
> +ReflowObserver.prototype = {
> +  QueryInterface: XPCOMUtils.generateQI(["nsIReflowObserver", "nsISupportsWeakReference"]),
> +
> +  _resolve() {

Maybe I'm just re-phrasing florian's review, but this `_resolve()` method, and the `_resolvePromise()` look like leftovers from an earlier iteration where the ReflowObserver was using Promises instead of callbacks.

So I'd suggest removing the `this.promise` and `_resolvePromise()` stuff entirely, and maybe call this method `onReflow()` instead.

::: toolkit/modules/PromiseUtils.jsm:78
(Diff revision 1)
> +  /**
> +   * Calls the given function when the given document has just reflowed,
> +   * and returns a promise which resolves after the function has been
> +   * called.
> +   *
> +   * The function *must not trigger any reflows*, or make any changes

I concur - having a "fatal exception" (whether that's a crash or just throwing) occur if `needsFlush` is true after the callback runs seems like it'd be a good idea.

::: toolkit/modules/PromiseUtils.jsm:82
(Diff revision 1)
> +   * @param {function} callback
> +   * @returns {Promise}

If we're going to support having the function return a result that gets passed to the `then` function, we should probably call that out here.
Attachment #8889012 - Flags: review?(mconley) → review-
Whiteboard: [qf] → [qf:p1]
Comment on attachment 8889012 [details]
Bug 1383367: Part 2 - Add promise helpers to defer operations until after a reflow.

https://reviewboard.mozilla.org/r/160056/#review165684

> Could we ensure this by making tests fail very clearly in debug builds when this requirement isn't respected?
> 
> I'm thinking that instead of running callback() directly, we could have a _runCallback method that in debug builds would add a reflow observer and assert that it's not getting called. Also assert that utils.needsFlush is still false after we are done running the callback.

Ideally, yes, but it's easier said than done, in part because needsFlush will always return true when called from a reflow observer. I'm happy to work on that in a follow-up, though.

> This function needs to be whitelisted for the reflow observer used by our reflow tests.
> 
> Could be something as simple as adding:
> 
> if (path.includes("reflowed/</<@resource://gre/modules/PromiseUtils.jsm"))
>   return;
> 
> at http://searchfox.org/mozilla-central/rev/3a3af33f513071ea829debdfbc628caebcdf6996/browser/base/content/test/performance/head.js#108

I don't see why. This should never appear in a reflow observer stack if it's used correctly.
Comment on attachment 8889012 [details]
Bug 1383367: Part 2 - Add promise helpers to defer operations until after a reflow.

https://reviewboard.mozilla.org/r/160056/#review165836

> Maybe I'm just re-phrasing florian's review, but this `_resolve()` method, and the `_resolvePromise()` look like leftovers from an earlier iteration where the ReflowObserver was using Promises instead of callbacks.
> 
> So I'd suggest removing the `this.promise` and `_resolvePromise()` stuff entirely, and maybe call this method `onReflow()` instead.

Yeah, I stopped using the promise after I decided it would make more sense to return the result of the individual handler callbacks, but decided to leave it in case I needed it elsewhere. But it should be simple enough to just re-add in that case.

> I concur - having a "fatal exception" (whether that's a crash or just throwing) occur if `needsFlush` is true after the callback runs seems like it'd be a good idea.

That requires some changes to the way needsFlush works, since it currently always returns true when called from a reflow observer. I'll file a follow-up for this, though.
(In reply to Kris Maglione [:kmag] from comment #12)

> > This function needs to be whitelisted for the reflow observer used by our reflow tests.
> > 
> > Could be something as simple as adding:
> > 
> > if (path.includes("reflowed/</<@resource://gre/modules/PromiseUtils.jsm"))
> >   return;
> > 
> > at http://searchfox.org/mozilla-central/rev/3a3af33f513071ea829debdfbc628caebcdf6996/browser/base/content/test/performance/head.js#108
> 
> I don't see why. This should never appear in a reflow observer stack if it's
> used correctly.

Because our reflow tests intentionally dirty the frame tree after each reflow so that we can catch functions doing several things in a row that would each cause a reflow.
(In reply to Florian Quèze [:florian] [:flo] (away until August 7th) from comment #14)
> Because our reflow tests intentionally dirty the frame tree after each
> reflow so that we can catch functions doing several things in a row that
> would each cause a reflow.

I'd rather not add an exception for this, since those tests should really fail for any reflows triggered by these callbacks. Ideally, we should just be able to arrange things so that we only dirty the layout state after all of the reflow observers have been called...
Comment on attachment 8889013 [details]
Bug 1383367: Part 3 - Defer getComputedStyle call if a reflow is currently pending.

https://reviewboard.mozilla.org/r/160058/#review166190

::: browser/base/content/browser.js:8702
(Diff revision 2)
>  
>      // The getComputedStyle calls and setting the brighttext are separated in
>      // two loops to avoid flushing layout and making it dirty repeatedly.
>      let cachedLuminances = this._toolbarLuminanceCache;
>      let luminances = new Map();
> +    await BrowserUtils.promiseLayoutFlushed(document, "style", () => {

(In reply to :Gijs from comment #7)
> How do we avoid this leading to flashes of 'wrong' icons (plus first loading
> the wrong icons, then the right ones) ?

This seems like a valid concern, I was going to say the same.
(In reply to Dão Gottwald [::dao] from comment #19)
> (In reply to :Gijs from comment #7)
> > How do we avoid this leading to flashes of 'wrong' icons (plus first loading
> > the wrong icons, then the right ones) ?
> 
> This seems like a valid concern, I was going to say the same.

I've been thinking about that too, and was planning to mention it as a precaution when I post to firefox-dev about these helpers.

In this particular case, I haven't been able to trigger any noticeable flickering, so I think it's worth the risk for the sake eliminating an extra 3.5ms layout flush before first paint (on my test machine, anyway). If it turns out to be problematic, I think we should be able to store initial values in the xulCache or startup cache to at least limit the cases where we see a flicker or have to force a layout flush.
Comment on attachment 8889012 [details]
Bug 1383367: Part 2 - Add promise helpers to defer operations until after a reflow.

https://reviewboard.mozilla.org/r/160056/#review166794

\o/

Thanks so much for this. Sentiment during yesterday's Photon meeting was very positive for these patches. Will you have time to write a firefox-dev post about it?
Attachment #8889012 - Flags: review?(mconley) → review+
https://hg.mozilla.org/integration/mozilla-inbound/rev/6679237a46723432264361b5542454bb91d4831e
Bug 1383367: Part 1 - Add JS helper to determine if a layout flush is required. r=mconley

https://hg.mozilla.org/integration/mozilla-inbound/rev/7e6da74099fe44de96396783c43b8e3e0c5b70a4
Bug 1383367: Part 2 - Add promise helpers to defer operations until after a reflow. r=mconley

https://hg.mozilla.org/integration/mozilla-inbound/rev/1f9ec38274940ff20a1bfc0a6b8c76ea5ae16cff
Bug 1383367: Part 3 - Defer getComputedStyle call if a reflow is currently pending. r=mconley
sorry had to back this out for conflicting with the m-c to m-i merge like merging modules/libpref/init/all.js
warning: conflicts while merging browser/base/content/test/performance/browser_windowopen_reflows.js! (edit, then use 'hg resolve --mark')
66 files updated, 5 files merged, 3 files removed, 1 files unresolved
Flags: needinfo?(kmaglione+bmo)
Backout by cbook@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/466818a625e9
Backed out changeset 1f9ec3827494 
https://hg.mozilla.org/integration/mozilla-inbound/rev/f74d81e43bb5
Backed out changeset 7e6da74099fe 
https://hg.mozilla.org/integration/mozilla-inbound/rev/d53d6c1d5c70
Backed out changeset 6679237a4672 for conflicting with m-c to m-i merge
https://hg.mozilla.org/integration/mozilla-inbound/rev/46d1fa12a0829046f2bee4ffd10d7af38616bba9
Bug 1383367: Part 1 - Add JS helper to determine if a layout flush is required. r=mconley

https://hg.mozilla.org/integration/mozilla-inbound/rev/9c966265dc4834242369f56bf7bac91abf44998e
Bug 1383367: Part 2 - Add promise helpers to defer operations until after a reflow. r=mconley

https://hg.mozilla.org/integration/mozilla-inbound/rev/ecaae0733ca1dc08db7e8defe5c572f8d708d8a6
Bug 1383367: Part 3 - Defer getComputedStyle call if a reflow is currently pending. r=mconley
Backed out for failing browser_windowopen_reflows.js after merge, especially on OS X:

https://hg.mozilla.org/mozilla-central/rev/6d1b50a370b4adffbb1ee73b9f51707c90d6a2b1
https://hg.mozilla.org/mozilla-central/rev/c23906663721688473844f931bbc59de938517f2
https://hg.mozilla.org/mozilla-central/rev/7051e8c01179ae255a323197e1e1c32d832e20d7

This started after this merge to inbound: https://treeherder.mozilla.org/#/jobs?repo=mozilla-inbound&revision=a09d4e4b31bc001d2a050fa311d32fca37b28db4&filter-resultStatus=testfailed&filter-resultStatus=busted&filter-resultStatus=exception&filter-resultStatus=retry&filter-resultStatus=usercancel&filter-resultStatus=success&filter-resultStatus=pending&filter-resultStatus=running&filter-searchStr=ccd49af3bbddd41c691ed3a2b56132ff8c6c54e9
A backout on Try had all browser-chrome tests on OSX opt passing (else it failed in bc5 with and without e10s).

Failure log: https://treeherder.mozilla.org/logviewer.html#?job_id=119350005&repo=mozilla-central

03:25:15     INFO - TEST-PASS | browser/base/content/test/performance/browser_windowopen_reflows.js | expected uninterruptible reflow: '[
03:25:15     INFO - 	"rect@chrome://browser/content/browser-tabsintitlebar.js:106:23",
03:25:15     INFO - 	"_update@chrome://browser/content/browser-tabsintitlebar.js:175:35",
03:25:15     INFO - 	"updateAppearance@chrome://browser/content/browser-tabsintitlebar.js:65:5",
03:25:15     INFO - 	"handleEvent@chrome://browser/content/tabbrowser.xml:6639:15",
03:25:15     INFO - 	"EventListener.handleEvent*tabbrowser-tabs_XBL_Constructor@chrome://browser/content/tabbrowser.xml:6028:11",
03:25:15     INFO - 	""
03:25:15     INFO - ]' - true == true - 
03:25:15     INFO - Buffered messages finished
03:25:15     INFO - TEST-UNEXPECTED-FAIL | browser/base/content/test/performance/browser_windowopen_reflows.js | unexpected uninterruptible reflow 
03:25:15     INFO - [
03:25:15     INFO - 	"handleEvent@chrome://browser/content/tabbrowser.xml:6641:19",
03:25:15     INFO - 	"EventListener.handleEvent*tabbrowser-tabs_XBL_Constructor@chrome://browser/content/tabbrowser.xml:6028:11",
03:25:15     INFO - 	""
03:25:15     INFO - ]
03:25:15     INFO -  - false == true - JS frame :: chrome://mochitests/content/browser/browser/base/content/test/performance/head.js :: reflow :: line 114
03:25:15     INFO - Stack trace:
03:25:15     INFO - chrome://mochitests/content/browser/browser/base/content/test/performance/head.js:reflow:114
03:25:15     INFO - chrome://browser/content/tabbrowser.xml:handleEvent:6641
03:25:15     INFO - Console message: OpenGL compositor Initialized Succesfully.
03:25:15     INFO - Version: 2.1 INTEL-10.6.33
03:25:15     INFO - Vendor: Intel Inc.
03:25:15     INFO - Renderer: Intel Iris OpenGL Engine
03:25:15     INFO - FBO Texture Target: TEXTURE_2D
03:25:15     INFO - TEST-PASS | browser/base/content/test/performance/browser_windowopen_reflows.js | expected uninterruptible reflow: '[
03:25:15     INFO - 	"select@chrome://global/content/bindings/textbox.xml:115:11",
03:25:15     INFO - 	"focusAndSelectUrlBar@chrome://browser/content/browser.js:2280:5",
03:25:15     INFO - 	"_delayedStartup@chrome://browser/content/browser.js:1574:10",
03:25:15     INFO - 	"EventListener.handleEvent*onLoad@chrome://browser/content/browser.js:1410:5",
03:25:15     INFO - 	"onload@chrome://browser/content/browser.xul:1:1",
03:25:15     INFO - 	""
03:25:15     INFO - ]' - true == true - 
03:25:15     INFO - Not taking screenshot here: see the one that was previously logged
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Depends on: 1385729
Assignee: kmaglione+bmo → amiyaguchi
Status: REOPENED → ASSIGNED
Attachment #8892017 - Attachment is obsolete: true
https://hg.mozilla.org/integration/mozilla-inbound/rev/2e872bf7791da6b36486e1ce6500d536b71ed9fb
Bug 1383367: Part 1 - Add JS helper to determine if a layout flush is required. r=mconley

https://hg.mozilla.org/integration/mozilla-inbound/rev/2729aa6de0c1783055d21d914c142300051b4a6d
Bug 1383367: Part 2 - Add promise helpers to defer operations until after a reflow. r=mconley
Summary: Fix uninterruptable reflow in ToolbarIconColor::inferFromText → Add JS helper to determine if a layout flush is required
Attachment #8889013 - Attachment is obsolete: true
Component: Toolbars and Customization → DOM
Product: Firefox → Core
Target Milestone: Firefox 56 → ---
Target Milestone: --- → mozilla56
Blocks: 1385729
No longer depends on: 1385729
Here's the email that went out to firefox-dev about this: https://mail.mozilla.org/pipermail/firefox-dev/2017-August/005675.html
Depends on: 1397360
Depends on: 1434376
Flags: needinfo?(kmaglione+bmo)
Component: DOM → DOM: Core & HTML
Performance Impact: --- → P1
Whiteboard: [qf:p1]
You need to log in before you can comment on or make changes to this bug.