462973 - (redheadedstepchild) 50ms increase in Ts 2008-11-02 am

Status: RESOLVED FIXED

(Firefox :: Session Restore, defect, P2)

Description

[Karl Tomlinson (:karlt)]

About 50ms on Linux:

http://graphs.mozilla.org/graph.html#show=395164,395133,1431030&sel=1225585249,1225698318

The effect on Windows XP is only about 10ms:

http://graphs.mozilla.org/graph.html#show=395006,395018,395046&sel=1225627022,1225676159

Looks like it happened about about the same time as bug 395488 landed:

http://hg.mozilla.org/mozilla-central/pushloghtml?startdate=nov+2+2008+9%3A00&enddate=nov+2+2008+11%3A00

Comment 1

[Simon Bünzli]

This is probably due to the fact that we now save the session state immediately at startup (during delayedStartup to be precise) instead of 10 seconds after.

I'm not sure if there's much we can do about that - except maybe optimize for the common cause of when no session had to be restored, in which case we could delay saving the session by a few seconds (unless we fear a crash-inducing homepage).

Comment 2

[Simon Bünzli]

Attachment: only save immediately after a session restore

If my assessment is correct, this should fix the regression. Only one way to find out, though...

The change is that when not resuming a session (and not displaying about:sessionrestore), we delay saving until the first delaySaveState request, which includes an implicit delay of 2 seconds. That should get the first save out of Ts range - otherwise we can always artificially increase the minimum time until the first save up to the original 10 seconds.

Comment 3

[Dietrich Ayala (:dietrich)]

Comment on attachment 346333 [details] [diff] [review]
only save immediately after a session restore

s/succeed/execute/

r=me otherwise

Comment 4

[Simon Bünzli]

Attachment: nit fixed (for check-in)

Comment 5

[Dietrich Ayala (:dietrich)]

http://hg.mozilla.org/mozilla-central/rev/4df50933e7cb

Comment 6

[Simon Bünzli]

