1376173 - 1.87% sessionrestore (linux64) regression on push 5e40f7e493966d724d36de5d25689018c4c65d6c (Fri Jun 23 2017)

Status: RESOLVED WONTFIX

(Firefox :: Session Restore, defect)

Description

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

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

Comment 1

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

: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?

Comment 2

[Bao Quan [:beekill]]

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?

Comment 3

[Bao Quan [:beekill]]

: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?

Comment 4

[:Gijs (out for now; he/him)]

(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 .

Comment 5

[Bao Quan [:beekill]]

(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

Comment 6

[Mike de Boer [:mikedeboer]]

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?

Comment 7

[Bao Quan [:beekill]]

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?

Comment 8

[Bao Quan [:beekill]]

(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

Comment 9

[Mike de Boer [:mikedeboer]]

(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 10

[Bao Quan [:beekill]]

Attachment: Bug 1376173 - Adding compressed version of session file to fixed session restore Talos test regression.

Review commit: https://reviewboard.mozilla.org/r/154164/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/154164/

Comment 11

[Mike de Boer [:mikedeboer]]

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.

Comment 12

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

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

Comment 13

[Bao Quan [:beekill]]

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.

Comment 14

[Geoff Brown [:gbrown]]

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.)

Comment 15

[Bao Quan [:beekill]]

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.

Comment 16

[Bao Quan [:beekill]]

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.

Comment 17

[Bao Quan [:beekill]]

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

Comment 18

[Bao Quan [:beekill]]

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.