6.9ms uninterruptible reflow at onxblpopupshowing@chrome://global/content/bindings/popup.xml:268:1

NEW
Unassigned

Status

()

enhancement
P4
normal
2 years ago
7 months ago

People

(Reporter: rjward0, Unassigned)

Tracking

(Blocks 2 bugs, {perf})

unspecified
Points:
---
Dependency tree / graph
Bug Flags:
qe-verify -

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [ohnoreflow][qf:p3][fxperf:p3])

Attachments

(1 attachment)

Reporter

Description

2 years ago
Here's the stack:

onxblpopupshowing@chrome://global/content/bindings/popup.xml:268:1

triggered by hitting alt to reveal the menu bar and clicking on file.
Flags: qe-verify?
Priority: -- → P2

Updated

2 years ago
Component: Untriaged → XUL Widgets
Product: Firefox → Toolkit
Whiteboard: [ohnoreflow][qf][photon-performance] → [ohnoreflow][qf:p1][photon-performance]
Flags: qe-verify? → qe-verify-
Priority: P2 → P3
Whiteboard: [ohnoreflow][qf:p1][photon-performance] → [ohnoreflow][qf:p1][reserve-photon-performance]
Whiteboard: [ohnoreflow][qf:p1][reserve-photon-performance] → [ohnoreflow][qf:p2][reserve-photon-performance]
Duplicate of this bug: 1388504
Priority: P3 → P4
Keywords: perf
Whiteboard: [ohnoreflow][qf:p2][reserve-photon-performance] → [ohnoreflow][qf:p1][reserve-photon-performance]

Updated

2 years ago
Whiteboard: [ohnoreflow][qf:p1][reserve-photon-performance] → [ohnoreflow][qf:i60][qf:p1][reserve-photon-performance]

Updated

a year ago
Whiteboard: [ohnoreflow][qf:i60][qf:p1][reserve-photon-performance] → [ohnoreflow][qf:f60][qf:p1][reserve-photon-performance]

Updated

a year ago
Blocks: 1384733

Updated

a year ago
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][reserve-photon-performance] [fxperf]

Updated

a year ago
Whiteboard: [ohnoreflow][qf:f61][qf:p1][reserve-photon-performance] [fxperf] → [ohnoreflow][qf:f61][qf:p1][reserve-photon-performance] [fxperf:p2]
I'll add a test in a second commit on top of the one I just posted.
Assignee: nobody → mconley
Whiteboard: [ohnoreflow][qf:f61][qf:p1][reserve-photon-performance] [fxperf:p2] → [ohnoreflow][qf:f61][qf:p1][fxperf:p1]

Comment 5

a year ago
mozreview-review
Comment on attachment 8957358 [details]
Bug 1356765 - Avoid flushing layout when showing menus with accel shortcuts.

https://reviewboard.mozilla.org/r/226284/#review232206

r=me assuming this works as evidenced by a test - but see below... I'm a bit surprised no reviewbots or commit hook eslint things turned this up!

