803884 - IonMonkey: if (!x) slower than if (x == 0)

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Alon Zakai (:azakai)]

Attachment: one.js

Attached are two versions of a primes benchmark compiled by emscripten. diff one.js two.js is

478c478
<       if(c % g == 0) {
---
>       if(!(c % g)) {

So the second version has !x instead of x == 0. The second version is preferable because in general it leads to shorter code (not here actually because we can't remove the parens, but generally speaking it is "==0" vs "!").

But the second version is 12% slower.

Comment 1

[Alon Zakai (:azakai)]

Attachment: two.js

Comment 2

[Ryan VanderMeulen [:RyanVM]]

(In reply to Alon Zakai (:azakai) from comment #0)
> So the second version has !x instead of x == 0.
> But the second version is 12% slower.

The title says that x == 0 is slower?

Comment 3

[Alon Zakai (:azakai)]

Good catch, my mistake :) fixed the title.

Comment 4

[Jan de Mooij [:jandem]]

So at least with IonMonkey the problem is that for x == 0 we use CompareAndBranch and emit

cmpl       $0x0, %eax
jne        <target>

However, for !x we use NotI and TestIAndBranch and emit

cmpl       $0x0, %eax
sete       %al
movzbl     %eax, %eax
testl      %eax, %eax
je         <target>

There's some code in MTest::foldsTo to fold MTest(MNot(x), b1, b2) to MTest(x, b2, b1), but I don't think foldsTo is ever called on control instructions. We could probably also do this during Lowering.

Comment 5

[Marty Rosenberg [:mjrosenb]]

The Compare + Branch -> CompareAndBranch fusing is done during lowering.  Brian and I were talking about this during dinner, and we thing that we need a better interface for fusing instructions together.  A pre-existing algorithm like maximal munch would likely be sufficient for all cases that have come up since we started IonMonkey.

Comment 6

[Sean Stangl [:sstangl]]

Attachment: Fold MControlInstructions.

one.js and two.js now run at exactly the same speed. !! also works as expected.

Comment 7

[Marty Rosenberg [:mjrosenb]]

Comment on attachment 675697 [details] [diff] [review]
Fold MControlInstructions.

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

::: js/src/ion/ValueNumbering.cpp
@@ +87,5 @@
> +        repl->setValueNumberData(new ValueNumberData);
> +
> +    MBasicBlock *block = def->block();
> +
> +    JS_ASSERT(def->useCount() == 0);

A comment saying why we are verifying that this instruction *isn't* used would be useful.

Comment 8

[Sean Stangl [:sstangl]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/829fcbcf7084

Comment 9

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/829fcbcf7084

Comment 10

[Alon Zakai (:azakai)]

two.js is still 5% slower than one.js for me, on latest m-c. Better than 12% from before  but still significant.

Comment 11

[Sean Stangl [:sstangl]]

(In reply to Alon Zakai (:azakai) from comment #10)
> two.js is still 5% slower than one.js for me, on latest m-c. Better than 12%
> from before  but still significant.

Testing locally: on x86, both one.js and two.js resolve in exactly 502ms with little variation. On x86_64, both resolve in exactly 670ms with little variation. After the patch in this bug landed, the code emitted for both cases should be identical. Confirmation in either direction or STR would be helpful.

Comment 12

[Alon Zakai (:azakai)]

All I did was build latest m-c from a few hours ago, then run `time js one.js` and `time js two.js` where js is the new build.

Comment 13

[Alon Zakai (:azakai)]

I am on latest 32-bit ubuntu on a fairly new core i7 (x230 thinkpad).

Comment 14

[Sean Stangl [:sstangl]]

So, I never actually verified this before, but it turns out that IonMonkey never compiles any function in either of the attachments -- every function is found to be excessively large.

Could you run a debug build with IONFLAGS=scripts,aborts and see whether there are any non-abort messages? It's suspicious that landing this patch would improve things.

Comment 15

[Alon Zakai (:azakai)]

I can do a debug build if it's helpful, but note that I didn't check with and without this patch, I checked just now and a month ago. So it's possible something else landed that narrowed the 12% to 5% over the last month (some JM+TI improvement?), and this patch has no effect since as you say nothing is ioned.

Do we have a bug on getting ion to compile bigger scripts?

Comment 16

[Sean Stangl [:sstangl]]

(In reply to Alon Zakai (:azakai) from comment #15)
> Do we have a bug on getting ion to compile bigger scripts?

Not currently. I am investigating the simple solution of bumping the script size limit from 1500 bytes to (ideally) 5000. Chunked compilation was considered to handle arbitrarily-sized scripts, but discarded. The best approach we're pursuing is with parallel compilation, which lets us (almost) not care about the amount of time required to compile.

Comment 17

[Alon Zakai (:azakai)]

Ok, cool. If you file a bug on that script size limit bump please cc me.