0.92ms uninterruptible reflow at _animateTabMove@chrome://browser/content/tabbrowser.xml:6165:26

NEW
Unassigned

Status

()

defect
P1
normal
2 years ago
4 months ago

People

(Reporter: rjward0, Unassigned)

Tracking

(Blocks 1 bug, {perf, stale-bug})

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

Firefox Tracking Flags

(Not tracked)

Details

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

Attachments

(2 attachments)

Reporter

Description

2 years ago
Here's the stack:

_animateTabMove@chrome://browser/content/tabbrowser.xml:6165:26
onxbldragover@chrome://browser/content/tabbrowser.xml:6773:11

Perhaps related to bug 1358721 ?
Component: Untriaged → Tabbed Browser
Flags: qe-verify?
Priority: -- → P2
It's:
          let tabWidth = draggedTab.getBoundingClientRect().width;
at http://searchfox.org/mozilla-central/rev/f225dbcb15ca2e38f7d434a9278a41d2340e7cf3/browser/base/content/tabbrowser.xml#6164

While the tab is being dragged, I think we can be confident that it has already been displayed and so that its size has already been computed, so I think this should be save to convert to getBoundsWithoutFlushing.

But there are a few access to boxObject a few lines later, and it's less obvious if these can be changed to not trigger a flush.
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]
Assignee: nobody → kmaglione+bmo
Depends on: 1383367
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Status: NEW → ASSIGNED
Priority: P3 → P1

Comment 4

2 years ago
mozreview-review
Comment on attachment 8889099 [details]
Bug 1358816: Part 1 - Avoid layout flushes during tab move animations.

https://reviewboard.mozilla.org/r/160122/#review165664

Thanks! Looks good from code inspection, and seems to work well. I couldn't get ohnoreflow to work on a current trunk build, and my computer is too fast to show any lag here, so I couldn't reproduce the problem to verify that it is fixed. What affects my perception of this animation the most is that it stops as soon as my cursor moves slightly out of the tab bar (bug 450915).
Attachment #8889099 - Flags: review?(florian) → review+

Comment 5

2 years ago
mozreview-review
Comment on attachment 8889100 [details]
Bug 1358816: Part 2 - Use client rather than screen coordinates to calculate drag positions.

https://reviewboard.mozilla.org/r/160124/#review165658

::: browser/base/content/tabbrowser.xml:7033
(Diff revision 1)
>          // _dragData.offsetX/Y give the coordinates that the mouse should be
>          // positioned relative to the corner of the new window created upon
>          // dragend such that the mouse appears to have the same position
>          // relative to the corner of the dragged tab.
>          function clientX(ele) {
>            return ele.getBoundingClientRect().left;

Isn't this (potentially) causing yet another sync reflow whenever we start dragging a tab? Can it be trivially fix using getBoundsWithoutFlushing?

Slightly off topic for this bug, so this can be a follow-up.
Attachment #8889100 - Flags: review?(florian) → review+
I don't want to require this to land the fixes, but here are more suggestions for reflow tests to add in the future:
- dragging a tab within the same window
- tearing off a tab; both from the "Move to New Window" context menu item, and from a drag of a tab outside the tab bar.
This is an assigned P1 bug without activity in two weeks. 

If you intend to continue working on this bug for the current release/iteration/sprint, remove the 'stale-bug' keyword.

Otherwise we'll reset the priority of the bug back to '--' on Monday, August 28th.
Keywords: stale-bug
Keywords: perf
I can still reproduce this when dragging tabs.
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

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

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]
Whiteboard: [ohnoreflow][qf:f61][qf:p1][reserve-photon-performance] [fxperf] → [ohnoreflow][qf:f61][qf:p1][fxperf]

Updated

a year ago
Whiteboard: [ohnoreflow][qf:f61][qf:p1][fxperf] → [ohnoreflow][qf:f61][qf:p1][fxperf:p2]
Whiteboard: [ohnoreflow][qf:f61][qf:p1][fxperf:p2] → [ohnoreflow][qf:f64][qf:p1][fxperf:p2]

Updated

a year ago
Whiteboard: [ohnoreflow][qf:f64][qf:p1][fxperf:p2] → [ohnoreflow][qf:p1:f64][fxperf:p2]
I'm reasonably certain that kmag is busy with Fission stuff and is unlikely to work on it, so I'm going to unassign. kmag, if I'm wrong, please let me know! :)
Assignee: kmaglione+bmo → nobody
Status: ASSIGNED → NEW
Hey florian, do you have any cycles to look at this?
Flags: needinfo?(florian)
I'll try to take a look at this now.
Assignee: nobody → mconley
Flags: needinfo?(florian)
Whiteboard: [ohnoreflow][qf:p1:f64][fxperf:p2] → [ohnoreflow][qf:p1:f64][fxperf:p1]
Assignee: mconley → felipc
STR:

 - Have an overflowed tab bar
 - Select two tabs that are not next to each other
 - Start dragging them

The code that groups the two tabs at the beginning of the dragging will invalidate the layout and then _animateTabMove will suffer a synchronous reflow
This is harder to trigger, and also a little more challenging to fix. I'm going to kick this out to qf:p1:f67. We're trading it in for bug 1499883, which is much, much more likely for users to actually hit - and it's also apparently easier to fix.
Whiteboard: [ohnoreflow][qf:p1:f64][fxperf:p1] → [ohnoreflow][qf:p1:f67][fxperf:p1]
Assignee: felipc → nobody
Whiteboard: [ohnoreflow][qf:p1:f67][fxperf:p1] → [ohnoreflow][qf:p1:f67][fxperf:p2]
Whiteboard: [ohnoreflow][qf:p1:f67][fxperf:p2] → [ohnoreflow][qf:p1:f67][fxperf:p3]
Whiteboard: [ohnoreflow][qf:p1:f67][fxperf:p3] → [ohnoreflow][fxperf:p3]
You need to log in before you can comment on or make changes to this bug.