943327 - Octane-mandreel spends a lot of time in ValueNumberer::lookupValue

Status: VERIFIED FIXED

(Core :: JavaScript Engine: JIT, defect)

Description

[Jan de Mooij [:jandem]]

According to Instruments, we spend > 240 ms doing HashMap lookups there. Even though this happens on the background thread, that's a lot of time.

Comment 1

[Jan de Mooij [:jandem]]

OK, this is caused by MNop. Because it has no operands, the HashNumber is the same for every MNop :(

I want to try to stop adding effectful/non-movable instructions to the HashMap, because we won't optimize them anyway.

Comment 2

[Jan de Mooij [:jandem]]

Attachment: Patch

Don't compute/store the hash number if we don't really need it (because no other instruction depends on it and we can't eliminate the instruction anyway). This avoids the collisions from a ton of MNop instructions. The patch also removes the unsued updateForFolding method.

Wins a few thousand points on Octane-mandreel on my machine.

Comment 3

[Jan de Mooij [:jandem]]

(In reply to Jan de Mooij [:jandem] from comment #2)
> Don't compute/store the hash number if

s/hash number/value number/

Comment 4

[Hannes Verschore [:h4writer]]

Comment on attachment 8338491 [details] [diff] [review]
Patch

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

Nice find. (Does this increase mandreel on octane 2.0, octane1.0 or both?)

::: js/src/jit/MIR.h
@@ +514,5 @@
>      bool hasOneDefUse() const;
>  
> +    // Test whether this MDefinition has at least one use.
> +    // (only counting MDefinitions, ignoring MResumePoints)
> +    bool hasDefUse() const;

hasDefUses

::: js/src/jit/ValueNumbering.cpp
@@ +295,5 @@
>      for (ReversePostorderIterator block(graph_.rpoBegin()); block != graph_.rpoEnd(); block++) {
>          for (MDefinitionIterator iter(*block); iter; iter++) {
>              JS_ASSERT(!iter->isInWorklist());
> +            JS_ASSERT(iter->valueNumber() != 0 ||
> +                      (!iter->hasDefUse() && (!iter->isMovable() || iter->isEffectful())));

I would change this to:
JS_ASSERT_IF(iter->valueNumber() == 0, !iter->hasDefUses() && ...);

Comment 5

[Jan de Mooij [:jandem]]

(In reply to Hannes Verschore [:h4writer] from comment #4)
> (Does this increase mandreel on octane 2.0, octane1.0 or both?)

Both..

https://hg.mozilla.org/integration/mozilla-inbound/rev/d2c15d1acb1d

Comment 6

[Guilherme Lima]

Great improvement, more on x64 than on x86. It also improved some Asm.js tests on Firefox -no-asmjs!

Comment 7

[Hannes Verschore [:h4writer]]

Before bug 930048 happened:
>           wo patch   w patch
> Mandreel: 25725      25406
> Mandreel: 25651      25676
> Mandreel: 25750      25725

After bug 930048 landed:

>           wo patch   w patch
> Mandreel: 23658      25430
> Mandreel: 23658      25528
> Mandreel: 23265      25430

In other words. I think this fixes almost fully the regression we saw. Since 930048 landed in what now aurora is, we should definitely backport this fix.

Comment 8

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

https://hg.mozilla.org/mozilla-central/rev/d2c15d1acb1d

Comment 9

[Jan de Mooij [:jandem]]

Comment on attachment 8338491 [details] [diff] [review]
Patch

[Approval Request Comment]
> Bug caused by (feature/regressing bug #): 
Bug 930048 (see also bug 932769).

> User impact if declined:
Regression on a major JS benchmark (Octane) and probably Mandreel code. On Octane-mandreel, this patch was > a 20% win (according to AWFY x86) and fixes the regression introduced by bug 930048.

> Testing completed (on m-c, etc.):
On m-c for a few days.

> Risk to taking this patch (and alternatives if risky): 
Low risk.

> String or IDL/UUID changes made by this patch:
None.

Comment 10

[bhavana bajaj [:bajaj]]

(In reply to Jan de Mooij [:jandem] from comment #9)
> Comment on attachment 8338491 [details] [diff] [review]
> Patch
> 
> [Approval Request Comment]
> > Bug caused by (feature/regressing bug #): 
> Bug 930048 (see also bug 932769).
> 
> > User impact if declined:
> Regression on a major JS benchmark (Octane) and probably Mandreel code. On
> Octane-mandreel, this patch was > a 20% win (according to AWFY x86) and
> fixes the regression introduced by bug 930048.
> 
> > Testing completed (on m-c, etc.):
> On m-c for a few days.
> 
> > Risk to taking this patch (and alternatives if risky): 
> Low risk.
> 
> > String or IDL/UUID changes made by this patch:
> None.

Looks like a Fx28 regression, unclear why an uplift to aurora is needed here. Bug 930048, Bug 932769 seem to have landed in Fx28, am i missing something ?

Comment 11

[Jan de Mooij [:jandem]]

Comment on attachment 8338491 [details] [diff] [review]
Patch

(In reply to bhavana bajaj [:bajaj] from comment #10)
> Looks like a Fx28 regression, unclear why an uplift to aurora is needed
> here. Bug 930048, Bug 932769 seem to have landed in Fx28, am i missing
> something ?

You're right! Sorry I should have noticed this. Resetting approval flag :)

Comment 12

[Ioana (away)]

Octane-mandreel looks fine on http://arewefastyet.com/ now. If you need this verified more thorougly, please point out what tool should be used.