Closed Bug 462973 (redheadedstepchild) Opened 11 years ago Closed 11 years ago

50ms increase in Ts 2008-11-02 am

Categories

(Firefox :: Session Restore, defect, P2, major)

x86
Linux
defect

Tracking

()

RESOLVED FIXED
Firefox 3.6a1

People

(Reporter: karlt, Assigned: dietrich)

References

Details

(Keywords: fixed1.9.1, perf)

Attachments

(1 file, 1 obsolete file)

Blocks: 395488
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).
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.
Attachment #346333 - Flags: review?(dietrich)
Comment on attachment 346333 [details] [diff] [review]
only save immediately after a session restore

s/succeed/execute/

r=me otherwise
Attachment #346333 - Flags: review?(dietrich) → review+
Assignee: nobody → zeniko
Attachment #346333 - Attachment is obsolete: true
Keywords: checkin-needed
Flags: blocking-firefox3.1+
http://hg.mozilla.org/mozilla-central/rev/4df50933e7cb
Severity: normal → major
Status: NEW → RESOLVED
Closed: 11 years ago
Keywords: checkin-needed
Priority: -- → P1
Resolution: --- → FIXED
Target Milestone: --- → Firefox 3.1b2
Yes, looks like it may have more than made up for the regression.
Status: RESOLVED → VERIFIED
I've backed this out as it caused a clear 130ms Ts regression on OSX.
Status: VERIFIED → REOPENED
Resolution: FIXED → ---
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.
Whiteboard: [needs investigation]
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?
(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.
(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?
(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.
We'll do all of this for final, but not for beta 2, since getting a quiet tree is proving to be hard enough.
Target Milestone: Firefox 3.1b2 → Firefox 3.1
--> P2 unless Dietrich objects ...
Priority: P1 → P2
taking. will try re-landing this soon.
Assignee: zeniko → dietrich
Whiteboard: [needs investigation] → [dietrich investigating]
backed out bug 395488: http://hg.mozilla.org/mozilla-central/rev/6682e271edff

will look at Ts effect once we have data.
Whiteboard: [dietrich investigating] → [needs investigation]
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.
Status: REOPENED → RESOLVED
Closed: 11 years ago11 years ago
Resolution: --- → FIXED
Whiteboard: [needs investigation]
Thanks for the investigation, Dietrich!
Keywords: checkin-needed
Whiteboard: [needs 1.9.1 branch landing]
Er, actually, I'm confused. Is it expected that this would cause a Mac Ts regression?
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?
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?
I'll back this out of 1.9.1 now.
Target Milestone: Firefox 3.1 → Firefox 3.2a1
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.
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...
(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.
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.
Alias: redheadedstepchild
Keywords: fixed1.9.1
You need to log in before you can comment on or make changes to this bug.