1274588 - 2.45% damp (windows7-32) regression on push 2e29695fee87 (Tue May 17 2016)

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

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

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

This is a list of all known regressions and improvements related to the push:
https://treeherder.mozilla.org/perf.html#/alerts?id=1241

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#DAMP

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 win32 -u none -t g2-e10s[Windows 7] --rebuild 5  # add "mozharness: --spsProfile" to generate profile data

(we suggest --rebuild 5 to be more confident in the results)

To run the test locally and do a more in-depth investigation, first set up a local Talos environment:
https://wiki.mozilla.lorg/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 damp

(add --e10s to run tests in e10s mode)

Making a decision:
As the patch author we need your feedback to help us handle this regression.
*** 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)]

I am having a hard time getting all the alerts lined up, but a compare to the previous push yields:
https://treeherder.mozilla.org/perf.html#/compare?originalProject=mozilla-inbound&originalRevision=a7f37ab1e0c3&newProject=mozilla-inbound&newRevision=2e29695fee87&framework=1

this isn't pgo though, and the regressions are slightly different there.

what I see in the compare view is;
5% win7 damp
3% winxp session restore [auto restore] e10s


we have a few other damp regressions on the tree around the same time, same with a ts_paint- looking to see if those are related at all.

:nbp, can you take a look at this and determine what is the root cause and how we should resolve this issue?

Comment 2

[Nicolas B. Pierron [:nbp]]

