Closed Bug 1358719 Opened 7 years ago Closed 6 years ago

1.26ms uninterruptible reflow at PT__updateChevronTimerCallback@chrome://browser/content/places/browserPlacesViews.js:1205:22

Categories

(Firefox :: Bookmarks & History, enhancement, P3)

Product:
Firefox
Component:
Bookmarks & History
Type:
enhancement

Tracking

()

Status:
RESOLVED FIXED
Milestone:
Firefox 61
Project Flags:
Performance Impact high
Tracking Flags:
Tracking Status
firefox61 --- fixed

People

(Reporter: geeknik, Assigned: mconley)

References

(Blocks 1 open bug)

Details

(Keywords: perf, Whiteboard: [ohnoreflow][fxperf:p1])

Attachments

(2 files)

Bug 1358719 - Get rid of synchronous layout flush in browserPlacesViews.js.
59 bytes, text/x-review-board-request
mak
: review+
Details
Bug 1358719 - Add window resize reflow test.
59 bytes, text/x-review-board-request
florian
: review+
Details 
Here's the stack:

PT__updateChevronTimerCallback@chrome://browser/content/places/browserPlacesViews.js:1205:22
PT_notify@chrome://browser/content/places/browserPlacesViews.js:1470:7
Several getBoundingClientRect calls in there.
Component: Untriaged → Bookmarks & History
Flags: qe-verify?
Priority: -- → P2
Whiteboard: [ohnoreflow][qf][photon-performance] → [ohnoreflow][qf:p1][photon-performance]
Priority: P2 → P3
Whiteboard: [ohnoreflow][qf:p1][photon-performance] → [ohnoreflow][qf:p1][reserve-photon-performance]
Flags: qe-verify? → qe-verify-
Whiteboard: [ohnoreflow][qf:p1][reserve-photon-performance] → [ohnoreflow][qf:p2][reserve-photon-performance]
Priority: P3 → P4
Keywords: perf
This appears to still be happening:

https://searchfox.org/mozilla-central/rev/9f3bd430c2b132c86c46126a0548661de876799a/browser/components/places/content/browserPlacesViews.js#1246

It looks like using getBoundsWithoutFlushing was investigated: https://searchfox.org/mozilla-central/rev/9f3bd430c2b132c86c46126a0548661de876799a/browser/components/places/content/browserPlacesViews.js#1600-1603
Whiteboard: [ohnoreflow][qf:p2][reserve-photon-performance] → [ohnoreflow][qf:p1][reserve-photon-performance]
Whiteboard: [ohnoreflow][qf:p1][reserve-photon-performance] → [ohnoreflow][qf:i60][qf:p1][reserve-photon-performance]
Whiteboard: [ohnoreflow][qf:i60][qf:p1][reserve-photon-performance] → [ohnoreflow][qf:f60][qf:p1][reserve-photon-performance]
Priority: P4 → P3
Whiteboard: [ohnoreflow][qf:f60][qf:p1][reserve-photon-performance] → [ohnoreflow][qf:f61][qf:p1][reserve-photon-performance]
Whiteboard: [ohnoreflow][qf:f61][qf:p1][reserve-photon-performance] → [ohnoreflow][qf:f61][qf:p1][fxperf]
Now that promiseDocumentFlushed exists, I think we can take some of this on.
Assignee: nobody → mconley
Whiteboard: [ohnoreflow][qf:f61][qf:p1][fxperf] → [ohnoreflow][qf:f61][qf:p1][fxperf:p2]
Depends on: 1442020
Whiteboard: [ohnoreflow][qf:f61][qf:p1][fxperf:p2] → [ohnoreflow][qf:f61][qf:p1][fxperf:p1]
Comment on attachment 8954907 [details]
Bug 1358719 - Get rid of synchronous layout flush in browserPlacesViews.js.

https://reviewboard.mozilla.org/r/224070/#review230276

