1342016 - Regression on Jan 11th 2017 on misc benchmarks

Status: RESOLVED FIXED

(Core :: JavaScript Engine: JIT, defect, P1)

Description

[Hannes Verschore [:h4writer]]

After bug 1329901 landed AWFY reported several regressions on different machines:

== Change summary for alert #4780 (as of January 11 2017 14:52 UTC) ==
> Regressions:
> 
>  13%  misc 0.8 bugs-608733-trace-compiler linux32 opt shell     57.72 -> 64.95
>  11%  misc 0.8 bugs-608733-interpreter linux32 opt shell        247 -> 273.98
>  10%  ss 1.0.1 fannkuch linux32 opt shell                       6.28 -> 6.92
>   5%  misc 0.8 bugs-508716-fluid-dynamics linux32 opt shell     100.99 -> 106.25
> 
> For up to date results, see: https://treeherder.mozilla.org/perf.html#/alerts?id=4780

== Change summary for alert #4796 (as of January 11 2017 15:21 UTC) ==
> Regressions:
> 
>  16%  ss 1.0.2 access-fannkuch windows10-64 opt      7.4 -> 8.6
> 
> For up to date results, see: https://treeherder.mozilla.org/perf.html#/alerts?id=4796

== Change summary for alert #4781 (as of January 11 2017 14:06 UTC) ==
> Regressions:
> 
>  14%  misc 0.8 bugs-608733-interpreter linux64 opt shell        213.16 -> 243.08
>  11%  ss 1.0.1 fannkuch linux64 opt shell                       5.8 -> 6.45
>  11%  misc 0.8 bugs-1131099-lodash1 linux64 opt shell           55.12 -> 61.18
>  10%  misc 0.8 bugs-1131099-lodash2 linux64 opt shell           58.67 -> 64.46
>   3%  misc 0.8 bugs-608733-trace-compiler linux64 opt shell     56.81 -> 58.65
>   3%  misc 0.8 bugs-508716-fluid-dynamics linux64 opt shell     103.7 -> 106.46
> 
> Improvements:
> 
>   3%  misc 0.8 map-iterator linux64 opt shell     65.83 -> 63.87
> 
> For up to date results, see: https://treeherder.mozilla.org/perf.html#/alerts?id=4781

== Change summary for alert #4777 (as of January 11 2017 11:12 UTC) ==
> 
> Regressions:
> 
>   6%  ss 1.0.1 format-xparb linux64 opt shell              14.42 -> 15.24
>   3%  kraken 1.1 stringify-tinderbox linux64 opt shell     42.02 -> 43.43
> 
> For up to date results, see: https://treeherder.mozilla.org/perf.html#/alerts?id=4777

Comment 1

[Hannes Verschore [:h4writer]]

Sean can you take a look?

Comment 2

[Hannes Verschore [:h4writer]]

The benchmark is located in:
https://github.com/h4writer/arewefastyet/tree/master/benchmarks/misc

Comment 3

[Hannes Verschore [:h4writer]]

needinfo ping?

Comment 4

[Sean Stangl [:sstangl]]

Attachment: 0001-Revert-Bug-1329901-Remove-expensive-isObservableOper.patch

Just for posterity:

Before removing an unreachable block, we look at its entry resume point. For each of the things used by the resume point, if that producer is observable outside the current frame, it's marked as "hasUseRemoved", which is interpreted by later passes to mean that the instruction has uses outside of SSA.

The patch in Bug 1329901 removed the "isObservable" check, for an extremely significant speedup in Google Docs script compilation and a small benchmark regression.

The reason for the slowdown is that because these instructions are now marked such that SSA doesn't hold for them, they remain live. In at least the case of fannkuch, this prevents Range Analysis from restricting ranges. In a tight loop, that effect gets magnified.

Some thoughts on this:

1. It looks like the real fix would be complicated.

2. Since it's requested that this performance regression fix be backported to 53, backporting a real fix would be unnecessarily risky.

3. Backing out the original patch in 53-55 is therefore the only plausible solution, *except* in the case that we determine that a speedup in Google Docs compilation is worth a regression in access-fannkuch.

4. A big question here is why we have a special flag that causes future passes to disregard SSA. Complicating the graph with invisible flags is a good way to make it difficult to reason about. There are designs that would allow expressing that invariant in graphical form. Secondly, if an instruction is observable outside of the frame, shouldn't it be the responsibility of the code that seeks to remove that instruction to determine if it is safe to do so?

This patch backs out Bug 1329901. nbp is on vacation, so asking r?h4writer.

Comment 5

[Nicolas B. Pierron [:nbp] {away}]

Wouldn't the idea suggested in Bug 1329901 comment 4, be easy to implement?

Comment 6

[Sean Stangl [:sstangl]]

