1531520 - Make sessionrestore talos tests measure the delta to process creation time, and not to sessionRestoreInit

Status: RESOLVED FIXED

(Testing :: Talos, defect)

Description

[:Felipe Gomes (needinfo for replies!)]

The sessionrestore talos tests currently measure the delta of time between sessionRestoreInit and sessionRestored.

This is not a really good metric because changes in the internal startup order that cause sessionRestoreInit to shift might be reported as regressions, even though sessionRestored might not have shifted or might even haven improved!

What matters to the user is the time from process launch until the time that the session is fully restored. So let's measure this.

This problem has tripped several people in the past. See bug 1491997 and bug 827976 as examples.

Comment 1

[:Felipe Gomes (needinfo for replies!)]

The actual change to do here is very simple [1], but there are two questions:

1. there are a few reasonable choices to choose for the beginning of the delta:

• process creation
• start (XPCOM start)
• main (XRE_mainInit)
• selectProfile
• afterProfileLocked

Originally we had talked about using process creation, but I don't know if this will make this test also sensitive to things that we don't want to be measuring (OS-level startup stuff, for example). On the other hand, anything other than process creation will end up having red herrings due to other ordering changes (which is what this bug is meant to prevent).

2. The sessionRestoreInit mark is only used by this test, and nothing else (at least in the codebase). Should we remove it, or is all of StartupTimeline reported to telemetry and someone might be interested in keeping this data around?

The marker is recorded at BrowserGlue::_onBeforeUIStartup -> SessionStartup.init(), so in a way it's a good measurement of when _onBeforeUIStartup begins.

[1] https://searchfox.org/mozilla-central/source/testing/talos/talos/startup_test/sessionrestore/addon/api.js#106

Comment 2

[Florian Quèze [:florian]]

(In reply to :Felipe Gomes (needinfo me!) from comment #1)

1. there are a few reasonable choices to choose for the beginning of the delta:

• process creation
• start (XPCOM start)
• main (XRE_mainInit)
• selectProfile
• afterProfileLocked

Originally we had talked about using process creation, but I don't know if this will make this test also sensitive to things that we don't want to be measuring (OS-level startup stuff, for example). On the other hand, anything other than process creation will end up having red herrings due to other ordering changes (which is what this bug is meant to prevent).

I would say process creation. Or if we are concerned that the time it takes us to get the profile lock (because the previous shutdown is taking a while) would have an impact, then afterProfileLocked would be a good choice. I don't think shutdown is an issue when running Talos though.

2. The sessionRestoreInit mark is only used by this test, and nothing else (at least in the codebase). Should we remove it, or is all of StartupTimeline reported to telemetry and someone might be interested in keeping this data around?

It's reported to telemetry, yes. I think we should keep it. Or removing it should be a separate discussion in its own bug. But I see value in keeping it, because the sooner we start session restore, the long the off main thread I/O has to actually read the file, and hopefully have it ready as soon as we have the first browser window displayed.

Comment 3

[Mike Conley (:mconley) (:⚙️)]

The fewer wild-goose chases we can create like the one you just went through, probably the better. I'm for process start, myself (I agree though that if this introduces noise due to waiting on the disk for the last shutdown to release the profile, afterProfileLocked is a smart choice).

Comment 4

[:Felipe Gomes (needinfo for replies!)]

Ok, let's try going with process creation for a start, and then if we see it's causing problems we can move to afterProfileLocked

Comment 5

[:Felipe Gomes (needinfo for replies!)]

Attachment: Bug 1531520 - Make sessionrestore talos tests measure the delta to process creation time, and not to sessionRestoreInit. r=mconley

Comment 6

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/0ecb667b6b04
Make sessionrestore talos tests measure the delta to process creation time, and not to sessionRestoreInit. r=mconley

Comment 7

[:Felipe Gomes (needinfo for replies!)]

I updated the test description on the wiki: https://wiki.mozilla.org/Buildbot/Talos/Tests#sessionrestore.2Fsessionrestore_no_auto_restore.2Fsessionrestore_many_windows

Comment 8

[Andrei Ciure[:aciure]]

https://hg.mozilla.org/mozilla-central/rev/0ecb667b6b04