::: browser/components/places/content/browserPlacesViews.js:1273
(Diff revision 3)
>      let childOverflowed = false;
>      for (let child of this._rootElt.childNodes) {
>        // Once a child overflows, all the next ones will.
>        if (!childOverflowed) {
> +        await window.promiseDocumentFlushed(() => {
> -        let childRect = child.getBoundingClientRect();
> +          let childRect = child.getBoundingClientRect();

Could we use GetBoundsWithoutFlushing at this point, since we already flushed a few rows above?

This for can loop for a lot of entries (even thousands) and I'm a little bit worried about awaiting inside it.

::: browser/components/places/content/browserPlacesViews.js:1280
(Diff revision 3)
> -                                     : (childRect.right > scrollRect.right);
> +                                       : (childRect.right > scrollRect.right);
> +        });
>        }
>  
>        if (childOverflowed) {
> +        window.requestAnimationFrame(() => {

if we can avoid awaiting on documentflushed, could we loop the whole for inside a single requestAnimationFrame?
Again, I'm a little bit worried of enqueuing a thousand of RAF callbacks.
Comment on attachment 8954907 [details]
Bug 1358719 - Get rid of synchronous layout flush in browserPlacesViews.js.

https://reviewboard.mozilla.org/r/224070/#review231350

Clearing while waiting for answers... in any case maybe we should delay these to 61 to avoid introducing unexpected flickering or issues in 60.
Attachment #8954907 - Flags: review?(mak77)
Comment on attachment 8954907 [details]
Bug 1358719 - Get rid of synchronous layout flush in browserPlacesViews.js.

https://reviewboard.mozilla.org/r/224070/#review231980

Looks good to me, though I didn't have a chance to verify in on the field, so please check this doesn't cause unexpected flickering in both non-overflowed and overflowed bookmarks toolbar cases. Other cases to test are when entering/exiting customize mode, and resizing the window.

::: browser/components/places/content/browserPlacesViews.js:1258
(Diff revision 4)
>        this._updateNodesVisibilityTimer.cancel();
>  
>      this._updateNodesVisibilityTimer = this._setTimer(100);
>    },
>  
> -  _updateNodesVisibilityTimerCallback: function PT__updateNodesVisibilityTimerCallback() {
> +  _updateNodesVisibilityTimerCallback: async function PT__updateNodesVisibilityTimerCallback() {

We don't need both name and label anymore, so you can use the shorthand version here. we usually nodernize this code when we touch it.

::: browser/components/places/content/browserPlacesViews.js:1263
(Diff revision 4)
> -  _updateNodesVisibilityTimerCallback: function PT__updateNodesVisibilityTimerCallback() {
> -    let scrollRect = this._rootElt.getBoundingClientRect();
> +  _updateNodesVisibilityTimerCallback: async function PT__updateNodesVisibilityTimerCallback() {
> +    if (this._updatingNodesVisibility) {
> +      return;
> +    }
> +
> +    this._updatingNodesVisibility = true;

nit: I'd remove the above newline since it's related to the previous if.

::: browser/components/places/content/browserPlacesViews.js:1266
(Diff revision 4)
> +    }
> +
> +    this._updatingNodesVisibility = true;
> +
> +    let scrollRect = await window.promiseDocumentFlushed(() => {
> +      return this._rootElt.getBoundingClientRect();

nit: you can avoid the {return} and just:
await window.promiseDocumentFlushed(
  () => this._rootElt.getBoundingClientRect());

::: browser/components/places/content/browserPlacesViews.js:1271
(Diff revision 4)
> +      return this._rootElt.getBoundingClientRect();
> +    });
>      let childOverflowed = false;
> +
> +    let dwu = window.QueryInterface(Ci.nsIInterfaceRequestor)
> +                    .getInterface(Ci.nsIDOMWindowUtils);

We started using this far more often, I see a lof of code in browser that does this QI+GI, that likely has an overhead cost. What about we add a DOMWindowUtils lazy getter in browser.js, use that here, and file a bug to replace wild usage around?
Attachment #8954907 - Flags: review?(mak77) → review+
(In reply to Marco Bonardo [::mak] from comment #11)

> ::: browser/components/places/content/browserPlacesViews.js:1271
> (Diff revision 4)
> > +      return this._rootElt.getBoundingClientRect();
> > +    });
> >      let childOverflowed = false;
> > +
> > +    let dwu = window.QueryInterface(Ci.nsIInterfaceRequestor)
> > +                    .getInterface(Ci.nsIDOMWindowUtils);
> 
> We started using this far more often, I see a lof of code in browser that
> does this QI+GI, that likely has an overhead cost. What about we add a
> DOMWindowUtils lazy getter in browser.js, use that here, and file a bug to
> replace wild usage around?

Or could we just get this on the window object as a chromeonly property, like promiseDocumentFlushed?
Comment on attachment 8956961 [details]
Bug 1358719 - Add window resize reflow test.

https://reviewboard.mozilla.org/r/225906/#review232050

::: browser/base/content/test/performance/browser_window_resize_reflows.js:20
(Diff revision 1)
> +const EXPECTED_REFLOWS = [
> +  {
> +    stack: [
> +      "onOverflow@resource:///modules/CustomizableUI.jsm",
> +      "handleEvent@chrome://browser/content/customizableui/toolbar.xml",
> +      "EventListener.handleEvent*toolbar_XBL_Constructor@chrome://browser/content/customizableui/toolbar.xml",

I think this will fail on beta. If I remember correctly, the feature to keep track of what code has setup the event listener is causing memory overhead and so is only pref'ed on on Nightly. I would remove the last line of this stack.

::: browser/base/content/test/performance/browser_window_resize_reflows.js:38
(Diff revision 1)
> +
> +const gToolbar = document.getElementById("PersonalToolbar");
> +
> +/**
> + * Sets the visibility state on the Bookmarks Toolbar, and
> + * waits for it to tranistion to fully visible.

"tranistion"

::: browser/base/content/test/performance/browser_window_resize_reflows.js:43
(Diff revision 1)
> + * waits for it to tranistion to fully visible.
> + */
> +async function toggleBookmarksToolbar(visible) {
> +  let transitionPromise =
> +    BrowserTestUtils.waitForEvent(gToolbar, "transitionend", (e) => {
> +      return e.propertyName == "max-height";

nit: I would format it this way:
    BrowserTestUtils.waitForEvent(gToolbar, "transitionend",
                                  e => e.propertyName == "max-height");
Attachment #8956961 - Flags: review?(florian) → review+
Comment on attachment 8954907 [details]
Bug 1358719 - Get rid of synchronous layout flush in browserPlacesViews.js.

https://reviewboard.mozilla.org/r/224070/#review231980

> We started using this far more often, I see a lof of code in browser that does this QI+GI, that likely has an overhead cost. What about we add a DOMWindowUtils lazy getter in browser.js, use that here, and file a bug to replace wild usage around?

Great idea, will file.
Comment on attachment 8956961 [details]
Bug 1358719 - Add window resize reflow test.

https://reviewboard.mozilla.org/r/225906/#review232050

> I think this will fail on beta. If I remember correctly, the feature to keep track of what code has setup the event listener is causing memory overhead and so is only pref'ed on on Nightly. I would remove the last line of this stack.

Ah, great catch, thanks.
Getting this test to run reliably on automation is kinda tricky, what with the variety of display sizes and resolutions. In particular, I'm having to deal with the fact that on some of our test machines, displays are so small that the new window starts out in sizemode maximized, and so switching sizemodes causes extra reflows. I also have to deal with the overflowable navigation toolbar, and figuring out how many items are going to overflow and cause reflows. :/

I might need a second review on the testing patch once I've had some time to run it in automation and get all greens. Sorry about that.
Filed bug 1444213 about doing something with nsIDOMWindowUtils.
Hey florian and mak, I had to alter these patches somewhat in order to get green from try. Can I carry your r+'s forward?
Flags: needinfo?(mak77)
Flags: needinfo?(florian)
no problem on my side, I just had a quick look everything looks ok
Flags: needinfo?(mak77)
(In reply to Mike Conley (:mconley) (:⚙️) from comment #22)
> Can I carry your r+'s forward?

Yes, thanks for making the lazy resize thing deterministic! It's been in my way too!
Flags: needinfo?(florian)
Thanks!
Pushed by mconley@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/acc79e65a9b3
Get rid of synchronous layout flush in browserPlacesViews.js. r=mak
https://hg.mozilla.org/integration/autoland/rev/be7a5a989cb3
Add window resize reflow test. r=florian
https://hg.mozilla.org/mozilla-central/rev/acc79e65a9b3
https://hg.mozilla.org/mozilla-central/rev/be7a5a989cb3
Status: NEW → RESOLVED
Closed: 6 years ago
status-firefox61: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 61
Performance Impact: --- → P1
Whiteboard: [ohnoreflow][qf:f61][qf:p1][fxperf:p1] → [ohnoreflow][fxperf:p1]
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: