Closed Bug 753139 Opened 12 years ago Closed 4 years ago

tab close/open animation should finish in a reasonable amount of time

Categories

(Firefox :: Tabbed Browser, defect)

Product:
Firefox
Component:
Tabbed Browser
Platform:
x86_64
Windows 7
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

()

Status:
RESOLVED INCOMPLETE

People

(Reporter: taras.mozilla, Unassigned)

References

Details

(Whiteboard: [Snappy:p1]) 

Most open/closes seem to take around 100ms, but there is a significant portion of 400+s ones(according to telemetry). On one of my machines tab animation consistently takes ~400ms. We should adjust our animation such that it finishes in a reasonable amount of time even on slower machines.
(In reply to Taras Glek (:taras) from comment #0)
> Most open/closes seem to take around 100ms, but there is a significant
> portion of 400+s ones(according to telemetry).

400+ seconds? or 400+ milliseconds?
(In reply to Jared Wein [:jaws] from comment #1)
> (In reply to Taras Glek (:taras) from comment #0)
> > Most open/closes seem to take around 100ms, but there is a significant
> > portion of 400+s ones(according to telemetry).
> 
> 400+ seconds? or 400+ milliseconds?

ms
CSS transitions are defined in 
http://mxr.mozilla.org/mozilla-central/source/browser/base/content/browser.css#36
Code to measure their duration is in http://dxr.lanedo.com/mozilla-central/browser/base/content/tabbrowser.xml.html#l3294

Jet, why do css transitions that are defined to take a 200-250ms(i'm not clear on how css transitions are declared) range from 3ms to 3seconds?
Whiteboard: [Snappy] → [Snappy:p1]
Depends on: 731974
Is there a good testcase for this, or just using the telemetry data?
(In reply to Vladimir Vukicevic [:vlad] [:vladv] from comment #4)
> Is there a good testcase for this, or just using the telemetry data?

1. open a browser with a bunch of about:blank tabs and ctrl+w them. 
2. open about:telemetry, search for FX_TAB_ANIM_CLOSE_MS, observe values in there.
Hardware: I use my amd netbook and i7 mba (both running windows), they seem to average 350-500ms(depending if I'm running the profiler)

See trace from doing ctrl+w of multiple overflow tabs below.
I'm not sure if FX_TAB_ANIM_CLOSE_MS captures *just* the tab CSS transition or if it also captures xul scrollbox overhead(which is huge).
http://people.mozilla.com/~bgirard/cleopatra/?report=0f72d90c38a24d9fe443d0e6cdaff2daa661081e
Derp, I use sidebar tabs which doesn't go through our normal tabstrip code so I wasn't seeing this telemetry entry.  I'll try with a new profile and see if my code makes a difference.
(In reply to Taras Glek (:taras) from comment #5)
> (In reply to Vladimir Vukicevic [:vlad] [:vladv] from comment #4)
> > Is there a good testcase for this, or just using the telemetry data?
> 
> 1. open a browser with a bunch of about:blank tabs and ctrl+w them. 
> 2. open about:telemetry, search for FX_TAB_ANIM_CLOSE_MS, observe values in
> there.
> Hardware: I use my amd netbook and i7 mba (both running windows), they seem
> to average 350-500ms(depending if I'm running the profiler)

If you're interested in meaningful / realistic FX_TAB_ANIM_CLOSE_MS values, you might need to disable the profiler (and make sure you don't run into https://github.com/bgirard/Gecko-Profiler-Addon/issues/21).
(In reply to Dão Gottwald [:dao] from comment #7
> If you're interested in meaningful / realistic FX_TAB_ANIM_CLOSE_MS values,
> you might need to disable the profiler (and make sure you don't run into
> https://github.com/bgirard/Gecko-Profiler-Addon/issues/21).

I'm a bit surprised to hear the profiler is impacting performance so much here. Is it particular to this operation or is it in general on windows. At the very worse I'd expect it just simulates a very slightly slower machine which many users have.

Note that the fix for issue 21 has already shipped if you let the extension selfupdate.
(In reply to Benoit Girard (:BenWa) from comment #8)
> (In reply to Dão Gottwald [:dao] from comment #7
> > If you're interested in meaningful / realistic FX_TAB_ANIM_CLOSE_MS values,
> > you might need to disable the profiler (and make sure you don't run into
> > https://github.com/bgirard/Gecko-Profiler-Addon/issues/21).
> 
> I'm a bit surprised to hear the profiler is impacting performance so much
> here. Is it particular to this operation or is it in general on windows. At
> the very worse I'd expect it just simulates a very slightly slower machine
> which many users have.
> 
> Note that the fix for issue 21 has already shipped if you let the extension
> selfupdate.
It has an impact in high-res mode on my slow machine. Either way, even if things run slow that should not cause the 200ms animation to run take 2x longer.
Benoit was looking into using css animation for tab strip. Problem is that animations do not start when the css property is set. 
As I understand: animations are started by the refresh driver. So if there is a lot of jank when the animation is requested(ie event loop is busy), the animation gets scheduled way late...thus ends late too. This is bad.
(In reply to Taras Glek (:taras) from comment #10)
> Benoit was looking into using css animation for tab strip.

CSS animations or transitions? Note that the open/close animations already use CSS transitions.
I'm using transition, transform and translate.
(In reply to Dão Gottwald [:dao] from comment #11)
> (In reply to Taras Glek (:taras) from comment #10)
> > Benoit was looking into using css animation for tab strip.
> 
> CSS animations or transitions? Note that the open/close animations already
> use CSS transitions.

I meant CSS transitions. I also meant doing them in a way that does not jank.
Depends on: 790567
(In reply to Taras Glek (:taras) from comment #10)
> Benoit was looking into using css animation for tab strip. Problem is that
> animations do not start when the css property is set. 
> As I understand: animations are started by the refresh driver. So if there
> is a lot of jank when the animation is requested(ie event loop is busy), the
> animation gets scheduled way late...thus ends late too. This is bad.

Starting the animation immediately would not help a lot in those situations (and it might in fact make things seem worse), since in order to render the second frame we still need to get to the refresh driver.  Well, that's true unless we use OMTC on the desktop, where you would still see a lag if the event loop is busy since the click message needs to be processed first. :(

Is this only a problem because animations start too late?  Or do they actually take longer to finish too with a busy event loop?
So, this got me curious, and I decided to profile this.  Here is a link to a profile which shows what happens if you hold Cmd+T to open tabs one after another.

http://people.mozilla.com/~bgirard/cleopatra/?report=bf5d5e321d9f6db3ad1f0ba435196e1e216c34f1

There's a bunch of bad things that we do:

* First and foremost, we seem to reflow on multiple occasions, which is bad if you expect anything to happen fast!  I filed bug 792296 on what I can say is my pet peeve.  Also filed bug 792297 which is outstanding in itself!
* Things like tabbrowser.xml's updateCurrentBrowser dispatch an event <http://mxr.mozilla.org/mozilla-central/source/browser/base/content/tabbrowser.xml#956> which seems expensive (partly because the addon-sdk code associated with the profiler add-on seems to handle that which is odd!).  Dispatching an event is rather odd since as far as I can tell, the thing which is supposed to get called here is onxblTabSelect in tabbrowser.xml, so perhaps we could just call that method directly?
* newTab.js's init code takes ~8% of the time, but I assume that's going to be fixed when we lazy load about:newtab?
* The stuff happening under tabbrowser's addTab() is particular expensive.

I should also say that I did not look very closely at this profile.  One could potentially dig in much deeper and find yet worse cases.
(In reply to Ehsan Akhgari [:ehsan] from comment #14)
> 
> Starting the animation immediately would not help a lot in those situations
> (and it might in fact make things seem worse), since in order to render the
> second frame we still need to get to the refresh driver.  Well, that's true
> unless we use OMTC on the desktop, where you would still see a lag if the
> event loop is busy since the click message needs to be processed first. :(
The goal here is to not make things smoother. The goal here is to have ui operations complete in a finite-ish amount of time.

> 
> Is this only a problem because animations start too late?  Or do they
> actually take longer to finish too with a busy event loop?

From what I see animation scheduling is fairly precise, bug 731974 should make that better yet.


To illustrate the problem in this bug: <close tab js><request anim><jank><anim start..(maybe hand off to omtc)> will delay the close of the animation by |<jank>|.

We are supposed to drop frames if we can't draw fast enough.
I see.  OK, if we're not talking about smoothness, then what you're saying makes sense.  Although I think there's much more gain to be had by actually optimizing the process of opening and closing tabs than just put things sooner on the event loop, perhaps by extending on my analysis in comment 15.
(In reply to Ehsan Akhgari [:ehsan] from comment #17)
> I see.  OK, if we're not talking about smoothness, then what you're saying
> makes sense.  Although I think there's much more gain to be had by actually
> optimizing the process of opening and closing tabs than just put things
> sooner on the event loop, perhaps by extending on my analysis in comment 15.

Ehsan we are fixing that in parallel(tracked by bug 789573). Jeff & Bas have done a lot of analysis, glad to have you help too.

I'm still waiting to hear a concrete plan on how to address this animation issue from a gfx/layout person.
David knows about animations.
I filed bug 792922 to track the performance of tab opening.
(In reply to Taras Glek (:taras) from comment #10)
> As I understand: animations are started by the refresh driver. So if there
> is a lot of jank when the animation is requested(ie event loop is busy), the
> animation gets scheduled way late...thus ends late too. This is bad.

So I think fundamentally what we want to do here is have a way of tying the animation/transition start time to the timeStamp in a DOM event (i.e., the mouse or key event that initiated the operation).

This requires that:
 * DOM events carry time stamps that are in useful interval time (which our implementation does, but doesn't match the spec; see http://lists.w3.org/Archives/Public/www-dom/2012JulSep/0098.html
 * we have an API for setting the start time of a transition or animation to something that comes from an event

The way animations and transitions are currently defined, I don't see a good way to do this.  However, it would probably be more straightforward if we had the API proposed in bug 774655.

Brian Birtles may also have ideas about APIs here.
(In reply to David Baron [:dbaron] from comment #21)
> Brian Birtles may also have ideas about APIs here.

The Web Animations API should cover this (I say should because I'm not 100% sure of the integration with CSS Transitions since Shane Stephens is working on that and he's on paternity leave).

SMIL also recommends using the event timestamp as the start of the interval for event-based timing, and in discussion with Adobe this sort of issue (exact synchronisation of events and animations) came up a lot and we're suggesting using the event timestamp for that. We'll possibly add a constructor to Web Animations that does this automatically (takes an event and uses its timestamp for the start time, converting it as necessary).

We're currently working on firming up the timing model for Web Animations. I hope implementation will start in a couple of months' time. If that's too late for this bug perhaps we can add an internal API such as David suggested until then.
Is there a reason to not set the animation start time to the time the css animation is setup? Brian, we need something asap.
David and Brian, i see the following options presented in your analyses
1) bug 774655 suggested by David
2) some web animation api suggested by Brian

Since we would like to fix this UI problem soon, 1) seems like the best way forward. Who could work on this?

ps. 
I'm not clear why we can't tweak the definition of when the animation starts. Does the spec explicitly say that animation start should occur on the next browser paint? If not, what limitation prevents this tweak? I'm not clear on why DOM event timestamp non-conformance makes a difference here.
(In reply to Taras Glek (:taras) from comment #24)
> David and Brian, i see the following options presented in your analyses
> 1) bug 774655 suggested by David
> 2) some web animation api suggested by Brian
> 
> Since we would like to fix this UI problem soon, 1) seems like the best way
> forward. Who could work on this?

Yes, the Web Animations stuff is much longer term. I think (1) is the way to go so long as it remains an internal API (which we can replace with (2) in the future).

> ps. 
> I'm not clear why we can't tweak the definition of when the animation
> starts. Does the spec explicitly say that animation start should occur on
> the next browser paint? If not, what limitation prevents this tweak? I'm not
> clear on why DOM event timestamp non-conformance makes a difference here.

Putting a monotonically-increasing timestamp in the event allows you to compensate for the delay between event dispatch (i.e. when the click actually occurred) and event handling (when you actually triggered the animation).
(In reply to Brian Birtles (:birtles) from comment #25)
> (In reply to Taras Glek (:taras) from comment #24)
> > David and Brian, i see the following options presented in your analyses
> > 1) bug 774655 suggested by David
> > 2) some web animation api suggested by Brian
> > 
> > Since we would like to fix this UI problem soon, 1) seems like the best way
> > forward. Who could work on this?
> 
> Yes, the Web Animations stuff is much longer term. I think (1) is the way to
> go so long as it remains an internal API (which we can replace with (2) in
> the future).
> 
> > ps. 
> > I'm not clear why we can't tweak the definition of when the animation
> > starts. Does the spec explicitly say that animation start should occur on
> > the next browser paint? If not, what limitation prevents this tweak? I'm not
> > clear on why DOM event timestamp non-conformance makes a difference here.
> 
> Putting a monotonically-increasing timestamp in the event allows you to
> compensate for the delay between event dispatch (i.e. when the click
> actually occurred) and event handling (when you actually triggered the
> animation).

So is this a realistic approach we should consider ie #3? Personally I would prefer to fix it this way so websites with similar transition-dependent elements to ours will benefit without having to make use of a new api.
(In reply to Taras Glek (:taras) from comment #26)
> (In reply to Brian Birtles (:birtles) from comment #25)
> > Putting a monotonically-increasing timestamp in the event allows you to
> > compensate for the delay between event dispatch (i.e. when the click
> > actually occurred) and event handling (when you actually triggered the
> > animation).
> 
> So is this a realistic approach we should consider ie #3? Personally I would
> prefer to fix it this way so websites with similar transition-dependent
> elements to ours will benefit without having to make use of a new api.

Internally we'll still need the new API, and externally too we'll need one eventually (for animations triggered from events but not in a declarative fashion). That's something covered in the Web Animations work, but if I understand correctly, you're proposing we automatically invoke this new API now for animations whose beginning is declaratively associated with an event (e.g. :hover selectors). This would also include storing the monotonically increasing timestamp in the event (but not in the timeStamp attribute since that's epoch time).

David, does CSS allow this kind of automatic adjustment? As I understand it, currently animations are supposed to begin when styles are resolved but is that sufficiently vague that we could automatically compensate for these delays without breaking content?
So I think if we had a way to do it it would still be conformant to CSS.

However, I don't think it's realistic.  We do a lot of batching of restyles in ways that means we no longer have a way to track which style changes resulted from which changes.  These batching optimizations are pretty critical to our performance in many cases.  So I think triggering animations based on event times is only something that can happen from cases where the animations are explicitly associated with the event rather than having the connection be a style change.
Flags: needinfo?(dbaron)
Blocks: 815354
Status: NEW → RESOLVED
Closed: 4 years ago
Resolution: --- → INCOMPLETE
You need to log in before you can comment on or make changes to this bug.