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.
Jan de Mooij: I should probably see if any changes to this regress the audio sample aliasing in some code of mine.
Created attachment 568105 [details] [diff] [review]
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).
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.
Hi Marco, I totally forgot about this bug, thanks for taking it!
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).
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.
Created attachment 568454 [details]
(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).
(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:
-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:
SunSpider results are the same.
Instead ubench results seem to have regressed a bit (I tested with 'sunspider --args="-m -n" ...'). Results are attached.
(In reply to Marco Castelluccio from comment #6)
> In the jit-test there is a failure, that however happens with or without the
> -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 if (rhsInt != 0)
We already do something similar for <<. Can you update the patch with this change? Thanks!
Created attachment 568816 [details] [diff] [review]
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 on attachment 568816 [details] [diff] [review]
Remove optimization v2
Thanks! Pushed to inbound: