1331937 - Extend session restore Talos test

Status: RESOLVED FIXED

(Firefox :: Session Restore, defect)

Description

[Mike de Boer [:mikedeboer]]

Currently we have one Talos test that opens one window and over a hundred tabs. Add a test with multiple windows and take that as the baseline.

Comment 1

[Bao Quan [:beekill]]

Attachment: ss-extend-talos.v1.patch

I created a new sessionstore file from the existed Talos test session file. The existed session file has 1 window and 89 tabs. The new file I created has 4 windows, the first window has 29 tabs, the other windows have 20 tabs.

Mike, is there anything that need to do to get this to work on try? I'm sure we need some data to adjust the number of windows and tabs.

David, since you're the creator of the session restore talos test, can you give some feedback?

Comment 2

[David Teller [:Yoric] - still alive but not very active]

Comment on attachment 8877942 [details] [diff] [review]
ss-extend-talos.v1.patch

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

lgtm, but I've been out of the loop for some time on anything Talos.

Comment 3

[Mike de Boer [:mikedeboer]]

Comment on attachment 8877942 [details] [diff] [review]
ss-extend-talos.v1.patch

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

I agree with David that this looks good - I'm redirecting my feedback request to Joel, because he knows all about Talos tests, how to best write them and how to get them running on our infra.
He'll also be (co-)reviewing your final patches here.

Comment 4

[Joel Maher ( :jmaher ) (UTC -8)]

Comment on attachment 8877942 [details] [diff] [review]
ss-extend-talos.v1.patch

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

from a talos perspective this looks good.  How does this really differ from sessionrestore and sessionrestore_no_auto_restore?  Will we ever be able to disable sessionrestore in place of the many_windows variety?  I would want to make sure we document this on the wiki: https://wiki.mozilla.org/Buildbot/Talos/Tests.  Also if there are questions about this from developers who regress this there needs to be a contact person.

Comment 5

[Bao Quan [:beekill]]

(In reply to Joel Maher ( :jmaher) from comment #4)
> Comment on attachment 8877942 [details] [diff] [review]
> ss-extend-talos.v1.patch
> 
> Review of attachment 8877942 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> How does this really differ from sessionrestore and sessionrestore_no_auto_restore?

I think this will reflect the behaviour of most power users which have many windows and many tabs each windows. But I'm don't think I have the right number of windows and number of tabs per window. It would be great to get those numbers somewhere, maybe from telemetry data?

> Will we ever be able to disable sessionrestore in place of the many_windows variety?

Mike, what do you think?

> I would want to make sure we document this on the wiki: https://wiki.mozilla.org/Buildbot/Talos/Tests.

Ok, how can I do that?

Not sure about a contact person since most of these Talos test are David's work. I just recreated the session file.

Comment 6

[Joel Maher ( :jmaher ) (UTC -8)]

I like the idea of telemetry for figuring this out, I know RyanVM has expert level skills for harvesting telemetry, maybe he can help here.

to edit the wiki, just log in and edit away :)  Likewise tell me data to enter and I can put it in the wiki.

Comment 7

[Ryan VanderMeulen [:RyanVM]]

