Talos has detected a Firefox performance regression from push: https://hg.mozilla.org/integration/autoland/pushloghtml?changeset=5e40f7e493966d724d36de5d25689018c4c65d6c As author of one of the patches included in that push, we need your help to address this regression. Regressions: 2% sessionrestore linux64 opt e10s 651.17 -> 663.33 You can find links to graphs and comparison views for each of the above tests at: https://treeherder.mozilla.org/perf.html#/alerts?id=7527 On the page above you can see an alert for each affected platform as well as a link to a graph showing the history of scores for this test. There is also a link to a treeherder page showing the Talos jobs in a pushlog format. To learn more about the regressing test(s), please see: https://wiki.mozilla.org/Buildbot/Talos/Tests For information on reproducing and debugging the regression, either on try or locally, see: https://wiki.mozilla.org/Buildbot/Talos/Running *** Please let us know your plans within 3 business days, or the offending patch(es) will be backed out! *** Our wiki page outlines the common responses and expectations: https://wiki.mozilla.org/Buildbot/Talos/RegressionBugsHandling

:beekill I see that you authored the patches in bug 934967, can you help figure out if this is expected and what to do for resolving this?

My patch dealt with loading and saving session store files. So if any regression appears, it should be in session_restore_no_autoload. However, my guess for the regression could be: + First the compressed files are loaded, if not found then the uncompressed versions are loaded [1] + After the uncompressed session file is loaded, we initiate a read and then a write to write lz4 compress version to $Path.cleanBackup [2] [1]: https://hg.mozilla.org/integration/autoland/rev/733cfc278628#l1.129 [2]: https://hg.mozilla.org/integration/autoland/rev/733cfc278628#l2.65 These two reasons may cause the 12-second difference, though I'm not sure. But if it is the case, we can consider compress the sessionstore.js file in Talos test and see how that goes. Mike, what do you think?

:mconley, I left you a messsage on IRC but you were not there, so I'll leave my message here, in case you haven't seen it: I want to compress the session file before the Talos test start. How can I do that?

(In reply to Bao Quan [:beekill] from comment #3) > :mconley, I left you a messsage on IRC but you were not there, so I'll leave > my message here, in case you haven't seen it: > > I want to compress the session file before the Talos test start. How can I > do that? My understanding is that the test files live in m-c, so you can edit them there. Specifically, I expect you'll want to change the sessionstore files under https://dxr.mozilla.org/mozilla-central/source/testing/talos/talos/startup_test/sessionrestore .

(In reply to :Gijs from comment #4) > (In reply to Bao Quan [:beekill] from comment #3) > > :mconley, I left you a messsage on IRC but you were not there, so I'll leave > > my message here, in case you haven't seen it: > > > > I want to compress the session file before the Talos test start. How can I > > do that? > > My understanding is that the test files live in m-c, so you can edit them > there. Specifically, I expect you'll want to change the sessionstore files > under > https://dxr.mozilla.org/mozilla-central/source/testing/talos/talos/ > startup_test/sessionrestore . I modified [1] to listen for profile-do-change [2]. So when the event happens, I compress the session store file using OS.File.writeAtomic. But I'm not sure if that the event is fired before any talos tests happen. And the function writeAtomic returns a Promise, so when the Talos test begins, the compressed session file may not be there. [1]: http://searchfox.org/mozilla-central/source/testing/talos/talos/startup_test/sessionrestore/addon/SessionRestoreTalosTest.js [2]: http://searchfox.org/mozilla-central/source/toolkit/profile/notifications.txt#23

Why can't you do https://dxr.mozilla.org/mozilla-central/source/testing/talos/talos/startup_test/sessionrestore/profile/sessionstore.js --> https://dxr.mozilla.org/mozilla-central/source/testing/talos/talos/startup_test/sessionrestore/profile/sessionstore.jsonlz4 ? In other words, compress the file locally and replace the current file with the compressed version, push it to try and see if it fixes the regression. How does that sound?

I did 2 push try with 5 rebuild. Here are the results: + [1]: this push doesn't have sessionstore.jsonlz4. The results are 748, 767, 765, 752, 766. + [2]: this push contains sessionstore.jsonlz4. The results are 750, 751, 771, 757, 758. With the exception of the third run in the second push try [2] that has the longest running time (771), to me, compressing session file locally seems to fix the regression. What left is how to compress the session file before the Talos test start?

(In reply to Bao Quan [:beekill] from comment #7) > I did 2 push try with 5 rebuild. Here are the results: > + [1]: this push doesn't have sessionstore.jsonlz4. The results are 748, > 767, 765, 752, 766. > + [2]: this push contains sessionstore.jsonlz4. The results are 750, 751, > 771, 757, 758. > > With the exception of the third run in the second push try [2] that has the > longest running time (771), to me, compressing session file locally seems to > fix the regression. > > What left is how to compress the session file before the Talos test start? [1]: https://treeherder.mozilla.org/#/jobs?repo=try&revision=87d0fac4c22e5e53aa1dfca9ffa66437be9d920c [2]: https://treeherder.mozilla.org/#/jobs?repo=try&revision=9889eacee1df57fc9498c5d9a8b433c46fc43bbf

(In reply to Bao Quan [:beekill] from comment #7) > What left is how to compress the session file before the Talos test start? I'd like you to check in the sessionstore.jsonlz4, meaning you don't need to add the compression step at all. There's no need to remove the sessionstore.js.

Comment on attachment 8883233 [details] Bug 1376173 - Adding compressed version of session file to fixed session restore Talos test regression. https://reviewboard.mozilla.org/r/154164/#review159270 Yeah, this looks good. Please show a try build which shows that this fixes the regression. We can do this together, so I can show & tell you how to use PerfHerder.

Sorry for the delay - the all hands bogged me down pretty hard. :( But it looks like you've worked this out with mikedeboer. Thanks!

I don't think the results from try are very reliable. I did four push try today to get the comparisons: + The first comparison: https://goo.gl/ABHra3 + The second comparison: https://goo.gl/DqnoPd The restoration time in linux seems to decrease but not much.

Are we going to land the reviewed patch? (I'm not that interested in the Talos regression, but rather looking for follow-up on https://bugzilla.mozilla.org/show_bug.cgi?id=934310#c946, in hopes of resolving bug 1343532, which is fairly urgent.)

Geoff, I think the patch is going to land. However, if the patch is not going to fixed the regression, I may have to file another bug and submit the reviewed patch there. The current Talos session restore test loads the wrong session files. When a new browser cycle begins, the sessionstore.js and sessioncheckpoints.json are resinstalled. However, in the profile, the sessionstore.jsonlz4 is still there and Firefox will load that file instead of sessionstore.js. I'm going check whether the patch fixes the regression.

I tested the patch 10 times. Here are the results: + Without the patch (*): > 617, 589, 586, 589, 641, 583, 574, 610, 572, 614 Mean: 597.8 + With the patch: > 638, 631, 617, 634, 620, 594, 588, 606, 626, 562 Mean: 611.6 The patch increases the time to restore the session, which is weird, because (*) has more tabs to restored.

File the bug 1380220 and move my patch here to that bug.

I tested the lz4 compression on my Linux machine. It seems that the time it takes to read .jsonlz4 usually 10 milliseconds longer, compares to reading raw text file. I think this regression will be a WONTFIX as this is how lz4 compression works.