Looks like this indeed fixed the regression (if I'm reading the graph correctly):

http://graphs.mozilla.org/graph.html#show=395164,395133,1431030&sel=1225832254,1226089881

http://hg.mozilla.org/mozilla-central/pushloghtml?startdate=nov+5+2008+13%3A00&enddate=nov+5+2008+16%3A30

Comment 7

[Karl Tomlinson (:karlt)]

Yes, looks like it may have more than made up for the regression.

Comment 8

[Dave Townsend [:mossop]]

I've backed this out as it caused a clear 130ms Ts regression on OSX.

Comment 9

[Simon Bünzli]

Hm, could this have anything to do with bug 459722?

Otherwise such a clear Ts regression should be theoretically impossible as with the patch we do significantly _less_ than without it.

Comment 10

[Mike Beltzner [:beltzner, not reading bugmail]]

Dietrich: you've got this marked as a beta 2 blocker, and we have a cause in our sights. Do we know what caused the 130ms Ts regression on OSX? How can we make progress on this bug?

Comment 11

[Dietrich Ayala (:dietrich)]

(In reply to comment #10)
> Dietrich: you've got this marked as a beta 2 blocker, and we have a cause in
> our sights. Do we know what caused the 130ms Ts regression on OSX? How can we
> make progress on this bug?

The Ts numbers are fantastically conflicting:

Here's a graph that clearly shows the Linux win and the OSX regression:

http://graphs.mozilla.org/graph.html#show=395006,395018,395046,787093,787114,787126,1431846,1431862&sel=1225832254,1226089881

And here's the same period that shows a clear WinXP win, and a clear Vista regression!

http://graphs.mozilla.org/graph.html#show=787093,787114,787126,1431846,395006,395018,395046,1431862&sel=1225832254,1226089881

The only recourse I see is to back-out bug 395488 and re-land with the patch on this bug, to see if we Talos decides to give less conflicting numbers this time.

Comment 12

[Simon Bünzli]

(In reply to comment #11)
> The only recourse I see is to back-out bug 395488 and re-land with the patch on
> this bug, to see if we Talos decides to give less conflicting numbers this
> time.

Except that the patch on this bug only applies _over_ the patch from bug 395488.

BTW: Have you been able to make any sense out of bug 459722?

Comment 13

[Dietrich Ayala (:dietrich)]

(In reply to comment #12)
> (In reply to comment #11)
> > The only recourse I see is to back-out bug 395488 and re-land with the patch on
> > this bug, to see if we Talos decides to give less conflicting numbers this
> > time.
> 
> Except that the patch on this bug only applies _over_ the patch from bug
> 395488.

sorry, i meant:

1. we should back-out bug 395488
2. once the graphs stabilize, re-land it along with the patch on this bug

> 
> BTW: Have you been able to make any sense out of bug 459722?

no, i haven't yet had time to look deeper into that.

Comment 14

[Mike Beltzner [:beltzner, not reading bugmail]]

We'll do all of this for final, but not for beta 2, since getting a quiet tree is proving to be hard enough.

Comment 15

[Mike Beltzner [:beltzner, not reading bugmail]]

--> P2 unless Dietrich objects ...

Comment 16

[Dietrich Ayala (:dietrich)]

taking. will try re-landing this soon.

Comment 17

[Dietrich Ayala (:dietrich)]

backed out bug 395488: http://hg.mozilla.org/mozilla-central/rev/6682e271edff

will look at Ts effect once we have data.

Comment 18

[Dietrich Ayala (:dietrich)]

data from backing out the patch in bug 395488:

vista and xp and linux all show a Ts decrease:
http://graphs.mozilla.org/#show=787126,787093,787114&sel=1235748829,1235806082
http://graphs.mozilla.org/#show=395006,395018,395046,912146,1431862,2699128,2701343,2698583&sel=1235768318,1235793900
http://graphs.mozilla.org/#show=395123,395133,395164,911692,1431030&sel=1235747336,1235801543

mac shows a Ts increase: http://graphs.mozilla.org/#show=794376,794405,794391&sel=1235684439,1235827429

next step is to re-land the patch in bug 395488, bundled with the patch in this bug.

Comment 19

[Dietrich Ayala (:dietrich)]

relanded bug 395488 + the patch on this bug this morning about 10am PST.

linux:
http://graphs.mozilla.org/#show=395123,395133,395164,911692&sel=1236330447,1236382480

mac 10.5:
http://graphs.mozilla.org/#show=794376,794391,794405,1431206&sel=1236343729,1236382624

win vista:
http://graphs.mozilla.org/#show=787093,787114,787126,1431846&sel=1236350365,1236382734

win xp:
http://graphs.mozilla.org/#show=395006,395018,395046,912146,1431862&sel=1236348688,1236382883

Comment 20

[Dietrich Ayala (:dietrich)]

the graphs, summarized:

linux: 3 no change (within range), 1 yo-yo

mac 10.5: 2 no change (within range), 1 yo-yo

vista: 3 no change (within range), 1 yo-yo

xp: 5 all ok within range (2 are really yo-yo-ish)


conclusion: after backing everything out, and re-landing, no noticeable trend on any platform.

resolving FIXED since the patch on this bug is likely what mitigated the loss from the original bug.

Comment 21

[Simon Bünzli]

Thanks for the investigation, Dietrich!

Comment 22

[Dão Gottwald [:dao]]

http://hg.mozilla.org/releases/mozilla-1.9.1/rev/0e90aaff293f

Comment 23

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

Did this patch not have the desired effect on 1.9.1?

http://groups.google.com/group/mozilla.dev.tree-management/browse_thread/thread/6ae9924a16c13d58#
http://groups.google.com/group/mozilla.dev.tree-management/browse_thread/thread/f7a82d63026ac90f#

Comment 24

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

Er, actually, I'm confused. Is it expected that this would cause a Mac Ts regression?

Comment 25

[Simon Bünzli]

This still isn't supposed to regress Ts: It seemed to do so on Trunk (see comment #8), but when backing out this patch and the one causing the original regression and then landing both together, we didn't get a regression at all (see comment #20). Will we have to do the same back-out-and-reland routine on the branch as well just to convice the Ts tests?

Comment 26

[Daniel Holbert [:dholbert]]

Whether it was supposed to or not, this landing seems to have coincided with a sharp increase in Ts on all of the the Mac boxes, and they've stayed at the increased level ever since.  Graphs thru today:
http://graphs.mozilla.org/#show=2417040,2417188,2418791&sel=1236277424,1236825014
http://graphs.mozilla.org/#show=2421338,2418509,2420017&sel=1236238887,1236827342

From the md.tree-management post linked in comment 23, the checkin range is:
http://hg.mozilla.org/releases/mozilla-1.9.1/pushloghtml?fromchange=e40d9b4121f4&tochange=e212796b220f

IMHO we should back this bug's changeset out ASAP.  If that doesn't fix the Ts regression, we need to back out other bugs from the checkin range, to see which of them were responsible.

Dao, can you take care of this, since you pushed 4 out of the 5 changesets in the blame range?

Comment 27

[Dietrich Ayala (:dietrich)]

I'll back this out of 1.9.1 now.

Comment 28

[Karl Tomlinson (:karlt)]

A 1.9.1 regression on Mac is expected because it is what happened on trunk.

Note that on trunk bug 395488 caused a Ts decrease on Mac, but an increase on other platforms (comment 18).  The patch here is expected to nullify the effects of bug 395488 (see comment 20).

(I haven't checked that the numbers exactly line up.  Since its so long ago now, possibly only way to really test for certain would be to perform the same dance on 1.9.1 as done on trunk.  But, as the test has already been done on trunk, I don't know that its necessary to repeat on branch.)

What is clear though is that there was something between bug 395488 and this bug that improved Mac startup.  If we want faster startup on Mac, then it would be worth filing a bug to investigate the reasons for that and come up with a patch that doesn't regress other platforms.

Comment 29

[Simon Bünzli]

I ask again: Could this have anything to do with bug 459722 (which also is about perf and Mac only)? If any Mac user could investigate...

Comment 30

[Dietrich Ayala (:dietrich)]

(In reply to comment #29)
> I ask again: Could this have anything to do with bug 459722 (which also is
> about perf and Mac only)? If any Mac user could investigate...

Sorry for lack of response there. I'll see if I can reproduce.

Comment 31

[Dietrich Ayala (:dietrich)]

backed out last night at 8:30pm pacific: http://hg.mozilla.org/releases/mozilla-1.9.1/rev/dd6c7a41575d

perf graphs:

mac 10.5: http://graphs.mozilla.org/#show=2418509,2418602,2420017,2421338&sel=1236807239,1236877765

mac 10.4: http://graphs.mozilla.org/#show=2416801,2417040,2417112,2417188,2418791&sel=1236811141,1236876723

linux: http://graphs.mozilla.org/#show=2417751,2418011,2418066,2421531,2424791&sel=1236808976,1236873170

vista: http://graphs.mozilla.org/#show=2418771,2418843,2419283,2419774&sel=1236809100,1236875029

winxp: http://graphs.mozilla.org/#show=2417583,2417617,2419204,2419313,2420850&sel=1236809059,1236874380

analysis coming up

Comment 32

[Dietrich Ayala (:dietrich)]

10.5: looks like a regression, but some boxes regressed before my checkin, some after.
10.4: kinda looks regression-y, except timing is a bit off, looks like it might be from before my checkin
linux: no change
vista: no change
winxp: looks like maybe no change. hard to tell, yo-yo numbers on all boxes there.

Comment 33

[Dietrich Ayala (:dietrich)]

re-landed the combined patch on 1.9.1. caused no perceptible Ts change on any platform, so closing this bug.

http://hg.mozilla.org/releases/mozilla-1.9.1/rev/802a2eb2ab84

10.4, no change
http://graphs.mozilla.org/#show=2416801,2417040,2417112,2417188,2418791&sel=1238329910,1238409698

10.5, no change
http://graphs.mozilla.org/#show=2418509,2418602,2420017,2421338&sel=1238328188,1238435385

Vista, no change
http://graphs.mozilla.org/#show=2418771,2418843,2419283,2419774&sel=1238313031,1238421445

WinXP, no change
http://graphs.mozilla.org/#show=2417583,2417617,2419204,2419313,2420850&sel=1238347184,1238427581

Linux, no change
http://graphs.mozilla.org/#show=2417751,2418011,2418066,2421531,2424791&sel=1238349695,1238425829