Closed Bug 743069 Opened 12 years ago Closed 4 years ago

Make tab strip async

Categories

(Firefox :: Tabbed Browser, defect)

x86_64
Windows 7
defect
Not set
normal

Tracking

()

RESOLVED WORKSFORME

People

(Reporter: taras.mozilla, Unassigned)

References

(Blocks 1 open bug)

Details

(Whiteboard: [Snappy:P1][telemetry-needed])

Attachments

(2 files, 5 obsolete files)

When switching tabs we draw tab and content synchronously. IE updates the tab first and draws content with a slight delay(can be seen from holding CTRL+Tab where only the tab bar is updating at high speed).
This is made worse by various drawing performance issues because the tab bar update speed depends on the complexity of the tab. For example in bug 695118, clicking on the google reader tab results in a long pause before anything happens.
Patrich has some thoughts on how to make the tab bar smooth, he started a canvas-based prototype in http://pcwalton.mimiga.net/tabstrip/tabstrip.html
Attached patch hacky PoC (obsolete) — Splinter Review
This makes switching the "tabpanel" (the thing the <browser> lives in) happen from a setTimeout once the tab itself is selected (this hacky patches apply to all tabboxes, we'd probably want a solution specific to the Firefox tab bar).

The risks here are that there could be things that assume that selected tabpanels and tabs are kept in sync synchronously, and will break when they aren't. Also, there may be cases where someone can trigger a tab switch and then jank us up enough to delay the panel switch, which could be used for spoofing. Auditing for that may be a little challenging.
Comment on attachment 612768 [details] [diff] [review]
hacky PoC

This would fix the case of clicking on a tab and nothing happening for a while. Do you have ideas for async holding down ctrl+tab behavior?
Note I still get lag been click and tab switch with this patch. Maybe it work better if tabpanel switched from mozafterpaint instead of settimeout-(guessing here).
In addition to yielding before actually switching content, this also yields before firing the "select" event (which is what updates the UI in response to the tab switch, i.e. updateCurrentBrowser).
Attachment #612768 - Attachment is obsolete: true
Attached patch patch with timing info (obsolete) — Splinter Review
This is a messy patch that does a new Date() in the mouse down handler and then compares that to how long it takes to update the tab.
So from clicking around looks like 2 things happen:
setTimeout in http://mxr.mozilla.org/mozilla-central/source/toolkit/content/widgets/tabbox.xml#800 means that we can get a CC inbetween the mousedown and actually changing tabs.
The delays I'm seeing also seem to correlate to time it takes to capture tab thumbnails on my computer(i'm clicking around frequently and probably tripping on those settimeouts)
Depends on: 742594
Attached patch restructure code + timing (obsolete) — Splinter Review
Got rid of image capture, got rid of timeout. This reduced worst case delays by 10x. There is still occasional lag when clicking on a tab and nothing happens, but it doesn't show up in the timings which suggests that that's caused by the event loop being busy at the time of the click vs post-click as before(and much harder to fix).
Attachment #613408 - Attachment is obsolete: true
Whiteboard: [Snappy:P1] → [Snappy:P1][telemetry-needed]
This gets rid of settimeouts, switches tab contents from mozafterpaint(ie after tab title paints) and allows tab paints to be cancelled altogether. 

With this patch firefox tab bar starts to behave somewhat like ie. Under normal conditions content switch is rarely cancelled, but when gfx slow down tab switching is cancelled quite often. 

Our tabbar painting is still horribly slow, but it's a lot smoother with this patch.
Attachment #613424 - Attachment is obsolete: true
This is as far as I can go here. I think mozRequestAnimationFrame is the proper way to do what my mozafterpaint hackery was aiming for.

