850163 - TabOpen animation regressed with the landing of bug 844466

Status: RESOLVED FIXED

(Firefox :: Tabbed Browser, defect)

Description

[Tim Taubert [:ttaubert] (inactive)]

Starting the tabopen animation synchronously regressed the previously achieved number of frames. This is not noticable on my Win 7 quad core machine but halves the number of frames on my slow Win 8 netbook.

Before: 15 frames to render the tabopen animation
After: 7 frames

With bug 844466 backed out we're back to 15 frames. I suggest we back out the patch from bug 844466 and find another solution to this problem.

Comment 1

[Tim Taubert [:ttaubert] (inactive)]

Attachment: part 1 - back out changeset d27445d1eac5 (bug 844466)

Comment 2

[Tim Taubert [:ttaubert] (inactive)]

Attachment: part 2 - use thread.dispatch() instead of setTimeout() to start the tabopen animation

It looks like we don't process the event queue while showing a modal dialog so timeouts will not be called. Thread.dispatch() seems to work.

Comment 3

[Tim Taubert [:ttaubert] (inactive)]

Pushed to try:

https://tbpl.mozilla.org/?tree=Try&rev=9f523d1e3dad

Comment 4

[Tim Taubert [:ttaubert] (inactive)]

I tried those patches on my netbook and they still yield 15 frames for the tabopen animation.

Comment 5

[Dão Gottwald [:dao]]

Comment on attachment 723887 [details] [diff] [review]
part 1 - back out changeset d27445d1eac5 (bug 844466)

Starting the animation later to make it smoother contradicts the other goal that the animation should finish in the expected amount of time. Note that before bug 844466, we weren't measuring this initial delay since _handleTabTelemetryStart was called too late.

Comment 6

[Tim Taubert [:ttaubert] (inactive)]

Before bug 844466 we started the animation *and* the measurement off a timeout. Both of it is now done synchronously and we should expect the same results. I still see the 15 frames with bug 844466 on my quad core machine, but only 7 on my netbook.

I understand we want to start the tabopen animation itself as early as possible but not if we regress the animation frame rate by doing so. We can either back out and push an easier fix that leaves us with the former async behavior and a better frame rate or keep the regression and a not so measurable win with a sync animation.

I'd rather do the first and file a bug to figure out what exactly causes the regression when starting synchronously. We can put that code back when we figured it out.

Comment 7

[Dão Gottwald [:dao]]

