1223082 - 3% Linux 64 sessionrestore regression on Fx-Team-Non-PGO on November 06, 2015 from push c951580b6f9b81f7dcab62b40d03ad96d666e1ca

Status: RESOLVED WONTFIX

(Testing :: Talos, defect)

Description

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

Talos has detected a Firefox performance regression from your commit c951580b6f9b81f7dcab62b40d03ad96d666e1ca.  We need to address this regression.

This is a list of all known regressions and improvements related to your bug:
http://alertmanager.allizom.org:8080/alerts.html?rev=c951580b6f9b81f7dcab62b40d03ad96d666e1ca&showAll=1

On the page above you can see Talos 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, please see: https://wiki.mozilla.org/Buildbot/Talos/Tests#sessionrestore.2Fsessionrestore_no_auto_restore

Reproducing and debugging the regression:
If you would like to re-run this Talos test on a potential fix, use try with the following syntax:
try: -b o -p linux64 -u none -t other  # add "mozharness: --spsProfile" to generate profile data

To run the test locally and do a more in-depth investigation, first set up a local Talos environment:
https://wiki.mozilla.org/Buildbot/Talos/Running#Running_locally_-_Source_Code

Then run the following command from the directory where you set up Talos:
talos --develop -e <path>/firefox -a sessionrestore

Making a decision:
As the patch author we need your feedback to help us handle this regression.
*** Please let us know your plans by Thursday, or the offending patch 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)]

:Yoric, you have a patch that might affect sessionrestore, this seems to be a linux64 only regression, can you weigh in here if this is probably related to your patch?  I can push to try to narrow this down if needed.

Comment 2

[Daosheng Mu[:daoshengmu]]

Bug 1215147 looks good. If you have any question, feel free to contact me. Thanks.
https://treeherder.mozilla.org/#/jobs?repo=try&revision=91ba8b15e70b

Comment 3

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

(In reply to Joel Maher (:jmaher) from comment #1)
> :Yoric, you have a patch that might affect sessionrestore, this seems to be
> a linux64 only regression, can you weigh in here if this is probably related
> to your patch?  I can push to try to narrow this down if needed.

This looks very unlikely. My patch affects Session Restore data collection. Normally, the code is not executed at all during startup.

Comment 4

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

NI back to Joel to narrow this down, as per comment 1.

Comment 5

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

and the winner is bug 1216250!

you can see some try pushes here:
https://treeherder.mozilla.org/#/jobs?repo=try&author=[e-mail]&fromchange=86a30ff621bf&group_state=expanded

the baseline (prior to this set of patches) is rev ff8ebe5251ec and we have ~2120 as the value for session restore.  The next patch in the series is rev bb09cf018056889554a4b2cfa7a629a043e4c49c (bug 1216250) and this has a value of 2180+.

Yoric, this is your patch, lets take a second look at it and see.

Comment 6

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

removing some bugs and cc'd folks.

Comment 7

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

I have pushed to try with the offending patch backed out to see if this would give the numbers properly:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=8f5bf2443e52

Comment 8

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

backing out the patch does in fact return the numbers back to normal.

Comment 9

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

Very strange. Either it's a bug in Session Restore itself or it's the impact of the code size (but then, I only added ~10 loc). I'll investigate.

Comment 10

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

thanks Yoric!

Comment 11

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

Yoric, any luck figuring this out?

Comment 12

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

Not yet, I'm attempting to determine whether that code can actually be triggered during startup.

Comment 13

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

Mmmh... The code *is* triggered during startup, while it shouldn't. This looks like a bug in Session Restore. Investigating further.

Comment 14

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

Sorry, experimentation error. As expected, the code added in the above bug is *not* triggered during the Talos benchmark. My best guess is that the code is either JIT-compiled or cached slightly less efficiently. In either case, there isn't much I can do about this bug.

Comment 15

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

thanks for looking into this, I vote for resolving this as wontfix then.

Comment 16

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

Agreed.