(In reply to Bao Quan [:beekill] from comment #5)
> I think this will reflect the behaviour of most power users which have many
> windows and many tabs each windows. But I'm don't think I have the right
> number of windows and number of tabs per window. It would be great to get
> those numbers somewhere, maybe from telemetry data?

Chris, I'm thinking this is something you or someone of your ilk could help with? :)

Comment 8

[Chris H-C :chutten]

What would my herd of ruminants have to do with... Oh. _ilk_. Right.

Well, you can play around on telemetry.mozilla.org with browser.engagement.* metrics to see what "typical" numbers might be. Of particular use would be max_concurrent_{tab,window}_count[1][2].

If you want to define a "heavy user" as one that exists in the top 5% of max tab+window openers you get numbers like 73 tabs across 3 windows on Nightly 56. (see the 95th Percentile figures on the right).

Release is of course a different picture with the 95th percentile something closer to 26 tabs across those 3 windows.

So the question then becomes "which 'power user's behaviour are you interested in modelling?"

(( Feel free to also fiddle with the "compare by" if you want to see if the numbers look different on Linux than Windows ))

[1]: https://mzl.la/2sVrA7G
[2]: https://mzl.la/2sVHBKG

Comment 9

[Mike de Boer [:mikedeboer]]

(In reply to Bao Quan [:beekill] from comment #5)
> Mike, what do you think?

Not sure, time will tell I think. From a userbase perspective they both have their merit, but if the same regression is always found for both 'sessionsrestore' and 'many_windows' we should move to only have the 'many_windows' one.

> Ok, how can I do that?

You can request an account there, using a form.

> Not sure about a contact person since most of these Talos test are David's
> work. I just recreated the session file.

You can list me as the contact person.

Comment 10

[Bao Quan [:beekill]]

Attachment: ss-extend-talos.v2.patch

The telemetry data from [1] looks different from what Chris said in comment 8. Since we're restoring tabs from previous session, I decided to make the session file contains 130 tabs. The number of windows restored [2] is similar to the number in comment 8, which is 3 windows. 

So in the session file, there are 130 tabs and 3 windows. The first windows will contains around 50 tabs, 80 remaining tabs are divided equally for 2nd and 3rd window.

[1]: https://goo.gl/E96a6E
[2]: https://goo.gl/kAxZFK

Comment 11

[Joel Maher ( :jmaher ) (UTC -8)]

Comment on attachment 8881639 [details] [diff] [review]
ss-extend-talos.v2.patch

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

this is good, but please add an entry in talos.json:
https://dxr.mozilla.org/mozilla-central/source/testing/talos/talos.json?q=path%3Atalos.json&redirect_type=single

specifically I think you can add it to other-e10s (we only run e10s these days).

Comment 12

[Bao Quan [:beekill]]

Attachment: ss-extend-talos.v3.patch

Added entries to talos.json

Comment 13

[Joel Maher ( :jmaher ) (UTC -8)]

Comment on attachment 8881640 [details] [diff] [review]
ss-extend-talos.v3.patch

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

thanks, can you show me a try push with |try -b o -p linux64,macosx64,win32,win64 -u none -t other-e10s --rebuild 5|

Comment 14

[Bao Quan [:beekill]]

(In reply to Joel Maher ( :jmaher) from comment #13)
> Comment on attachment 8881640 [details] [diff] [review]
> ss-extend-talos.v3.patch
> 
> Review of attachment 8881640 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> thanks, can you show me a try push with |try -b o -p
> linux64,macosx64,win32,win64 -u none -t other-e10s --rebuild 5|

Try server: https://treeherder.mozilla.org/#/jobs?repo=try&revision=2d47ea79c5bb9fbc9d110cf3e9c0c9e1e607eb22

Comment 15

[Joel Maher ( :jmaher ) (UTC -8)]

test.py shows:
startup_test/sessionstore/profile-manywindows

and the filesystem is:
startup_test/sessionrestore/profile-manywindows

you can test this locally with |./mach talos-test -a sessionrestore_many_windows|

Comment 16

[Bao Quan [:beekill]]

Attachment: ss-extend-talos.v4.patch

Fixed wrong profile path. New try server: https://treeherder.mozilla.org/#/jobs?repo=try&revision=fe668ad6ef8a87fa98873182041d33616c4bda98

Comment 17

[Mike de Boer [:mikedeboer]]

Comment on attachment 8881760 [details] [diff] [review]
ss-extend-talos.v4.patch

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

Cool stuff, Quan! Thanks for working on this! :)

General nit: please add the reviewers in the commit message.

Comment 18

[Bao Quan [:beekill]]

Attachment: ss-extend-talos.v4.patch

Updated reviewers' name. Is this ready for check-in?

Comment 19

[Bao Quan [:beekill]]

Updated wiki page: https://wiki.mozilla.org/Buildbot/Talos/Tests#sessionrestore.2Fsessionrestore_no_auto_restore.2Fsessionrestore_many_windows

Comment 20

[Joel Maher ( :jmaher ) (UTC -8)]

please land this!

Comment 21

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/15cc8dd66362
Extend session restore Talos test with many windows. r=mikedeboer, r=jmaher

Comment 22

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

Backed out for eslint failure at testing/talos/talos/startup_test/sessionrestore/profile-manywindows/sessionstore.js:2: Unexpected token ':':

https://hg.mozilla.org/integration/mozilla-inbound/rev/ab3889356fa2a4f19366d6f548d55536262106a4

Push with failure: https://treeherder.mozilla.org/#/jobs?repo=mozilla-inbound&revision=15cc8dd663627ec12ecd4be11a69f0caad8e4e11&filter-resultStatus=testfailed&filter-resultStatus=busted&filter-resultStatus=exception&filter-resultStatus=retry&filter-resultStatus=usercancel&filter-resultStatus=runnable
Failure log: https://treeherder.mozilla.org/logviewer.html#?job_id=110845806&repo=mozilla-inbound

>  TEST-UNEXPECTED-ERROR | /home/worker/checkouts/gecko/testing/talos/talos/startup_test/sessionrestore/profile-manywindows/sessionstore.js:2:12 | Parsing error: Unexpected token : (eslint)

Comment 23

[Bao Quan [:beekill]]

Attachment: ss-extend-talos.v5.patch

Fixed eslint error by adding an entry to .eslintignore. :aryx, can you push this or I need to do try server first?

Comment 24

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/05bb5793e57f
Extend session restore Talos test with many windows. r=mikedeboer,jmaher

Comment 25

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

Thank you for the fix, pushed https://hg.mozilla.org/integration/mozilla-inbound/rev/05bb5793e57f2db7fc79bc88b9e994774940fdf4

Comment 26

[Phil Ringnalda (:philor)]

https://hg.mozilla.org/mozilla-central/rev/05bb5793e57f