(In reply to Tim Taubert [:ttaubert] from comment #6)
> Before bug 844466 we started the animation *and* the measurement off a
> timeout.

Right, and that was wrong. The animation ideally starts without a delay and we should have measured the delay in order to determine how long the animation takes from the user's perspective.

> Both of it is now done synchronously and we should expect the same
> results. I still see the 15 frames with bug 844466 on my quad core machine,
> but only 7 on my netbook.

Ideally we would get the same results, but in reality the animation competes with other stuff happening on the main thread. Waiting for an arbitrary amount of time can improve the frame rate or make it worse, depending on what other asynchronously triggered tasks (especially those related to loading about:newtab) happen to take place at the same time. I don't think that's deterministic and your different results seem to support this.

> I understand we want to start the tabopen animation itself as early as
> possible but not if we regress the animation frame rate by doing so.

I don't think it's as simple as that. The animation starting and finishing quickly but with few frames can be preferable over a delayed animation with more frames.

But we don't even have a proof for the delay resulting in more frames. All we seem to have is a single data point.

Comment 8

[Dão Gottwald [:dao]]

One thing we do immediately after addTab returns is to call focusAndSelectUrlBar. So that's a task that can deterministically impair the animation. Do you get any different numbers if you skip focusAndSelectUrlBar?

Comment 9

[Tim Taubert [:ttaubert] (inactive)]

(In reply to Dão Gottwald [:dao] from comment #7)
> (In reply to Tim Taubert [:ttaubert] from comment #6)
> > Before bug 844466 we started the animation *and* the measurement off a
> > timeout.
> 
> Right, and that was wrong. The animation ideally starts without a delay and
> we should have measured the delay in order to determine how long the
> animation takes from the user's perspective.

I don't think this is wrong, it's just two different metrics. The one thing we want to measure is how smooth the tabopen animation is (i.e. its frame rate). The second thing is how long it takes to complete.

> > Both of it is now done synchronously and we should expect the same
> > results. I still see the 15 frames with bug 844466 on my quad core machine,
> > but only 7 on my netbook.
> 
> Ideally we would get the same results, but in reality the animation competes
> with other stuff happening on the main thread. Waiting for an arbitrary
> amount of time can improve the frame rate or make it worse, depending on
> what other asynchronously triggered tasks (especially those related to
> loading about:newtab) happen to take place at the same time. I don't think
> that's deterministic and your different results seem to support this.

I'm opening a tab a couple of times and report the median frame rate for 'about:blank' as the new tab page. I can reliably reproduce 15 frames without bug 844466 and 7 frames with it applied on my netbook.

> > I understand we want to start the tabopen animation itself as early as
> > possible but not if we regress the animation frame rate by doing so.
> 
> I don't think it's as simple as that. The animation starting and finishing
> quickly but with few frames can be preferable over a delayed animation with
> more frames.

There's a certain duration X we specified for the tabopen animation. Immediately after addTab() was called we have the a time frame of X where we want to paint as many frames as possible without the animation taking any more time than X (measured from when addTab was called). This includes both the initial delay and the frame rate.

> But we don't even have a proof for the delay resulting in more frames. All
> we seem to have is a single data point.

I of course don't have a proof because I don't exactly understand what's going on when not using a timeout. All I can do is reliably reproduce the frame rates I reported.

Comment 10

[Tim Taubert [:ttaubert] (inactive)]

(In reply to Dão Gottwald [:dao] from comment #8)
> One thing we do immediately after addTab returns is to call
> focusAndSelectUrlBar. So that's a task that can deterministically impair the
> animation. Do you get any different numbers if you skip focusAndSelectUrlBar?

No change here. Still down to 7 frames.

Comment 11

[Dão Gottwald [:dao]]

(In reply to Tim Taubert [:ttaubert] from comment #9)
> > > Before bug 844466 we started the animation *and* the measurement off a
> > > timeout.
> > 
> > Right, and that was wrong. The animation ideally starts without a delay and
> > we should have measured the delay in order to determine how long the
> > animation takes from the user's perspective.
> 
> I don't think this is wrong, it's just two different metrics. The one thing
> we want to measure is how smooth the tabopen animation is (i.e. its frame
> rate). The second thing is how long it takes to complete.

I was talking about the duration, i.e. the second thing.

> > Ideally we would get the same results, but in reality the animation competes
> > with other stuff happening on the main thread. Waiting for an arbitrary
> > amount of time can improve the frame rate or make it worse, depending on
> > what other asynchronously triggered tasks (especially those related to
> > loading about:newtab) happen to take place at the same time. I don't think
> > that's deterministic and your different results seem to support this.
> 
> I'm opening a tab a couple of times and report the median frame rate for
> 'about:blank' as the new tab page. I can reliably reproduce 15 frames
> without bug 844466 and 7 frames with it applied on my netbook.

I meant different results on different machines.

Also, since you tested with about:blank, what numbers do you get with about:newtab? The about:blank case is interesting and the difference should be understood, but it's not directly relevant to our users.

Comment 12

[Dão Gottwald [:dao]]

There was a similar discussion on the patch in bug 627159 that wanted to start the tab /closing/ animation off a timeout.

Comment 13

[Avi Halachmi (:avih)]

(In reply to Tim Taubert [:ttaubert] from comment #9)
> (In reply to Dão Gottwald [:dao] from comment #7)
> I'm opening a tab a couple of times and report the median frame rate for
> 'about:blank' as the new tab page. I can reliably reproduce 15 frames
> without bug 844466 and 7 frames with it applied on my netbook.

You say you measure the median frame rate, but the values you mention (15, 7) are the number of animation frames. These are different units. Are 15/7 a median/typical of number of frames per animation? Are you focusing on number of frames only? what does the average interval look like?

Also, median frame rate (or interval, which is what we actually measure) might be misleading. E.g. a sequence of 70 16 17 17 16 17 would result in ideal median frame interval, but far from ideal animation, which is why the initial automation patch reports average frame rate - to take most intervals into account.

Also worth mentioning in this regard is that the first reported frame interval is somewhat unreliable, since it's an interval to the previous frame which might have been presented even seconds ago. In a continuous open/close sequence though, we can rely on the first interval as well - of all animations except the first one in that sequence.

The average frame interval which the initial automation patch reports thus excludes the first frame interval (of all animations, not just the first one). The first reported *paint* time, however, is reliable, since typically the startRecording call is invoked before the first recorded paint starts.

(In reply to Dão Gottwald [:dao] from comment #5)
> Starting the animation later to make it smoother contradicts the other goal
> that the animation should finish in the expected amount of time. Note that
> before bug 844466, we weren't measuring this initial delay since
> _handleTabTelemetryStart was called too late.

This is indeed a contradiction, and probably the first question we should answer: Do we prefer smoother but delayed animation? or timely finish of the animation, but with some percentage of the beginning of it not actually animating?

I don't think there are absolute rights and wrongs here, as it's a subjective issue. They both feel sluggish on a slow system. Personally, I think I prefer the sluggishness of a delayed animation start over that of a choppy/non-existing animation.

Also, the case where we test with about:blank is the easiest, as the delayed start (or first frame interval) would be minimal. The harder cases (and better candidates with which to judge this preference) are "heavy" pages, such as the (current) newtab page, gmail, etc. On a slow system (which is were we're hurting the most here), such pages could easily take longer to render than the intended animation length.

Comment 14

[Tim Taubert [:ttaubert] (inactive)]

(In reply to Avi Halachmi (:avih) from comment #13)
> You say you measure the median frame rate, but the values you mention (15,
> 7) are the number of animation frames. These are different units. Are 15/7 a
> median/typical of number of frames per animation? Are you focusing on number
> of frames only? what does the average interval look like?

I ran the measurements again, multiple times, and those are the average interval and paint values I get:

with patch from bug 844466:         interval=20.3 paint=13.8
patch from bug 844466 backed out:   interval=16.4 paint=8.9
bug 844466 backed out + my patches: interval=16.9 paint=9.2

If I interpret those values correctly there is a regression caused by bug 844466, no? We have bigger intervals between frames (so a lower frame rate) and need more time to paint.

Comment 15

[Avi Halachmi (:avih)]

(In reply to Tim Taubert [:ttaubert] from comment #14)
> I ran the measurements again, multiple times, and those are the average
> interval and paint values I get:
> 
> with patch from bug 844466:         interval=20.3 paint=13.8
> patch from bug 844466 backed out:   interval=16.4 paint=8.9
> bug 844466 backed out + my patches: interval=16.9 paint=9.2
> 
> If I interpret those values correctly there is a regression caused by bug
> 844466, no? We have bigger intervals between frames (so a lower frame rate)
> and need more time to paint.

Depends on your perspective. If you look at animation smoothness, then yes, it's a regression. OTOH, we get less delay on animation start since we don't postpone it anymore.

I think it comes down to the question I posted earlier: do we want delayed but smoother animation? or stick to the timing as much as we can, which would cost us in smoothness on slower machines.

Comment 16

[Tim Taubert [:ttaubert] (inactive)]

(In reply to Avi Halachmi (:avih) from comment #15)
> (In reply to Tim Taubert [:ttaubert] from comment #14)
> > If I interpret those values correctly there is a regression caused by bug
> > 844466, no? We have bigger intervals between frames (so a lower frame rate)
> > and need more time to paint.
> 
> Depends on your perspective. If you look at animation smoothness, then yes,
> it's a regression. OTOH, we get less delay on animation start since we don't
> postpone it anymore.

I see that we have two factors here but I don't think they exclude each other. We should (theoretically) be able to achieve the same interval and paint times without delaying the animation start. Profiling the tabopen animation benchmark seems quite difficult because of the many things going on there.

Maybe the small regression comes from the about:blank docShell etc. that is created while the animation is running. This previously *may* have happened before we started the animation. Just a wild guess.

Comment 17

[Avi Halachmi (:avih)]

Attachment: Testpatch for various anim start/end approaches

Adding summary from a discussion held at the perf work week at Paris with taras, ttaubert, myself regarding this bug:

1. taras suggested another approach which I don't recall as mentioned so far: start async, but do the full animation and finish on time - resulting in a later-starting-and-shorter but otherwise full animation (with fewer frames) which finishes on time, and hopefully doesn't have the initial animation hiccup associated with synchronous animation start.

2. We tested a patch which implemented and allowed switching between the different approaches for tab animation init:
- sync.
- async with configurable timeout for animation start (e.g. 0ms, 50ms, 100ms, etc).
- same async as above, but with deadline matching the required animation duration from the trigger event (and not from the later actual animation start).

3. We thought that sync init is always going to cause visible hiccups on slow systems, so we should probably start by reverting to async animation start (possibly with ttaubert's approach of thread dispatch to not regress this bug).

4. Late deadline as a result of late async start could be handled at a different bug using an approach similar to the one implemented at the test patch, but hopefully less hacky.


PS.
- The attached test patch can be controlled by filtering about:config with 'animTest'. No restart is required after changing prefs.
- isTarasMode sets the animation to finish at the original deadline when using late async start.
- There might be some opacity bug when the transition CSS is modified for shorter overall duration on async init (AKA 'tarasMode'), but this doesn't prevent assessing the behavior in this mode.

Comment 18

[Dão Gottwald [:dao]]

(In reply to Avi Halachmi (:avih) from comment #17)
> 3. We thought that sync init is always going to cause visible hiccups on
> slow systems,

We should get reliable data on this, with about:newtab rather than about:blank as presented earlier in this bug.

Comment 19

[Tim Taubert [:ttaubert] (inactive)]

We had a couple of conversations at the Paris work week about this. Taras, Avih, Gavin and I tried a couple of different things to mitigate the regression we're seeing. I'm not sure we can deliver "reliable" data on this as about:newtab is just too slow anyway and we'll never get more than three frames. I'm also unsure about what "reliable data" actually means here. It's a very visible regression on our target performance platform and we can of course waste even more time waiting and trying or just back out and replace the original with a simpler fix.

We came to the conclusion that the best way would be to not have the animation race with docShell/browser creation etc. and we should start the animation off a timeout. We'd need to implement a couple of improvements for about:newtab so that the initial loading time (including layout etc.) is as small as possible and the animation can run smoothly.

We all agreed that having a smooth animation and finishing on time are two goals we need to meet and therefore the only way to achieve both would be to start off a timeout and deduct the time until the timeout fired from the overall animation duration.

I think we can land the patches attached to this bug to get rid of the animation regression and bug 844466. Shortening the tabopen animation can be done in a follow-up. Improving loading times for about:newtab is already on file.

Comment 20

[Dão Gottwald [:dao]]

If when you say regression, you're still talking about about:blank, then this just isn't relevant to users. "about:newtab is just too slow anyway and we'll never get more than three frames" makes it sound like your patch isn't going to provide real-world benefits despite potentially delaying the transition (which would then be a regression).

I also don't think I agree that there always needs to be a hickup at the start of the animation. Off-main thread composition/painting and the e10s revival should fundamentally address this. Short-term, I'd like to understand what this bug is really about. The talk about docshell and browser creation feels somewhat wishy-washy. Does this stuff happen synchronously after addTab returns? If not, how does your patch help deterministically and reliably?

Comment 21

[:Gavin Sharp [email: [e-mail]]]

(In reply to Dão Gottwald [:dao] from comment #20)
> If when you say regression, you're still talking about about:blank, then
> this just isn't relevant to users.

This doesn't follow - we have conclusive evidence that starting the animation synchronously affects the animation frame rate, which certainly affects users. That the impact is worse for about:newtab than for about:blank doesn't mean we shouldn't do anything about it.

> we'll never get more than three frames" makes it sound like your patch isn't
> going to provide real-world benefits despite potentially delaying the
> transition (which would then be a regression).

On its own it might not, but we're separately going to improve about:newtab to avoid doing anything expensive until after the animation is complete, AIUI. So together, these changes will address the regression in the common case, but this change is a prerequisite.

> I also don't think I agree that there always needs to be a hickup at the
> start of the animation. Off-main thread composition/painting and the e10s
> revival should fundamentally address this.

Those aren't viable short-term fixes, as you know, so they're not directly relevant to what we do here.

> Short-term, I'd like to
> understand what this bug is really about. The talk about docshell and
> browser creation feels somewhat wishy-washy. Does this stuff happen
> synchronously after addTab returns? If not, how does your patch help
> deterministically and reliably?

I agree that it would be good to understand what exactly is causing the jank. A useful experiment would be to always call b.stop() (and not call loadURIWithFlags) in addTab, and see what impact that has on the animation.

Comment 22

[Dão Gottwald [:dao]]

(In reply to :Gavin Sharp (use [e-mail] for email) from comment #21)
> (In reply to Dão Gottwald [:dao] from comment #20)
> > If when you say regression, you're still talking about about:blank, then
> > this just isn't relevant to users.
> 
> This doesn't follow - we have conclusive evidence that starting the
> animation synchronously affects the animation frame rate, which certainly
> affects users. That the impact is worse for about:newtab than for
> about:blank doesn't mean we shouldn't do anything about it.

As I understand it, there's some regression on one machine with about:blank (although there was some dissent as to whether the numbers were interpreted correctly) but no regression at all was demonstrated for about:newtab, so I don't see what you mean by saying that the impact would be worse for about:newtab.

Comment 23

[Avi Halachmi (:avih)]

Dao, is there a fundamental reason which was not expressed on this thread, for what appears as a general objection from you to start the animation asynchronously?

While I agree with both you and gavin that we should probably try to understand better what exactly in sync init hurts animation, it does seem reasonable to me that starting async would prevent _some_ competition between animation and tab initialization. Do you think that's an unreasonable assessment?

Comment 24

[:Gavin Sharp [email: [e-mail]]]

(In reply to Dão Gottwald [:dao] from comment #22)
> As I understand it, there's some regression on one machine with about:blank
> (although there was some dissent as to whether the numbers were interpreted
> correctly)

Where was the dissent? Comment 15 suggests we're on the same page (there is a measurable animation smoothness regression).

> but no regression at all was demonstrated for about:newtab

... because the impact of about:newtab was large enough to make measurements at this scale indiscernible. But that's a separate problem, with a separate fix in a separate bug. Doesn't really have any bearing on what we do here, AFAICT.

Comment 25

[Dão Gottwald [:dao]]

(In reply to Avi Halachmi (:avih) from comment #23)
> Dao, is there a fundamental reason which was not expressed on this thread,
> for what appears as a general objection from you to start the animation
> asynchronously?

It causes the animation to start later or, as one might say, less snappily. That not the end of the world -- it's a tradeoff we can make if the smoothness improvement is significant.

> While I agree with both you and gavin that we should probably try to
> understand better what exactly in sync init hurts animation, it does seem
> reasonable to me that starting async would prevent _some_ competition
> between animation and tab initialization. Do you think that's an
> unreasonable assessment?

This completely depends on what you mean by "tab initialization", when that takes place, how long it takes, etc.

(In reply to :Gavin Sharp (use [e-mail] for email) from comment #24)
> > As I understand it, there's some regression on one machine with about:blank
> > (although there was some dissent as to whether the numbers were interpreted
> > correctly)
> 
> Where was the dissent? Comment 15 suggests we're on the same page (there is
> a measurable animation smoothness regression).

Avi and Tim may well be on the same page by now as far as Tim's measurement for about:blank is concerned -- I've only skimmed those comments.

> > but no regression at all was demonstrated for about:newtab
> 
> ... because the impact of about:newtab was large enough to make measurements
> at this scale indiscernible. But that's a separate problem, with a separate
> fix in a separate bug. Doesn't really have any bearing on what we do here,
> AFAICT.

We're not going to replace about:newtab with about:blank, and the way we supposedly want to speed it up (swapping docshells?) seems different enough from loading about:blank that the characteristics may be different. So I would suggest that instead of poking around in the dark, we measure the real in-product behavior and if/when something makes starting the animation asynchronously a net win (taking into account the initial delay it may cause), we can make that change.

Comment 26

[:Gavin Sharp [email: [e-mail]]]

(In reply to Dão Gottwald [:dao] from comment #25)
> We're not going to replace about:newtab with about:blank, and the way we
> supposedly want to speed it up (swapping docshells?)

No, we want to avoid having it draw/load images (i.e. anything computationally expensive) before the tab animation completes.

I agree that we need to measure the combination of these patches and that patch, but I don't think it necessarily needs to block landing this one.

Comment 27

[Avi Halachmi (:avih)]

(In reply to Dão Gottwald [:dao] from comment #25)
> It causes the animation to start later or, as one might say, less snappily.
> That not the end of the world -- it's a tradeoff we can make if the
> smoothness improvement is significant.

Just to make sure we're on the same page, when using setTimeout(0, startTabAnim) (or possibly requestAnimationFrame), "late" here would be at most one frame, and possibly (probably?) none, correct?

If we don't miss frames, then the only difference would be that the animation start timestamp would be slightly later, but it would still render the first animation frame at exactly the same time as on sync start (ending a bit later as well if we don't handle the deadline truncation), but the animation might appear more complete because the first animation frame would be closer to the start timestamp on async start, then it would be on sync start (assuming, of course, that there's still some sync stuff going on before we paint the first animation frame, and if there isn't, then we're not losing anything anyway with async).

So at the worst case, we'll be late by 1 frame with async, but I _think_ we actually won't miss even a single frame, and instead just make the animation look more complete, since the first animated frame would be closer to the animation "timeline" start if we trigger it async.

Interestingly, with the test patch from comment 17, I tried various async timeout durations, and found out that even delaying the start by 100ms produces (subjectively) acceptable results (even if noticeably different than 0ms), so I'd say that 17ms for one frame late start should not count as a real perceived regression, and if it can (and it seems it can) improve animation on some cases, then between the choices of sync/async, I'd pick async.

Comment 28

[Dão Gottwald [:dao]]

Yes, I'd be concerned about missing the first frame. Of the different approaches I tried in bug 844466, one was to explicitly let a frame pass before starting the animation and that looked rather janky to me.

Comment 29

[Avi Halachmi (:avih)]

(In reply to Dão Gottwald [:dao] from comment #28)
> Yes, I'd be concerned about missing the first frame. Of the different
> approaches I tried in bug 844466, one was to explicitly let a frame pass
> before starting the animation and that looked rather janky to me.

On that case, wouldn't requestAnimationFrame return just before the next layout flush, thus not missing the first frame, but still skipping any sync stuff leftovers from tab open, and still making the first frame closer to the start timestamp, thus making it "jump" less on the first frame?

Comment 30

[Dão Gottwald [:dao]]

That's the first thing I tried in bug 844466. It caused various intermittent test failures on OS X that I didn't understand.

Comment 31

[Avi Halachmi (:avih)]

Hmm.. that's interesting. I would have expected setTimeout(0) to return not later than rAF if called from the same place at the code, but the different behavior which you observed might suggest otherwise. Maybe this is something we should look into?

Comment 32

[Dão Gottwald [:dao]]

Attachment: use mozRequestAnimationFrame

If you want to give it a try, here's a patch using mozRequestAnimationFrame. I don't think this has any downside, so it would be nice if we could get it to work.

Comment 33

[Dão Gottwald [:dao]]

Comment on attachment 730949 [details] [diff] [review]
use mozRequestAnimationFrame

https://tbpl.mozilla.org/?tree=Try&rev=21fc3272c2af

Comment 34

[Dão Gottwald [:dao]]

Comment on attachment 730949 [details] [diff] [review]
use mozRequestAnimationFrame

(In reply to Dão Gottwald [:dao] from comment #33)
> Comment on attachment 730949 [details] [diff] [review]
> use mozRequestAnimationFrame
> 
> https://tbpl.mozilla.org/?tree=Try&rev=21fc3272c2af

Hm, seems like the OS X issue resolved itself in the past few weeks.

Comment 35

[Avi Halachmi (:avih)]

(In reply to Avi Halachmi (:avih) from comment #31)
> Hmm.. that's interesting. I would have expected setTimeout(0) to return not
> later than rAF if called from the same place at the code...

For reference, rAF will indeed let the tab open sync stuff finish before firing, but it would typically fire earlier than setTimeout, and this especially holds when the event queue has many items in it to handle (as I'd imagine would be the case during tab open).

This happens because setTimeout(0) will append a (future) event at the far end of the event queue - to be typically handled after all existing events were handled, while rAF registers synchronously, and so it will fire when the next generic refresh driver interval is handled, which may be an existing event already waiting within the queue - at which case it would be served earlier than some events at the queue.

So for the cause of skipping as many "heavy" tab open operations to prevent competition for animation, setTimeout would probably skip more of those than rAF, and would cause later start and smoother animation than rAF.

However, rAF will make sure we don't miss any frames, while setTimeout can't assure that.

I would still think that setTimeout is better than rAF for our cause (smoother animation at a subjectively low cost/delay), but rAF is still better than synchronous trigger.

Comment 36

[Tim Taubert [:ttaubert] (inactive)]

Comment on attachment 730949 [details] [diff] [review]
use mozRequestAnimationFrame

Review of attachment 730949 [details] [diff] [review]:
-----------------------------------------------------------------

I see 12 frames with this patch. That's not the 15 using thread.dispatch()/setTimeout() but also not the 7 we're currently at. I discussed this with Avi again and the result is as expected: we never miss the next possible animation frame but that seems on average not enough time to process racy stuff and remove it from the event queue. I don't even know which side I'm on as delaying the animation for an undetermined amount of time just to process the event queue is not a real option either.

I think we should go with this compromise for now and see if we can come up with an even better solution later.

Comment 37

[Avi Halachmi (:avih)]

I gave this subject some more thought. While we do have "hard evidence" that using timeout instead of rAF improves tab animation of about:blank, we indeed don't have such evidence for about:newtab animation.

And since about:newtab is our main goal eventually, this discussion is somewhat premature. Let's land the patch with rAF, which gives some improvement without sacrificing nothing, and revisit this decision (or other approaches) once about:newtab renders fast enough such that we can actually measure differences in its tab open animation.

Comment 38

[Dão Gottwald [:dao]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/7b662fc4a237

Comment 39

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/7b662fc4a237