Last Comment Bug 642957 - TI+JM: remove x|0 and x << 0 optimization
: TI+JM: remove x|0 and x << 0 optimization
Status: RESOLVED FIXED
:
Product: Core
Classification: Components
Component: JavaScript Engine (show other bugs)
: unspecified
: All All
: -- normal with 1 vote (vote)
: mozilla10
Assigned To: Marco Castelluccio [:marco]
:
: Jason Orendorff [:jorendorff]
Mentors:
Depends on:
Blocks: TypeInference
  Show dependency treegraph
 
Reported: 2011-03-18 13:02 PDT by Jan de Mooij [:jandem]
Modified: 2011-10-25 04:55 PDT (History)
14 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
Remove optimization (1.67 KB, patch)
2011-10-19 10:07 PDT, Marco Castelluccio [:marco]
no flags Details | Diff | Splinter Review
ubench results (1.22 KB, text/plain)
2011-10-20 11:38 PDT, Marco Castelluccio [:marco]
no flags Details
Remove optimization v2 (2.43 KB, patch)
2011-10-21 16:18 PDT, Marco Castelluccio [:marco]
jdemooij: review+
Details | Diff | Splinter Review

Description Jan de Mooij [:jandem] 2011-03-18 13:02:44 PDT
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 2011-03-30 13:32:19 PDT
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] 2011-10-19 10:07:55 PDT
Created attachment 568105 [details] [diff] [review]
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] 2011-10-19 10:31:32 PDT
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] 2011-10-19 12:12:16 PDT
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 [:bz] (still a bit busy) 2011-10-19 12:30:36 PDT
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] 2011-10-20 11:38:23 PDT
Created attachment 568454 [details]
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] 2011-10-21 06:55:02 PDT
(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] 2011-10-21 16:18:40 PDT
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 9 Jan de Mooij [:jandem] 2011-10-24 10:15:15 PDT
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] 2011-10-25 04:55:05 PDT
https://hg.mozilla.org/mozilla-central/rev/a955bd83e9df

Note You need to log in before you can comment on or make changes to this bug.