Occasionally this results in really smooth ctrl+tab-on-repeat animation behavior(ie when we don't jank due to CC, GC, session_store, etc and when intel gfx is not performing weirdly)
Attachment #613449 - Attachment is obsolete: true
This calls mozRequestAnimationFrame twice. Once to wait for the tab button to paint, and once to paint content.
This should be more equivalent to the mozAfterPaint code earlier. This makes ctrl+tab as fast as it can be without cheating, clicking on tabs that are slow to paint seems more responsive, etc. Promise this is my last patch for today.
Attachment #613456 - Attachment is obsolete: true
(In reply to Taras Glek (:taras) from comment #11)
> Test build is available at
> http://dl.dropbox.com/u/5961467/moz/firefox-14.0a1.en-US.win32.
> tab%20animation.zip

Using this build, when a new tab is opened the URL bar is not focused. Also when cycling through tabs some tabs never get selected.
Yeah, I noticed the former when my tested my hacky patch. We'd probably also need to avoid the latter if we did something like this.
I had mentioned in another bug on this (but was not directly related) -  https://bugzilla.mozilla.org/show_bug.cgi?id=500791#c19


Now, I tried it again, with running strace + switch tabs in firefox, and in strace window I see this: 

strace -p 53329 -e 'trace=madvise'          
Process 53329 attached - interrupt to quit
madvise(0x7f6653971000, 4096, MADV_DONTNEED) = 0
madvise(0x7f66538d2000, 4096, MADV_DONTNEED) = 0
madvise(0x7f6653702000, 32768, MADV_DONTNEED) = 0
madvise(0x7f664fa58000, 32768, MADV_DONTNEED) = 0
madvise(0x7f664fa4e000, 32768, MADV_DONTNEED) = 0
madvise(0x7f664ae84000, 8192, MADV_DONTNEED) = 0
madvise(0x7f664a010000, 32768, MADV_DONTNEED) = 0
madvise(0x7f6636746000, 28672, MADV_DONTNEED) = 0
madvise(0x7f6634f8b000, 65536, MADV_DONTNEED) = 0
madvise(0x7f66331cd000, 49152, MADV_DONTNEED) = 0
madvise(0x7f663231b000, 65536, MADV_DONTNEED) = 0
madvise(0x7f662a4c4000, 12288, MADV_DONTNEED) = 0
madvise(0x7f662750e000, 12288, MADV_DONTNEED) = 0
madvise(0x7f66270c1000, 49152, MADV_DONTNEED) = 0
madvise(0x7f66270ba000, 12288, MADV_DONTNEED) = 0
madvise(0x7f6627078000, 12288, MADV_DONTNEED) = 0
madvise(0x7f6621c34000, 12288, MADV_DONTNEED) = 0
madvise(0x7f6621787000, 4096, MADV_DONTNEED) = 0
madvise(0x7f661f490000, 24576, MADV_DONTNEED) = 0
madvise(0x7f661f479000, 24576, MADV_DONTNEED) = 0
madvise(0x7f661ee22000, 49152, MADV_DONTNEED) = 0
madvise(0x7f661e048000, 24576, MADV_DONTNEED) = 0
madvise(0x7f661c38c000, 16384, MADV_DONTNEED) = 0
madvise(0x7f661bf56000, 20480, MADV_DONTNEED) = 0
madvise(0x7f661bbd3000, 16384, MADV_DONTNEED) = 0
madvise(0x7f661bb24000, 28672, MADV_DONTNEED) = 0
madvise(0x7f6619cd2000, 20480, MADV_DONTNEED) = 0
madvise(0x7f6619cc3000, 40960, MADV_DONTNEED) = 0
madvise(0x7f6619c06000, 45056, MADV_DONTNEED) = 0
madvise(0x7f66197c8000, 49152, MADV_DONTNEED) = 0
madvise(0x7f66197ad000, 16384, MADV_DONTNEED) = 0
madvise(0x7f66197a4000, 16384, MADV_DONTNEED) = 0
madvise(0x7f661977b000, 86016, MADV_DONTNEED) = 0
madvise(0x7f661975c000, 20480, MADV_DONTNEED) = 0
madvise(0x7f66195ce000, 20480, MADV_DONTNEED) = 0
madvise(0x7f66195bc000, 24576, MADV_DONTNEED) = 0
madvise(0x7f661914b000, 65536, MADV_DONTNEED) = 0
madvise(0x7f661910b000, 65536, MADV_DONTNEED) = 0
madvise(0x7f6618d02000, 921600, MADV_DONTNEED) = 0
madvise(0x7f6618502000, 925696, MADV_DONTNEED) = 0

I presumed earlier this to be from some sort malloc/GC (jemalloc can do this?)

Can it be related to this?
Depends on: 750953
:jesse - what reasoning do you have for marking this one sec-review-needed? triage meeting was not able to discern a reason.
Assignee: nobody → jruderman
See comment 1.
Assignee: jruderman → nobody
No longer depends on: 750953
Whiteboard: [Snappy:P1][telemetry-needed] → [Snappy:P1][telemetry-needed][sec-assigned:jesse]
Flags: sec-review?(jruderman)
See Also: → 790567
Whiteboard: [Snappy:P1][telemetry-needed][sec-assigned:jesse] → [Snappy:P1][telemetry-needed]
Flags: sec-review?(jruderman)
No longer depends on: 742594
Blocks: 78957
No longer blocks: 593680
Blocks: tabswitch
No longer blocks: 78957
Blocks: 815354
Status: NEW → RESOLVED
Closed: 4 years ago
Resolution: --- → WORKSFORME
You need to log in before you can comment on or make changes to this bug.