Closed Bug 1331937 Opened 3 years ago Closed 2 years ago

Extend session restore Talos test

Categories

(Firefox :: Session Restore, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
Firefox 56
Tracking Status
firefox56 --- fixed

People

(Reporter: mikedeboer, Assigned: beekill)

References

(Blocks 1 open bug)

Details

Attachments

(1 file, 5 obsolete files)

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.
Attached patch ss-extend-talos.v1.patch (obsolete) — Splinter Review
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?
Attachment #8877942 - Flags: feedback?(mdeboer)
Attachment #8877942 - Flags: feedback?(dteller)
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.
Attachment #8877942 - Flags: feedback?(dteller) → feedback+
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.
Attachment #8877942 - Flags: feedback?(mdeboer) → feedback?(jmaher)
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.
Attachment #8877942 - Flags: feedback?(jmaher) → feedback+
(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.
Flags: needinfo?(mdeboer)
Flags: needinfo?(jmaher)
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.
Flags: needinfo?(jmaher) → needinfo?(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? :)
Flags: needinfo?(ryanvm) → needinfo?(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
Flags: needinfo?(chutten)
Assignee: nobody → nnn_bikiu0707
Status: NEW → ASSIGNED
Flags: needinfo?(mdeboer)
(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.
Attached patch ss-extend-talos.v2.patch (obsolete) — Splinter Review
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
Attachment #8877942 - Attachment is obsolete: true
Attachment #8881639 - Flags: review?(mdeboer)
Attachment #8881639 - Flags: review?(jmaher)
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).
Attachment #8881639 - Flags: review?(jmaher) → review-
Attached patch ss-extend-talos.v3.patch (obsolete) — Splinter Review
Added entries to talos.json
Attachment #8881639 - Attachment is obsolete: true
Attachment #8881639 - Flags: review?(mdeboer)
Attachment #8881640 - Flags: review?(mdeboer)
Attachment #8881640 - Flags: review?(jmaher)
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|
Attachment #8881640 - Flags: review?(jmaher) → review+
(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
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|
Attached patch ss-extend-talos.v4.patch (obsolete) — Splinter Review
Fixed wrong profile path. New try server: https://treeherder.mozilla.org/#/jobs?repo=try&revision=fe668ad6ef8a87fa98873182041d33616c4bda98
Attachment #8881640 - Attachment is obsolete: true
Attachment #8881640 - Flags: review?(mdeboer)
Attachment #8881760 - Flags: review?(mdeboer)
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.
Attachment #8881760 - Flags: review?(mdeboer) → review+
Attached patch ss-extend-talos.v4.patch (obsolete) — Splinter Review
Updated reviewers' name. Is this ready for check-in?
Attachment #8881760 - Attachment is obsolete: true
Flags: needinfo?(jmaher)
please land this!
Flags: needinfo?(jmaher)
Keywords: checkin-needed
Pushed by ryanvm@gmail.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/15cc8dd66362
Extend session restore Talos test with many windows. r=mikedeboer, r=jmaher
Keywords: checkin-needed
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)
Flags: needinfo?(nnn_bikiu0707)
Fixed eslint error by adding an entry to .eslintignore. :aryx, can you push this or I need to do try server first?
Attachment #8882064 - Attachment is obsolete: true
Flags: needinfo?(nnn_bikiu0707) → needinfo?(aryx.bugmail)
Pushed by archaeopteryx@coole-files.de:
https://hg.mozilla.org/integration/mozilla-inbound/rev/05bb5793e57f
Extend session restore Talos test with many windows. r=mikedeboer,jmaher
https://hg.mozilla.org/mozilla-central/rev/05bb5793e57f
Status: ASSIGNED → RESOLVED
Closed: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 56
You need to log in before you can comment on or make changes to this bug.