1378791 - stylo: 8.32 - 9.32% tp5n main_startup_fileio (windows7-32) regression on push 06ec956f753346236bf89f2bead2066d22778fc2 (Wed Jul 5 2017)

Status: RESOLVED WONTFIX

(Core :: CSS Parsing and Computation, defect, P2)

Description

[Ionuț Goldan [:igoldan]]

Talos has detected a Firefox performance regression from push:

https://hg.mozilla.org/integration/autoland/pushloghtml?changeset=06ec956f753346236bf89f2bead2066d22778fc2

As author of one of the patches included in that push, we need your help to address this regression.

Regressions:

  9%  tp5n main_startup_fileio windows7-32 opt e10s     62,055,874.58 -> 67,842,069.50
  8%  tp5n main_startup_fileio windows7-32 pgo e10s     75,003,637.38 -> 81,240,849.08


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

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

[Ionuț Goldan [:igoldan]]

The alerts for this regression got in after the ones from bug 1378667. :froydnj I guess you have to look over this one too.

Comment 2

[Emilio Cobos Álvarez (:emilio)]

I guess this is related to loading the ua stylesheets for both style backends?

Comment 3

[Chris Peterson [:cpeterson]]

jryans, can you please take a look at this Talos regression? Why are we doing more file I/O even though Stylo is disabled?

Comment 4

[Bobby Holley (:bholley)]

Hm, that regression amount looks awfully similar to the current codesize increase due to stylo (which we're tracking in bug stylo-codesize). rwood, the wiki page about this test is empty. Do you know how this test performs its measurements? I see something about xperf, but I don't see how 'startup' is identified, and what xperf bits this corresponds to.

I think dmajor is familar with xperf, so he may have some insight as well.

Comment 5

[Robert Wood [:rwood]]

(In reply to Bobby Holley (:bholley) (busy with Stylo) from comment #4)
> Hm, that regression amount looks awfully similar to the current codesize
> increase due to stylo (which we're tracking in bug stylo-codesize). rwood,
> the wiki page about this test is empty. Do you know how this test performs
> its measurements? I see something about xperf, but I don't see how 'startup'
> is identified, and what xperf bits this corresponds to.
> 
> I think dmajor is familar with xperf, so he may have some insight as well.

No sorry I'm not really familiar with xperf but can hopefully point you in the right direction:

https://wiki.mozilla.org/Buildbot/Talos/Tests#xperf

http://searchfox.org/mozilla-central/source/testing/talos/talos/xtalos

:aklotz, I see your name beside xperf on the wiki, perhaps you can help :bholley? Thanks!

Comment 6

[(Away)]

I highly suspect that it's just xul.dll bloat. This also came up in bug 1376174 comment 3.

Comment 7

[Bobby Holley (:bholley)]

cpearce, can you elaborate on what you found in bug 1376174 comment 3? Do you think there's anything we need to do here, or is it enough to just accept that patches which increase libxul size will also regress this metric?

Comment 8

[(No longer employed by Mozilla) Aaron Klotz]

(In reply to David Major [:dmajor] from comment #6)
> I highly suspect that it's just xul.dll bloat. This also came up in bug
> 1376174 comment 3.

+1

Comment 9

[Chris Pearce [:cpearce (Not reading bugmail)]]

(In reply to Bobby Holley (:bholley) (busy with Stylo) from comment #7)
> cpearce, can you elaborate on what you found in bug 1376174 comment 3? Do
> you think there's anything we need to do here, or is it enough to just
> accept that patches which increase libxul size will also regress this metric?

I didn't investigate this myself, but we discussed bug 1376174 in our team stand up, and we concluded that if the size of the binary on disk gets bigger, it's reasonable to expect that it takes longer for the binary to be read off disk into memory when the program is loaded, and so you should expect a startup time regression. This load penalty could be mitigated by moving the code into its own shared library, so that the loading happens right before the module is needed. For media, this is fine, as typically the media subsystem isn't needed right after startup. I expect the style system is needed ASAP, so it doesn't make sense to do this for stylo.

I think we need to accept this hit. It's not helpful to block adding code because it makes our binary bigger.

Comment 10

[Ralph Giles (:rillian)]

Thanks for clarifying, Chris. Closing based on that reasoning.

Comment 11

[Mike Hommey [:glandium]]

Note those regressions are about startup *file I/O*, not startup time. Of course, when xul.dll is larger, there is obviously more to read from disk.