Last Comment Bug 774330 - Huge performance regression in Emscripten generated code
: Huge performance regression in Emscripten generated code
Status: RESOLVED FIXED
: perf, regression
Product: Core
Classification: Components
Component: JavaScript Engine (show other bugs)
: Trunk
: All All
: -- normal (vote)
: mozilla17
Assigned To: Alex Crichton [:acrichto]
:
Mentors:
: 774438 (view as bug list)
Depends on:
Blocks: gecko-games 772078
  Show dependency treegraph
 
Reported: 2012-07-16 09:43 PDT by :Ehsan Akhgari (out sick)
Modified: 2012-07-19 11:12 PDT (History)
13 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---
+
fixed


Attachments
patch (1.19 KB, patch)
2012-07-16 18:04 PDT, Alex Crichton [:acrichto]
luke: review+
lukasblakk+bugs: approval‑mozilla‑aurora+
Details | Diff | Review

Description :Ehsan Akhgari (out sick) 2012-07-16 09:43:32 PDT
The 07/13 nightly regressed the performance of Emscripten generated code by a huge amount (about 30 times slower).

The regression range is http://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=46804c31366b&tochange=6489be1890c0.  I'm currently bisecting it to see what changeset is at fault.
Comment 1 Gregor Wagner [:gwagner] 2012-07-16 10:00:52 PDT
Might be bug 765435 if you are dealing with >500MB JS heap.
Comment 2 :Ehsan Akhgari (out sick) 2012-07-16 10:02:26 PDT
(In reply to Gregor Wagner [:gwagner] from comment #1)
> Might be bug 765435 if you are dealing with >500MB JS heap.

That could very well be it.  I'm half way through bisection so I don't wanna back out that single patch right now and ruin my bisection session, I'll update the bug as soon as I confirm this.
Comment 3 Gregor Wagner [:gwagner] 2012-07-16 10:16:20 PDT
You could just set javascript.options.mem.gc_dynamic_heap_growth to false in about:config
Comment 4 :Ehsan Akhgari (out sick) 2012-07-16 13:01:33 PDT
(In reply to Gregor Wagner [:gwagner] from comment #3)
> You could just set javascript.options.mem.gc_dynamic_heap_growth to false in
> about:config

That didn't help, and unfortunately hg bisect took me to a merge changeset:

$ hg bis -gThe first bad revision is:
changeset:   99101:6489be1890c0
parent:      99070:6a640ca09064
parent:      99100:1f4ad785cca8
user:        Ryan VanderMeulen <ryanvm@gmail.com>
date:        Thu Jul 12 20:46:27 2012 -0400
summary:     Merge the last PGO-green inbound changeset to m-c.

Maybe I should try bisecting in git...
Comment 5 Gregor Wagner [:gwagner] 2012-07-16 13:08:47 PDT
Mabye it's not me :)
If it is GC related you can see it if you turn on javascript.options.mem.log and watch the Error Console for GC events.
Comment 6 Alon Zakai (:azakai) 2012-07-16 13:17:18 PDT
I believe hg bisect should bisect through merges intelligently, if you start it early enough (from a shared ancestor of the merged branches) - should be the same as with git.
Comment 7 :Ehsan Akhgari (out sick) 2012-07-16 13:18:19 PDT
(In reply to comment #5)
> Mabye it's not me :)
> If it is GC related you can see it if you turn on javascript.options.mem.log
> and watch the Error Console for GC events.

What would I watch for?  A lot of GCs happening?
Comment 8 :Ehsan Akhgari (out sick) 2012-07-16 13:21:56 PDT
(In reply to comment #6)
> I believe hg bisect should bisect through merges intelligently, if you start it
> early enough (from a shared ancestor of the merged branches) - should be the
> same as with git.

I've already started to bisect with git.  Git should be able to handle this type of bisection fine, even if you don't use a common ancestor...
Comment 9 Gregor Wagner [:gwagner] 2012-07-16 13:24:43 PDT
(In reply to Ehsan Akhgari [:ehsan] from comment #7)
> (In reply to comment #5)
> > Mabye it's not me :)
> > If it is GC related you can see it if you turn on javascript.options.mem.log
> > and watch the Error Console for GC events.
> 
> What would I watch for?  A lot of GCs happening?

Yes. Maybe compare the number of GCs with a nightly that doesn't show the regression.
Comment 10 Ryan VanderMeulen [:RyanVM] 2012-07-16 15:38:19 PDT
Why not bisect on inbound?
Comment 11 :Ehsan Akhgari (out sick) 2012-07-16 17:20:30 PDT
OK, found the offending changeset:

http://hg.mozilla.org/mozilla-central/rev/17bc02a42a1a

Alex, could you please take a look?

(This only happens if the Gecko Profiler add-on is turned on.)
Comment 12 :Ehsan Akhgari (out sick) 2012-07-16 17:21:42 PDT
*** Bug 774438 has been marked as a duplicate of this bug. ***
Comment 13 Alex Crichton [:acrichto] 2012-07-16 17:22:56 PDT
Just to be sane, the performance is normal if the profiler is turned off?
Comment 14 Alex Crichton [:acrichto] 2012-07-16 18:04:15 PDT
Created attachment 642815 [details] [diff] [review]
patch

I ran this with the demo in bug 774438 and the fps was back up where it was without profiling enabled.

Fun fact: the interpreter is a lot slower than the JIT.
Comment 15 Luke Wagner [:luke] 2012-07-16 18:06:24 PDT
Comment on attachment 642815 [details] [diff] [review]
patch

Schwoops

nit: no { } around single-line then branch
Comment 16 Alex Crichton [:acrichto] 2012-07-16 18:12:47 PDT
http://hg.mozilla.org/integration/mozilla-inbound/rev/36fcd6eabf67
Comment 17 :Ehsan Akhgari (out sick) 2012-07-16 18:44:48 PDT
Thanks Alex for the quick fix!  :-)
Comment 18 Vladimir Vukicevic [:vlad] [:vladv] 2012-07-16 20:32:08 PDT
If that bug went to aurora, we should get the patch on that branch as well.
Comment 19 Luke Wagner [:luke] 2012-07-16 20:49:26 PDT
Comment on attachment 642815 [details] [diff] [review]
patch

[Approval Request Comment]
Bug caused by (feature/regressing bug #): 772078
User impact if declined: profiling turns of the jit entirely
Testing completed (on m-c, etc.): m-c
Risk to taking this patch (and alternatives if risky): very low: only exercised when profiling is on
Comment 20 :Ehsan Akhgari (out sick) 2012-07-16 21:02:43 PDT
(In reply to comment #18)
> If that bug went to aurora, we should get the patch on that branch as well.

Absolutely!
Comment 21 Ed Morley [:emorley] 2012-07-17 02:10:28 PDT
https://hg.mozilla.org/mozilla-central/rev/36fcd6eabf67
Comment 22 Lukas Blakk [:lsblakk] use ?needinfo 2012-07-18 17:51:12 PDT
Comment on attachment 642815 [details] [diff] [review]
patch

low risk, tested fix, approving.
Comment 23 :Ehsan Akhgari (out sick) 2012-07-19 11:12:32 PDT
http://hg.mozilla.org/releases/mozilla-aurora/rev/c5081fdf23e0

Note You need to log in before you can comment on or make changes to this bug.