642957 - TI+JM: remove x|0 and x << 0 optimization

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Jan de Mooij [:jandem]]

In bug 592631 I added a fast-path for double|0 and double << 0. This was a nice optimization for Firefox 4 but AFAIK the TI branch now inlines double-to-int conversion for bitwise operators. This makes the fast path redundant. We should remove it and verify the benchmark in bug 592631 comment 0 does not regress.

Comment 1

[Grant Galitz]

Jan de Mooij: I should probably see if any changes to this regress the audio sample aliasing in some code of mine.

Comment 2

[Marco Castelluccio [:marco]]

Attachment: Remove optimization

I've tested the benchmark from bug 592631 (within the web console), and it didn't regressed (actually it's better by some ms).
However with Chromium I get better results (in Firefox without the patch 899 ms, with the patch 894 ms, in Chromium 37 ms).

Comment 3

[Marco Castelluccio [:marco]]

I've tested a bit more with the benchmark, the results without the patch and with the patch are more or less the same (in a small range of more or less 10 ms).

Grant Galitz, could you test the patch with your code? I'd like to be sure there isn't any regression.

Comment 4

[Jan de Mooij [:jandem]]

Hi Marco, I totally forgot about this bug, thanks for taking it!

However, these performance numbers don't look right. We should be about as fast as Chrome here, probably even a bit faster with TI. Apparently the web console is doing something weird or JM is disabled somehow. For JS patches like this we don't usually build the whole browser, instead we use the command-line JS shell. For more information see https://wiki.mozilla.org/JavaScript:New_to_SpiderMonkey It also explains how you can run the JS shell tests.

When testing your patch, you should pass the -m (enable JM) and -n (enable TI) flags to the shell, like this:

$ ./js -m -n test.js

The patch looks great though so the only thing left here is running jit-test (debug build) and testing the micro-benchmark (release build).

Comment 5

[Boris Zbarsky [:bzbarsky]]

The web console is running in a sandboxed environment with various stuff going through proxies, so running any sort of performance tests in it is useless.  But yes, looks like the sandbox also doesn't enable the jit.

Oh, and we're a bit slower than Chrome with TI here, comparing our 64-bit apples to their 32-bit oranges.

Comment 6

[Marco Castelluccio [:marco]]

Attachment: ubench results

(In reply to Jan de Mooij from comment #4)
> The patch looks great though so the only thing left here is running jit-test
> (debug build) and testing the micro-benchmark (release build).

Thank you for the informations Jan. This is the first bug for me in the JavaScript Engine, so I needed them :)

(In reply to Boris Zbarsky (:bz) from comment #5)
> The web console is running in a sandboxed environment with various stuff
> going through proxies, so running any sort of performance tests in it is
> useless.  But yes, looks like the sandbox also doesn't enable the jit.
> 
> Oh, and we're a bit slower than Chrome with TI here, comparing our 64-bit
> apples to their 32-bit oranges.

Thank you Boris, I've specified I was testing with web console because I needed your feedback (also if I suspected the web console isn't a good testing environment).

Now I've done the tests with the js shell, and the results are exactly the same with and without the patch:
./js -m    : 147 ms
./js -m -n : 42 ms

In the jit-test there is a failure, that however happens with or without the patch:
FAILURES:
    -m -j -p sunspider/check-date-format-tofte.js

There are also three regressions in the ref-tests, that are present also without the patch:
REGRESSIONS
    ecma_3/Date/15.9.5.6.js
    js1_5/extensions/toLocaleFormat-02.js
    js1_5/extensions/toLocaleFormat-01.js

SunSpider results are the same.
Instead ubench results seem to have regressed a bit (I tested with 'sunspider --args="-m -n" ...'). Results are attached.

Comment 7

[Jan de Mooij [:jandem]]

(In reply to Marco Castelluccio from comment #6)
> In the jit-test there is a failure, that however happens with or without the
> patch:
> FAILURES:
>     -m -j -p sunspider/check-date-format-tofte.js

That's bug 600522, the test depends on the time zone. I get the same jit-test/reftest failures, it's annoying...

> Instead ubench results seem to have regressed a bit (I tested with
> 'sunspider --args="-m -n" ...'). Results are attached.

None of the ubench tests use bitwise operators, so it's probably safe to assume this is noise.

It looks like we will now generate an extra "or 0, reg" instruction though. You can get rid of it by changing

else
    masm.or32(Imm32(rhsInt), reg);

to this:

else if (rhsInt != 0)
    masm.or32(Imm32(rhsInt), reg);

We already do something similar for <<. Can you update the patch with this change? Thanks!

Comment 8

[Marco Castelluccio [:marco]]

Attachment: Remove optimization v2

I've done the change.
Now the ubench results are better with the patch than without, probably as you said it's noise :)

Comment 9

[Jan de Mooij [:jandem]]

Comment on attachment 568816 [details] [diff] [review]
Remove optimization v2

Thanks! Pushed to inbound:

http://hg.mozilla.org/integration/mozilla-inbound/rev/a955bd83e9df

Comment 10

[Marco Bonardo [:mak]]

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