1290502 - 3.6 - 3.66% tp5n main_startup_fileio (windows7-32) regression on push 281bbbf61f36 (Thu Jul 28 2016)

Status: VERIFIED FIXED

(Firefox :: Untriaged, defect)

Description

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

Talos has detected a Firefox performance regression from push 281bbbf61f36. As author of one of the patches included in that push, we need your help to address this regression.

Summary of tests that regressed:

  tp5n main_startup_fileio windows7-32 opt - 3.66% worse
  tp5n main_startup_fileio windows7-32 opt e10s - 3.6% worse


You can find links to graphs and comparison views for each of the above tests at: https://treeherder.mozilla.org/perf.html#/alerts?id=2128

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

:mshal, I assume this is intended, but I would like to hear from you if we intended to increase the fileio (usually read) on the main thread during startup.

Comment 2

[Michael Shal [:mshal]]

Hmm, this looks fairly similar to bug 1264276, though this time for some reason entries are being added multiple times in dependentlibs.list:

$ cat bad/firefox/dependentlibs.list  | sort | uniq -c
      4 api-ms-win-crt-convert-l1-1-0.dll
      3 api-ms-win-crt-environment-l1-1-0.dll
      2 api-ms-win-crt-filesystem-l1-1-0.dll
      1 api-ms-win-crt-heap-l1-1-0.dll
      1 api-ms-win-crt-locale-l1-1-0.dll
      3 api-ms-win-crt-math-l1-1-0.dll
      1 api-ms-win-crt-multibyte-l1-1-0.dll
      4 api-ms-win-crt-runtime-l1-1-0.dll
      4 api-ms-win-crt-stdio-l1-1-0.dll
      4 api-ms-win-crt-string-l1-1-0.dll
      2 api-ms-win-crt-time-l1-1-0.dll
      2 api-ms-win-crt-utility-l1-1-0.dll
      1 lgpllibs.dll
      2 mozglue.dll
      2 MSVCP140.dll
      1 nss3.dll
      3 VCRUNTIME140.dll
      1 xul.dll

(They should all be '1')

It's definitely not intended - I'm currently looking into it.

Comment 3

[Michael Shal [:mshal]]

The problem was there were checks to avoid duplicates, but when I switched it to use full paths internally (in order to pass the paths back to mozbuild), these checks were broken. Switching to a dict of 'filename: fullpath' seems to be much simpler and works with the de-duping.

Comment 4

[Michael Shal [:mshal]]

Attachment: Bug 1290502 - Avoid duplicate entries in dependentlibs.list;

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

Comment 5

[Alison Shiue]

This regression patch 281bbbf61f36 has been merged into Aurora, so please make sure to uplift any fixes to Aurora, thank you.

https://treeherder.mozilla.org/perf.html#/alerts?id=2185

  tp5n main_startup_fileio windows7-32 pgo - 5.08% worse
  tp5n main_startup_fileio windows7-32 pgo e10s - 5.02% worse

Comment 6

[(not currently active) Ted Mielczarek]

Comment on attachment 8776177 [details]
Bug 1290502 - Avoid duplicate entries in dependentlibs.list;

https://reviewboard.mozilla.org/r/68074/#review65934

Comment 7

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/9d5365ee6944
Avoid duplicate entries in dependentlibs.list; r=ted

Comment 8

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

I had to back this out in https://hg.mozilla.org/integration/autoland/rev/46cc19e88e94 for mass bustage: https://treeherder.mozilla.org/#/jobs?repo=autoland&revision=9d5365ee6944046216d3fff3662de79680d41aaf

Comment 9

[Michael Shal [:mshal]]

I'm looking into it - not sure what happened. I only ran builds on try, not tests.

Comment 10

[Michael Shal [:mshal]]

So it turns out the order of libraries in dependentlibs.list matters. Switching from a dict to an OrderedDict seems to work.

Comment 11

[Michael Shal [:mshal]]

Comment on attachment 8776177 [details]
Bug 1290502 - Avoid duplicate entries in dependentlibs.list;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/68074/diff/1-2/

Comment 12

[(not currently active) Ted Mielczarek]

Comment on attachment 8776177 [details]
Bug 1290502 - Avoid duplicate entries in dependentlibs.list;

https://reviewboard.mozilla.org/r/68074/#review66324

Comment 13

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/2eb3854c6c47
Avoid duplicate entries in dependentlibs.list; r=ted

Comment 14

[Alison Shiue]

This fix did have an improvement:
https://treeherder.mozilla.org/perf.html#/alerts?id=2249

Summary of tests that improved:

  tp5n main_startup_fileio windows7-32 opt - 3.54% better
  tp5n main_startup_fileio windows7-32 opt e10s - 3.57% better

Thank you for this patch!

Comment 15

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/2eb3854c6c47

Comment 16

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

we should consider uplifting this to aurora, :mshal- any concerns with doing that?

Comment 17

[Michael Shal [:mshal]]

(In reply to Joel Maher ( :jmaher ) from comment #16)
> we should consider uplifting this to aurora, :mshal- any concerns with doing
> that?

Nope, hopefully it just works :)

Comment 18

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

Comment on attachment 8776177 [details]
Bug 1290502 - Avoid duplicate entries in dependentlibs.list;

Approval Request Comment
[Feature/regressing bug #]:
[User impact if declined]:
[Describe test coverage new/current, TreeHerder]:
[Risks and why]: 
[String/UUID change made/needed]:

I am not sure about all these things- maybe :mshal or :ted could highlight any possible differences for the Approval request comment.  Overall, this fixes a file IO regression that landed shortly before the merge to aurora.

Comment 19

[Michael Shal [:mshal]]

Bug 1254115 was supposed to be a build config only change, with identical output. Until the patch in this bug, I didn't quite get the "identical output" part right though :). So if we're happy now with how things look on central, I think there should be no impact to uplift this to aurora, and we should do so to prevent the startup time regression from being released.

Comment 20

[Ritu Kothari (:ritu) (Inactive, please n-i to RyanVM, jcristau, or pascal)]

Hi Joel, could you please confirm that this fix addresses the perf regression that we noticed? If yes, I can approve uplift to Aurora. Thanks!

Comment 21

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

I have verified this is fixed on nightly!  thanks for checking :)

Comment 22

[Ritu Kothari (:ritu) (Inactive, please n-i to RyanVM, jcristau, or pascal)]

(In reply to Joel Maher ( :jmaher ) from comment #21)
> I have verified this is fixed on nightly!  thanks for checking :)

Great! Thanks.

Comment 23

[Ritu Kothari (:ritu) (Inactive, please n-i to RyanVM, jcristau, or pascal)]

Comment on attachment 8776177 [details]
Bug 1290502 - Avoid duplicate entries in dependentlibs.list;

Perf regression that was verified on Nightly, Aurora50+

Comment 24

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/582822e22e47