adjustTabstrip is slow, due to flushing layout

RESOLVED FIXED in Firefox 55

Status

()

Firefox
Tabbed Browser
P1
normal
RESOLVED FIXED
2 months ago
a month ago

People

(Reporter: florian, Assigned: dao)

Tracking

(Blocks: 2 bugs)

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

Firefox Tracking Flags

(firefox55 fixed)

Details

(Whiteboard: [photon-performance])

MozReview Requests

()

Submitter Diff Changes Open Issues Last Updated
Loading...
Error loading review requests:

Attachments

(1 attachment)

(Reporter)

Description

2 months ago
On a fast last generation macbook, I just had adjustTabstrip cause 37ms of jank (within 700ms of jank while detaching a mozreview tab to a new window). These 37ms are spent flushing layout.

I'm almost sure this layout flush isn't needed, because on one window I had several hundred tabs, so removing one won't change anything to what we do with close tab icons, and the new window had only one tab so we could guess easily without flushing that there was plenty of room.

Could we somehow use overflow/underflow event to replace this code?
Or would get use getBoundsWithoutFlushing to get a rought estimate, and if it's close to the threshold do an actual flush?
(Reporter)

Updated

2 months ago
See Also: → bug 1356707
(Assignee)

Updated

2 months ago
See Also: bug 1356707
(Assignee)

Updated

2 months ago
Duplicate of this bug: 1356707
(Assignee)

Updated

2 months ago
Flags: needinfo?(dao+bmo)

Updated

2 months ago
Flags: qe-verify?
Priority: -- → P2
(Assignee)

Comment 2

2 months ago
(In reply to Florian Quèze [:florian] [:flo] from comment #0)
> Could we somehow use overflow/underflow event to replace this code?

The tab clip width is intentionally larger than the tab minimum width, so we can't replace this code, but we can return early if we're overflowing.
Assignee: nobody → dao+bmo
Status: NEW → ASSIGNED
Flags: needinfo?(dao+bmo)
(Assignee)

Updated

2 months ago
Flags: qe-verify? → qe-verify-

Updated

2 months ago
Priority: P2 → P1
Comment hidden (mozreview-request)
(Reporter)

Comment 4

2 months ago
mozreview-review
Comment on attachment 8858644 [details]
Bug 1356655 - Let adjustTabstrip return early if the tab strip is overflowing.

https://reviewboard.mozilla.org/r/130612/#review133212

Thanks for looking into this! :-)

::: browser/base/content/tabbrowser.xml:5934
(Diff revision 1)
>  
>        <method name="adjustTabstrip">
>          <body><![CDATA[
> +          // If we're overflowing, tab widths don't change anymore, so we can
> +          // return early to avoid flushing layout.
> +          if (this.getAttribute("overflow") == "true") {

I started looking at the code to try to find the answer to this question, but you probably know it or can find it faster than me: will adjustTabstrip be called again after we remove the 'overflow' attribute?
I assume if we are removing it because we closed just one tab it won't matter, but if we removed plenty of tabs (eg. we triggered removeTabsToTheEndFrom from the third tab), it may change whether we need to display the close buttons.
(Assignee)

Comment 5

2 months ago
(In reply to Florian Quèze [:florian] [:flo] from comment #4)
> I started looking at the code to try to find the answer to this question,
> but you probably know it or can find it faster than me: will adjustTabstrip
> be called again after we remove the 'overflow' attribute?
> I assume if we are removing it because we closed just one tab it won't
> matter, but if we removed plenty of tabs (eg. we triggered
> removeTabsToTheEndFrom from the third tab), it may change whether we need to
> display the close buttons.

We call adjustTabstrip in _endRemoveTab.
(Reporter)

Comment 6

2 months ago
mozreview-review
Comment on attachment 8858644 [details]
Bug 1356655 - Let adjustTabstrip return early if the tab strip is overflowing.

https://reviewboard.mozilla.org/r/130612/#review133888
Attachment #8858644 - Flags: review?(florian) → review+

Comment 7

2 months ago
Pushed by dgottwald@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/7640ebe4a0e2
Let adjustTabstrip return early if the tab strip is overflowing. r=florian

Comment 8

2 months ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/7640ebe4a0e2
Status: ASSIGNED → RESOLVED
Last Resolved: 2 months ago
status-firefox55: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 55

Updated

2 months ago
Iteration: --- → 55.4 - May 1

Updated

2 months ago
See Also: → bug 1354194
(Reporter)

Updated

2 months ago
See Also: → bug 1358721
(Assignee)

Updated

2 months ago
Blocks: 1358721
See Also: bug 1358721
(Reporter)

Updated

a month ago
No longer blocks: 1348289
(Reporter)

Updated

a month ago
Blocks: 1348289
(Assignee)

Updated

a month ago
Depends on: 1366473
You need to log in before you can comment on or make changes to this bug.