Open Bug 1358816 Opened 6 years ago Updated 3 years ago

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

Categories

(Firefox :: Tabbed Browser, defect, P3)

defect

Tracking

()

People

(Reporter: rjward0, Unassigned)

References

(Blocks 1 open bug)

Details

(Keywords: perf, stale-bug, Whiteboard: [ohnoreflow][fxperf:p3])

Attachments

(2 files)

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
Status: NEW → ASSIGNED
Priority: P3 → P1
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 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]
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]
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]
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]
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]

Dragging multiple tabs is uncommon, and though this was P1 it no longer has an assignee and has had no activity for some years, so marking P3 instead.

Priority: P1 → P3
You need to log in before you can comment on or make changes to this bug.