Open Bug 1588165 Opened 2 months ago Updated 18 days ago

Investigate surprising ~80% improvement on sessionrestore_many_windows Talos test on Windows shippable with Fission enabled

Categories

(Firefox :: Session Restore, task)

task
Not set

Tracking

()

Fission Milestone M6

People

(Reporter: mconley, Assigned: mconley)

References

(Depends on 1 open bug, Blocks 1 open bug)

Details

Assignee: nobody → mconley

The good news is that I can reproduce this result locally. Starting to investigate now...

Okay, I've figured out the problem.

The way that the SessionRestore Talos tests work is by restoring a session, and then listening for SSTabRestored events firing in each window that gets restored. 10 seconds after the last SSTabRestored event is observed, the session is considered restored, and we record the delta between process start and the last SSTabRestored event time.

The problem is here:

https://searchfox.org/mozilla-central/rev/7536d7f480a7f18c941a590a2d4c5119d9f52770/browser/components/sessionstore/StartupPerformance.jsm#225

Wayyyyy back in bug 1261657, I made it so that StartupPerformance ignores SSTabRestored events caused by remoteness flips. See that bug for the reason why.

With Fission enabled, for each tab that's getting restored automatically, we're doing a remoteness flip because the initial remoteType of the initial tab ("web") does not match what is being loaded in the test ("webIsolated=http://localhost"). So each of the restored tabs does a remoteness flip.

And that means that we don't count any of the SSTabRestored events. Because we see no SSTabRestored events, the delta that we end up recording is between process start and the sessionstore-restoring-on-startup, so it's no wonder that Fission is reporting a better score here - none of the tab restorations are being counted!

Hey pbone, from what I can tell from the Fission meeting notes, you're working on moving us to the new process flipping mechanism, and (blessedly!) removing that responsibility from front-end / SessionStore. Do you have any WIP patches that I can look at, or planning documents or notes or something I can look at to see how that factors in with this test?

Flags: needinfo?(pbone)

I have some patches that are still in the "embarrassing" range of WIPness. It's possible I can tidy them up and show you tomorrow. The plan is to remove the onMayChangeProcess stuff from SessionStore.jsm and move it into nsDocumentChannelParent.cpp. Moving the majority of this code into C++ where we can debug it a bit easier. It was originally in SessionStore because of some historical reasons that I was probably told but don't remember. I'm not sure how this could affect the SSTabRestored notification (it'll probably still be broken when my patches land).

If you think you've got a quick-ish fix to the session store code then land it now, if it conflicts with mine I'll figure it out. My change won't land until at least the week after next since I'm going on PTO next week.

Flags: needinfo?(pbone)

We'll want to fix this for Fission milestone M6 (enable Fission in Nightly).

Fission Milestone: --- → M6

Paul, is there work remaining for this bug?

Flags: needinfo?(pbone)

Having spoken with pbone, mattwoodrow and kmag, there's still quite a bit of work remaining to get process switching out of the wheelhouse of SessionStore.

I don't think it makes sense to try to fix this until that work is done.

Flags: needinfo?(pbone)

AIUI it depends on Bug 1586985

Depends on: 1586985
You need to log in before you can comment on or make changes to this bug.