(In reply to Nicolas B. Pierron [:nbp] from comment #5)
> Wouldn't the idea suggested in Bug 1329901 comment 4, be easy to implement?

At least personally, that seems like an inappropriate patch to take in Beta.

Comment 7

[Sean Stangl [:sstangl]]

Attachment: 0001-Bug-1342016-Fast-path-for-isObservableSlot-.-r-nbp.patch

Here's another simple option.

This un-regresses the benchmarks (verified on Fannkuch) and succeeds in keeping Google Docs compilation time low. (~28% to 4% in FlagAllOperandsAsHavingRemovedUses).

If this patch looks good, I would take the other patch for 53 and 54, and land this only in 55.

Comment 8

[Hannes Verschore [:h4writer]]

Comment on attachment 8848642 [details] [diff] [review]
0001-Revert-Bug-1329901-Remove-expensive-isObservableOper.patch

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

For aurora and beta. If you want a try-run on awfy, feel free to post a try-revision and I will run it.

Comment 9

[Nicolas B. Pierron [:nbp] {away}]

Comment on attachment 8849618 [details] [diff] [review]
0001-Bug-1342016-Fast-path-for-isObservableSlot-.-r-nbp.patch

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

::: js/src/jit/CompileInfo.h
@@ +448,5 @@
>      // would have to be executed and that they cannot be removed even if they
>      // are unused.
> +    inline bool isObservableSlot(uint32_t slot) const {
> +        if (slot >= firstLocalSlot())
> +            return false;

Nice catch, this is probably the most common case as well.

Comment 10

[Sean Stangl [:sstangl]]

Comment on attachment 8848642 [details] [diff] [review]
0001-Revert-Bug-1329901-Remove-expensive-isObservableOper.patch

Approval Request Comment
[Feature/Bug causing the regression]: Reverts Bug 1329901.
[User impact if declined]: Small regression in some benchmarks.
[Is this code covered by automated tests?]: Yes.
[Has the fix been verified in Nightly?]: It's a backout of Bug 1329901. The surrounding code hasn't changed; it's safe to revert.
[Needs manual test from QE? If yes, steps to reproduce]: No.
[List of other uplifts needed for the feature/fix]: None.
[Is the change risky?]: No.
[Why is the change risky/not risky?]: Reverts a small amount of code to previous state.
[String changes made/needed]: None.

The actual fix is in Comment 7. It's a small change, but hasn't been verified in Nightly.

Comment 11

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/7524a513967b
Fast-path for isObservableSlot(). r=nbp

Comment 12

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

Backed out for assert while running bug1308578.js:

https://hg.mozilla.org/integration/mozilla-inbound/rev/00f614e9064eb283529c6fef213662939da6122b

Push with failures: https://treeherder.mozilla.org/#/jobs?repo=mozilla-inbound&revision=7524a513967bb17bf9ff6daaf6ff2872649f01cc&filter-resultStatus=testfailed&filter-resultStatus=busted&filter-resultStatus=exception&filter-resultStatus=retry&filter-resultStatus=usercancel&filter-resultStatus=runnable
Failure log: https://treeherder.mozilla.org/logviewer.html#?job_id=85412644&repo=mozilla-inbound

[task 2017-03-21T19:08:51.538831Z] Assertion failure: data.s.payload.why == why, at /home/worker/workspace/build/src/obj-spider/dist/include/js/Value.h:563
[task 2017-03-21T19:08:51.538918Z] Exit code: -11
[task 2017-03-21T19:08:51.538967Z] FAIL - debug/bug1308578.js
[task 2017-03-21T19:08:51.539115Z] TEST-UNEXPECTED-FAIL | js/src/jit-test/tests/debug/bug1308578.js | Assertion failure: data.s.payload.why == why, at /home/worker/workspace/build/src/obj-spider/dist/include/js/Value.h:563 (code -11, args "--ion-eager --ion-offthread-compile=off")
[task 2017-03-21T19:08:51.539166Z] INFO exit-status     : -11
[task 2017-03-21T19:08:51.539220Z] INFO timed-out       : False
[task 2017-03-21T19:08:51.539300Z] INFO stderr         2> Assertion failure: data.s.payload.why == why, at /home/worker/workspace/build/src/obj-spider/dist/include/js/Value.h:563
[task 2017-03-21T19:08:51.539386Z] Assertion failure: data.s.payload.why == why, at /home/worker/workspace/build/src/obj-spider/dist/include/js/Value.h:563
[task 2017-03-21T19:08:51.539429Z] Exit code: -11
[task 2017-03-21T19:08:51.539474Z] FAIL - debug/bug1308578.js
[task 2017-03-21T19:08:51.539628Z] TEST-UNEXPECTED-FAIL | js/src/jit-test/tests/debug/bug1308578.js | Assertion failure: data.s.payload.why == why, at /home/worker/workspace/build/src/obj-spider/dist/include/js/Value.h:563 (code -11, args "--ion-eager --ion-offthread-compile=off --non-writable-jitcode --ion-check-range-analysis --ion-extra-checks --no-sse3 --no-threads")
[task 2017-03-21T19:08:51.539683Z] INFO exit-status     : -11
[task 2017-03-21T19:08:51.539735Z] INFO timed-out       : False
[task 2017-03-21T19:08:51.539834Z] INFO stderr         2> Assertion failure: data.s.payload.why == why, at /home/worker/workspace/build/src/obj-spider/dist/include/js/Value.h:563

Comment 13

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/3df85eb27d47
Fast-path for isObservableSlot(). r=nbp

Comment 14

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/3df85eb27d47

Comment 15

[Gerry Chang [:gchang]]

Comment on attachment 8848642 [details] [diff] [review]
0001-Revert-Bug-1329901-Remove-expensive-isObservableOper.patch

Fix a regression. Aurora54+ & Beta53+.

Comment 16

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/8768c45a3c6e

Comment 17

[Carsten Book [:Tomcat]]

backed out the wrong patch and uplifted the correct one in https://treeherder.mozilla.org/#/jobs?repo=mozilla-aurora&revision=44602fbbbcd961f524b2f2926d2e4a1b9042e387

Comment 18

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/releases/mozilla-beta/rev/3473ebce94c59a99e73eaf7c3a6b4534c82cb16a