(In reply to Joel Maher (:jmaher) from comment #1)
> I am having a hard time getting all the alerts lined up, but a compare to
> the previous push yields:
> https://treeherder.mozilla.org/perf.html#/compare?originalProject=mozilla-
> inbound&originalRevision=a7f37ab1e0c3&newProject=mozilla-
> inbound&newRevision=2e29695fee87&framework=1

The Base & New are miss-leading as it does not show all the patches in the patch series.

This issue concern all patches from Bug 1261826, except for the part 10.1 (which is not landed yet) and part 5 (which is not going to land).  Bug 1261826 is one step toward enabling a Jit optimization (Bug 1229813).

Part 1, 1.5, 2 and 6 are minimal changes, and I do not expect them to have any major performance impact.

Part 9 and 10, are not supposed to be used today, as these are adding branches for code which is not yet being executed (unless code coverage is enabled by the Debugger).  I would not expect these to have any impact on performances (except code-size wise)

The most notable impact of Bug 1261826 is that the bytecode size increased by 5.8%, as mentioned in Bug 1261826 comment 36.

Another possible reasons might be caused by the lack of inlined functions for the new code which unifies the 2 different ways for doing jumps in the bytecode.  And/or that fact that we chose the most general way of handling instead of specializing cases where there is only a single jump instruction.

If this is the bytecode size (since part 8), then I might have ways to reduce the impact, but not to remove it completely. Checking performances on part 7 should highlight if the issue comes from the part 8 to 10.

If this is the lack of inlined function, then the problem should already exists since the part 4.  Thus checking performances on part 3, and part 7 should highlight if the issue is already present between part 4 and 7.  Note, I would expect talos performances on part 3, to behave the same way as on the "BASE" changeset.

(In reply to Joel Maher (:jmaher) from comment #1)
> :nbp, can you take a look at this and determine what is the root cause and
> how we should resolve this issue?

We should send try runs on part 7 (and 3 to remove doubts if present on part 7), to know if the problems comes from part 4, or part 8.  Once we have results we should decide how to fix it.

Comment 3

[Nicolas B. Pierron [:nbp]]

I've pushed part 7 to try, in order to compare talos results:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=dcf5d8dcfe83a27546304366091f2b889f950201

I will check later today, or on Monday to see how we should continue.

Comment 4

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

I don't see a difference in the try push, so I assumed part 7 isn't changing numbers.

Comment 5

[Nicolas B. Pierron [:nbp]]

In the mean time I will work on a patch to fold consecutive jump targets, and if this is still an issue, I will look for removing fallthrough jump.

Comment 6

[Nicolas B. Pierron [:nbp]]

Try is no longer open for the moment …

(In reply to Nicolas B. Pierron [:nbp] from comment #5)
> In the mean time I will work on a patch to fold consecutive jump targets,
> and if this is still an issue, I will look for removing fallthrough jump.

Comment 7

[Nicolas B. Pierron [:nbp]]

Attachment: part 0.1 - Add MOZ_MUST_USE to popStatement functions.

Comment 8

[Jason Orendorff [:jorendorff]]

Comment on attachment 8755926 [details] [diff] [review]
part 0.1 - Add MOZ_MUST_USE to popStatement functions.

Review of attachment 8755926 [details] [diff] [review]:
-----------------------------------------------------------------

Sure.

Comment 9

[Marcia Knous [:marcia]]

Tracking 49+ since it would be great to address this perf issue.

Comment 10

[Nicolas B. Pierron [:nbp]]

Attachment: part 1.0 - Alias consecutive jump targets.

Comment 11

[Nicolas B. Pierron [:nbp]]

Attachment: part 1.1 - Update code coverage reports to accounts for the aliased jump targets.

Comment 12

[Nicolas B. Pierron [:nbp]]

Attachment: part 2 - Rely on aliased jump target to remove extra popStatement function.

Comment 13

[Nicolas B. Pierron [:nbp]]

The current set of patches seems to fix the issue:

try-push: (based on old patches)
https://treeherder.mozilla.org/#/jobs?repo=try&revision=0c4bfadc236370c6b6da2c7a51fa34c5e5ec3051

https://treeherder.mozilla.org/perf.html#/compare?originalProject=mozilla-inbound&originalRevision=a7f37ab1e0c3&newProject=try&newRevision=0c4bfadc2363&framework=1&showOnlyImportant=0

Comment 14

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

this is exciting- can we get this landed and resolved? :)

Comment 15

[Nicolas B. Pierron [:nbp]]

(In reply to Joel Maher (:jmaher) from comment #14)
> this is exciting- can we get this landed and resolved? :)

Part 1.0 and 1.1 are the 2 mandatory patches for fixing this issue.

Comment 16

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

oh sorry, I saw r+ in the last comment and didn't look at the other patches- I will wait :)

Comment 17

[Jason Orendorff [:jorendorff]]

Comment on attachment 8756471 [details] [diff] [review]
part 1.0 - Alias consecutive jump targets.

Review of attachment 8756471 [details] [diff] [review]:
-----------------------------------------------------------------

::: js/src/frontend/BytecodeEmitter.h
@@ +379,5 @@
> +    // Check if the last emitted opcode is a jump target.
> +    bool lastOpcodeIsJumpTarget() const {
> +        return offset() - current->lastTarget.offset == ptrdiff_t(JSOP_JUMPTARGET_LENGTH);
> +    }
> +    // JumpTarget should not be part of the emitted statement, as they can be

style nit: blank line before comment

@@ +383,5 @@
> +    // JumpTarget should not be part of the emitted statement, as they can be
> +    // aliased by multiple statements. If we included the jump target as part of
> +    // the statement we might have issues where the enclosing statement might
> +    // not contain all the opcodes of the enclosed statements.
> +    ptrdiff_t lastStmtOffset() const {

This should be called lastNonJumpTargetOffset(), as lastStmtOffset() seems to be saying something else.

Verbose, I know, but it's only called twice.

Comment 18

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/cd714f1275be
part 0.1 - Add MOZ_MUST_USE to popStatement functions. r=jorendorff
https://hg.mozilla.org/integration/mozilla-inbound/rev/0792b0daef2a
part 1.0 - Alias consecutive jump targets. r=jorendorff
https://hg.mozilla.org/integration/mozilla-inbound/rev/32eb8332d618
part 1.1 - Update code coverage reports to accounts for the aliased jump targets. r=bhackett
https://hg.mozilla.org/integration/mozilla-inbound/rev/e838c11fd532
part 2 - Rely on aliased jump target to remove extra popStatement function. r=jorendorff

Comment 19

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/cd714f1275be
https://hg.mozilla.org/mozilla-central/rev/0792b0daef2a
https://hg.mozilla.org/mozilla-central/rev/32eb8332d618
https://hg.mozilla.org/mozilla-central/rev/e838c11fd532

Comment 20

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

I went to verify this and find alerts to show improvements and the graphs look the same, there are no improvements.

Possibly there is not much else to do?

Comment 21

[Nicolas B. Pierron [:nbp]]

(In reply to Joel Maher (:jmaher) from comment #20)
> I went to verify this and find alerts to show improvements and the graphs
> look the same, there are no improvements.
> 
> Possibly there is not much else to do?

There is still one option.  Currently we are producing jump-target opcodes for fallthrough cases of jumps, but intuitively this should not be needed if the jump target is only a fallthrough.  Doing so, implies that counters instrumentation should be placed on the branch-side instead of being on the target side.

I opened bug 1278207, to address this issue as this is likely a larger patch.

Comment 22

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

ok, thanks for the information!  Lets leave this bug open and see how bug 1278207 affects this test.

Comment 23

[Avi Halachmi (:avih)]

(In reply to Joel Maher (:jmaher) from comment #22)
> ok, thanks for the information!  Lets leave this bug open and see how bug
> 1278207 affects this test.

While useful to keep tracking this, we should keep in mind that the goal is not the test numbers, but rather making sure that performance tradeoffs implied by a patch are made explicit and accounted for (of course, with possible followups in case the owner thinks it's due).

Comment 24

[Nicolas B. Pierron [:nbp]]

(In reply to Joel Maher (:jmaher) from comment #20)
> I went to verify this and find alerts to show improvements and the graphs
> look the same, there are no improvements.
> 
> Possibly there is not much else to do?

I fixed another regression caused by the same patch set. (Bug 1273955)  It might be plausible that this help mitigate the regression seen here as well.

Comment 25

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

Are you thinking that the other fix introduced a regression which nullified the improvement we were expecting?  If that is the case, are we fine with wontfixing this, or should we do more investigation?

Comment 26

[Nicolas B. Pierron [:nbp]]

(In reply to Joel Maher (:jmaher) from comment #25)
> Are you thinking that the other fix introduced a regression which nullified
> the improvement we were expecting?  If that is the case, are we fine with
> wontfixing this, or should we do more investigation?

I am thinking that other fix, might also fix this issue.

Comment 27

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

sorry for the confusion, I will wait for bug 1278207 then!