914981 - Fix octane-mandreel regression caused by Float32 landing

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Benjamin Bouvier [:bbouvier] (inactive)]

Attachment: fix-sqrt.patch

TL;DR: Sqrt is not inlined when the input is Float32, while it should. See fix: IsNumberType <=> is double or is integer or is float32.

If you have more time, see how a situation of catastrophic math cache lookups creates this regression.

(In reply to Hannes Verschore [:h4writer] from comment #86 of bug 888109)
> (In reply to Benjamin Bouvier [:bbouvier] from comment #85)
> > https://hg.mozilla.org/integration/mozilla-inbound/rev/a43cf13bd6a6
> 
> This landing caused a regression of 7% on octane-mandreel.

Using perf reports, I can see that we now spend more time in math functions like cos (>2.5%), probably because math cache lookups fail. I confirmed this by deactivating caches on the call to cos on the unpatched version (which doesn't fix the regression but shows that the spent time on the initial unpatched version becomes the same).

It looks like some computations are trying to use Float32 as inputs (I see some inlined calls to cos(x) where x is a Float32 \o/). All math functions calls cannot be specialized for Float32, as of now, but that will get done in a near future.

-- A few hours later...

After one day of debugging, I finally found who the guilty is. Sqrt doesn't get inlined if the argument type isn't Double or Integer. As it sees Float32 flowing in, it doesn't get inlined and thus uses the cached VM call. Bug 913282 contains a patch for sqrt lowering than can specialize if the float32 use is valid. So if we wait for this operator to land, we're good (and it's even actually slightly faster than the previous unpatched shell, as the sqrtf function is faster than sqrt).

Before the sqrt Float32 optimization lands, we can just change the condition of sqrt inlining so that it gets inlined if the input is a Float32 and it's fine. The operation won't get specialized for Float32 until 913282 lands, but it gets inlined for Double and a conversion is inserted.

It also looks like mandreel is alternatively using calls to sqrt(0) and cos(0). As sqrt and cos are not inlined in this version, and as the math cache uses only the input value for hashing (and not which function is called), it creates a situation of catastrophic lookups:
- let's say cos(0) is called the first. cos(0) is not found as it's the first call, and it gets cached in cache[h(0)].
- then sqrt(0) is called. 0 is in the cache, but the attached function is cos, not sqrt. Compute sqrt(0) and put it into cache[h(0)].
- cos(0) is called. 0 is in the cache, but the attached function is sqrt, not cos. Compute cos(0) and put it into cache[h(0)].
- do it again, say 5,000 times.

This only happens as sqrt is not inlined. When sqrt is inlined, this doesn't occur. I will investigate adding the function address as the part of the cache lookup, as it might help other benchmarks too.

Comment 1

[Benjamin Bouvier [:bbouvier] (inactive)]

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

Comment 2

[Ryan VanderMeulen [:RyanVM]]

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