1387535 - 2.14 - 7.57% a11yr / damp / tart / tcanvasmark / tsvgr_opacity / tsvgx (linux64, windows10-64, windows7-32) regression on push bbe6f10263b955dd1a4add95089b3da6ab3ee602 (Thu Aug 3 2017)

Status: RESOLVED FIXED

(Core :: JavaScript Engine: JIT, defect)

Description

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

Talos has detected a Firefox performance regression from push:

https://hg.mozilla.org/integration/mozilla-inbound/pushloghtml?changeset=bbe6f10263b955dd1a4add95089b3da6ab3ee602

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

Regressions:

  8%  tart summary windows7-32 pgo e10s     4.89 -> 5.26
  7%  damp summary windows7-32 pgo e10s     207.25 -> 221.53
  7%  a11yr summary windows7-32 pgo e10s    461.61 -> 492.25
  5%  tsvgx summary windows7-32 pgo e10s    432.72 -> 455.21
  5%  damp summary windows10-64 pgo e10s    209.99 -> 219.77
  5%  tcanvasmark summary windows7-32 pgo e10s9,754.00 -> 9,311.04
  4%  tsvgr_opacity summary windows7-32 pgo e10s352.17 -> 365.39
  2%  damp summary linux64 opt e10s         247.52 -> 252.82


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

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

:jandem, I know :igoldan emailed you out of band, these are pgo only regressions, but sustained across branches and covering a lot of tests/platforms- I suspect this is a PGO artifact, sort of the lottery of the unlucky- do you have ideas on fixing this, considering backing this out, or just accepting it?

Comment 2

[Jan de Mooij [:jandem]]

(In reply to Joel Maher ( :jmaher) (UTC-9) from comment #1)
> I suspect this is a PGO artifact, sort of the lottery of
> the unlucky- do you have ideas on fixing this, considering backing this out,
> or just accepting it?

Yeah this is sad :( I'll try to rewrite this code and will do some PGO Try pushes to see if it helps...

Comment 3

[Jan de Mooij [:jandem]]

I have some patches that seem to fix this on Try and are a pretty nice clean up anyway.

Comment 4

[Jan de Mooij [:jandem]]

Attachment: Part 1 - Clean up JmpSrc and JmpDst

JmpSrc and JmpDst have some unused methods and fields we're not using.

Comment 5

[Jan de Mooij [:jandem]]

Attachment: Part 2 - Encapsulate Label better

LabelBase::INVALID_OFFSET is stored as -1, and a bunch of other code relies on it being this exact value. In particular label->use() will return -1 the first time it's used.

This dependency is not great and makes it hard to rewrite Label's internals, so this patch fixes it. label->use() no longer returns the previous offset and INVALID_OFFSET is now private.

This patch also removes more dead code (like AbsoluteLabel).

Comment 6

[Jan de Mooij [:jandem]]

Attachment: Part 3 - Refactor Label implementation

Label can now store offset_ as unsigned integer (instead of signed) with INVALID_OFFSET being the max uint31 value (instead of -1). This has some advantages, for instance we can store a larger range of values and it makes it clear offset_ is never negative.

I also swapped the offset_ and bound_ fields, strangely enough this seems to be necessary to make Win32 PGO happy :/

Comment 7

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/fc97fe73b4d2
part 1 - Clean up JmpSrc and JmpDst. r=bhackett
https://hg.mozilla.org/integration/mozilla-inbound/rev/45d45d3e658f
part 2 - Encapsulate Label fields better. r=bhackett
https://hg.mozilla.org/integration/mozilla-inbound/rev/afcf2e48a28e
part 3 - Rewrite Label implementation to hopefully fix PGO regression. r=bhackett

Comment 8

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

https://hg.mozilla.org/mozilla-central/rev/fc97fe73b4d2
https://hg.mozilla.org/mozilla-central/rev/45d45d3e658f
https://hg.mozilla.org/mozilla-central/rev/afcf2e48a28e

Comment 9

[Ionuț Goldan [:igoldan]]

Improvements noted after latest push. Thanks!

== Change summary for alert #8652 (as of August 08 2017 13:41 UTC) ==

Improvements:

  7%  a11yr summary windows7-32 pgo e10s     496.38 -> 463.37
  6%  tart summary windows7-32 pgo e10s      5.35 -> 5.05
  5%  tsvgx summary windows7-32 pgo e10s     457.73 -> 434.87
  4%  tcanvasmark summary windows7-32 pgo e10s9,348.33 -> 9,724.12
  4%  tsvgr_opacity summary windows7-32 pgo e10s365.80 -> 352.69

For up to date results, see: https://treeherder.mozilla.org/perf.html#/alerts?id=8652