806480 - Emscripten benchmarks regression during JS work week

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Alon Zakai (:azakai)]

During the JS work week we saw a big regression on several emscripten benchmarks on AWFY. bhackett found it was caused by a patch that he landed during that week and would investigate. Filing this bug to track the issue since dlmalloc for example is 40% slower currently that it used to be.

(STR: Grab dlmalloc from AWFY and run it)

Comment 1

[Luke Wagner [:luke]]

Which patch was it?

Comment 2

[Alon Zakai (:azakai)]

Not sure. He said it was a patch he had written a long time ago to work around bad initial compilation of a large codebase, something to do with invalidation, and just landed now. 

If he gets back in a few weeks this might not be urgent, my only concern is if this ends up shipping in a release because it gets pushed to aurora before he returns (but even so I suppose we might be able to fix it on aurora).

Comment 3

[Sean Stangl [:sstangl]]

(In reply to Luke Wagner [:luke] from comment #1)
> Which patch was it?

The only patches in the regressing range are from Bug 781657 and Bug 788822:

http://hg.mozilla.org/integration/mozilla-inbound/rev/9a5191dfae8d
http://hg.mozilla.org/integration/mozilla-inbound/rev/ca391c7bceb8

Comment 4

[David Anderson [:dvander] - inactive, e-mail if emergency]

(In reply to Sean Stangl from comment #3)
> (In reply to Luke Wagner [:luke] from comment #1)
> > Which patch was it?
> 
> The only patches in the regressing range are from Bug 781657 and Bug 788822:
> 
> http://hg.mozilla.org/integration/mozilla-inbound/rev/9a5191dfae8d
> http://hg.mozilla.org/integration/mozilla-inbound/rev/ca391c7bceb8

Thanks, it was in fact the first of these changesets. Bill, I believe this patch is desired for GC? Since we don't Ion-compile much of Emscripten yet, maybe we should take some short-term steps to fix that instead of backing this out.

Comment 5

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

Yeah, it would be nice if we didn't have to back that out. It removed a lot of really buggy gunk from stack scanning. I'm not sure what we could do about it, though, besides Ion-compiling the Emscripten code.

Comment 6

[Alon Zakai (:azakai)]

To be clear, I'm not suggesting we back this out. Hopefully we can fix this before the aurora merge, but if not, it sounds like it's worth this regression for the benefits the patch brings.

Comment 7

[Alon Zakai (:azakai)]

I talked to bhackett on irc. He says this regression will remain for now, until the benchmarks no longer depend on JM (i.e. we can run more of them in ion) or we remove JM entirely.

Comment 8

[Guilherme Lima]

Per comment 7, fixed due to JM removal.