::: toolkit/content/widgets/popup.xml:293
(Diff revision 1)
> -            }
> +              }
> -          }
> +            }
> -          for (var i = 0; i < array.length; i++)
> +          }
> +          window.requestAnimationFrame(() => {
> +            for (let i = 0; i < array.length; i++) {
> -            array[i].width = width;
> +              array[i].width = width;

ahould be maxWidth on the RHS. :-)
Attachment #8957358 - Flags: review?(gijskruitbosch+bugs) → review+

Comment 6

a year ago
mozreview-review-reply
Comment on attachment 8957358 [details]
Bug 1356765 - Avoid flushing layout when showing menus with accel shortcuts.

https://reviewboard.mozilla.org/r/226284/#review232206

> ahould be maxWidth on the RHS. :-)

Ooof - how embarassing! Thanks - will correct and re-test.

Comment 7

a year ago
would it make sense to 

> requestAnimationFrame() 

only when the array actually contains elements?

Comment 8

a year ago
mozreview-review
Comment on attachment 8957358 [details]
Bug 1356765 - Avoid flushing layout when showing menus with accel shortcuts.

https://reviewboard.mozilla.org/r/226284/#review232342

::: toolkit/content/widgets/popup.xml:283
(Diff revision 1)
>              if (menuitem.localName == "menuitem" && menuitem.hasAttribute("acceltext")) {
> -              var accel = document.getAnonymousElementByAttribute(menuitem, "anonid", "accel");
> +              let accel = document.getAnonymousElementByAttribute(menuitem, "anonid", "accel");
>                if (accel && accel.boxObject) {
>                  array.push(accel);
> -                if (accel.boxObject.width > width)
> -                  width = accel.boxObject.width;
> +                // The popup is showing, which means it has already been laid
> +                // out, so these bounds should still be accurate.

How confident are you about this? Isn't the popupshowing event listener used by other code to insert menu items dynamically? Or are we relying (both with and without your patch) on dynamically inserted menu items never having any accel key?

::: toolkit/content/widgets/popup.xml:284
(Diff revision 1)
> -              var accel = document.getAnonymousElementByAttribute(menuitem, "anonid", "accel");
> +              let accel = document.getAnonymousElementByAttribute(menuitem, "anonid", "accel");
>                if (accel && accel.boxObject) {
>                  array.push(accel);
> -                if (accel.boxObject.width > width)
> -                  width = accel.boxObject.width;
> +                // The popup is showing, which means it has already been laid
> +                // out, so these bounds should still be accurate.
> +                let itemWidth = dwu.getBoundsWithoutFlushing(accel);

I doubt getBoundsWithoutFlushing returns a width.

::: toolkit/content/widgets/popup.xml:292
(Diff revision 1)
> -              }
> +                }
> -            }
> +              }
> -          }
> +            }
> -          for (var i = 0; i < array.length; i++)
> +          }
> +          window.requestAnimationFrame(() => {
> +            for (let i = 0; i < array.length; i++) {

nit: Touching this line seems like a good opportunity to switch this to for..of

Comment 9

a year ago
mozreview-review-reply
Comment on attachment 8957358 [details]
Bug 1356765 - Avoid flushing layout when showing menus with accel shortcuts.

https://reviewboard.mozilla.org/r/226284/#review232342

> I doubt getBoundsWithoutFlushing returns a width.

Yeah, holy smokes - this'll teach me to cobble together a quick patch just before going home. :/
So having thought about this more, I think florian has found a pretty fatal weakness in my patch here - in the event that an event handler injects a menuitem during the popupshowing event, it'll have no dimensions calculated for it unless I flush, which means I have to wait for it to appear. Which means that there's chance of flicker if I want to get the widths of all accel labels and then adjust them.

I think what'd be best is if we could somehow just get layout to calculate all of this stuff for us rather than doing it in JS. I wonder if the new CSS grid stuff can be used in here.
(In reply to Mike Conley (:mconley) (:⚙️) (Totally backlogged on reviews and needinfos) from comment #10)
> So having thought about this more, I think florian has found a pretty fatal
> weakness in my patch here - in the event that an event handler injects a
> menuitem during the popupshowing event, it'll have no dimensions calculated
> for it

Do we have a way to detect this edge case? Eg. would getBoundsWithoutFlushing return a 0x0 rect that could be used as a hint that we need a flush?

I wouldn't want the rare edge case to stand in the way of a significant perf win for the usual case :-).
I'm throwing this back into the pond. Might pick it up again down the road.
Whiteboard: [ohnoreflow][qf:f61][qf:p1][fxperf:p1] → [ohnoreflow][qf:f61][qf:p1][fxperf:p3]
Assignee: mconley → nobody
Whiteboard: [ohnoreflow][qf:f61][qf:p1][fxperf:p3] → [ohnoreflow][qf:f64][qf:p3][fxperf:p3]

Updated

a year ago
Whiteboard: [ohnoreflow][qf:f64][qf:p3][fxperf:p3] → [ohnoreflow][qf:p3:f64][fxperf:p3]

Updated

7 months ago
Whiteboard: [ohnoreflow][qf:p3:f64][fxperf:p3] → [ohnoreflow][qf:p3][fxperf:p3]
You need to log in before you can comment on or make changes to this bug.