Bug 19180 – speed up SunSpider by optimizing immediate number cases

• Component: JavaScriptCore
• Status: RESOLVED FIXED

[Darin Adler]

I want to add immediate number special cases to hot opcodes. It seems to be working.

[Darin Adler]

Created attachment 21285 [details]
patch for < operator

[Maciej Stachowiak]

Comment on attachment 21285 [details]
patch for < operator

r=me

[Darin Adler]

Comment on attachment 21285 [details]
patch for < operator

Clearing since I landed this.

[Darin Adler]

Created attachment 21348 [details]
patch for &, |, and ^ operators

[Alexey Proskuryakov]

I'm going to see if jsAdd can be similarly optimized - about 60% of number additions in SunSpider are for immediates.

[Darin Adler]

(In reply to comment #5)
> I'm going to see if jsAdd can be similarly optimized - about 60% of number
> additions in SunSpider are for immediates.

For what it's worth, I tried that and failed.

[Darin Adler]

Comment on attachment 21348 [details]
patch for &, |, and ^ operators

Clear flag since I landed this patch.

[Alexey Proskuryakov]

Created attachment 21363 [details]
Patch for + and -

Hmm, makes me wonder if I got my bit operations wrong. This passes JSC tests, still rebuilding to run layout tests.

Oliver has a similar patch for ++, which we discussed a little yesterday.

[Darin Adler]

OK. My original patch did ++, --, + and -, but there was a net slowdown for all of them.

[Darin Adler]

Comment on attachment 21363 [details]
Patch for + and -

r=me

[Alexey Proskuryakov]

So, I tried to land my patch, and re-measured performance with ToT, just in case. And it has turned into a 6% regression!

I've triple-checked the numbers, and it is just that: your patch is a 1% win, mine is 1.8% win, but together, they become a 6% regression.

[Darin Adler]

(In reply to comment #11)
> I've triple-checked the numbers, and it is just that: your patch is a 1% win,
> mine is 1.8% win, but together, they become a 6% regression.

Which specific patch do you mean when you say "your patch"?

[Alexey Proskuryakov]

I was talking about the latest one, which I didn't have in my tree when measuring originally, "patch for &, |, and ^ operators".

[Alexey Proskuryakov]

Comment on attachment 21363 [details]
Patch for + and -

Landed this patch, it was a 2% improvement after a slight random change that should have been a slowdown - I've moved VM_CHECK_EXCEPTION() out of else block. Applying the same change to bitops regresses performance, though.

On disassembly, I saw a lot of changes everywhere in Machine::executePrivate, but I can't tell what gcc was thinking. There are also changes in completely unrelated places, such as base64 encoding and registerThread().

[Alexey Proskuryakov]

Created attachment 21384 [details]
Patch for ++ and --

[Maciej Stachowiak]

Comment on attachment 21384 [details]
Patch for ++ and --

r=me

[Alexey Proskuryakov]

Comment on attachment 21384 [details]
Patch for ++ and --

Clearing since I landed this.

[Alexey Proskuryakov]

From the looks of it, right shift should also benefit from such optimization, but I cannot get it to work yet. I do not see any other cases to optimize in this manner.

[Alexey Proskuryakov]

Created attachment 21395 [details]
Patch for >>, >>>, ==, ===, !=, !==

[Alexey Proskuryakov]

Comment on attachment 21395 [details]
Patch for >>, >>>, ==, ===, !=, !==

Oops, this is buggy...

[Darin Adler]

Comment on attachment 21395 [details]
Patch for >>, >>>, ==, ===, !=, !==

 124         return reinterpret_cast<JSValue*>((reinterpret_cast<intptr_t>(val) >> ((reinterpret_cast<uintptr_t>(shift) >> 2) & 0x1f)) | NumberType);

Does this do the correct thing on numbers that have low bits set? What about a 1 shifted right by 1?

[Alexey Proskuryakov]

Created attachment 21398 [details]
Patch for >>, ==, ===, !=, !==

Removed the fast path for >>> for now, as it was quite incorrect for negative numbers, and fixing this results in rather mixed performance changes.

(In reply to comment #21)
> Does this do the correct thing on numbers that have low bits set? 

As far as I can tell, it does.

> What about a 1 shifted right by 1?

That's (7 >> (7 >> 2)) | 3 == (7 >> 1) | 3 == 3, which is correct.

[Alexey Proskuryakov]

Created attachment 21425 [details]
Patch for >>, ==, ===, !=, !==

Renamed v1/v2 to src1/src2, to make code match documentation.

I just realized that there is the same issue on PPC with << - I'll see if a fix for it can be combined with an optimization again, or will file a separate bug.

[Oliver Hunt]

Comment on attachment 21425 [details]
Patch for >>, ==, ===, !=, !==

r=me, assuming layout and jsc tests pass.  And assuming you tested perf on current ToT :D

[Alexey Proskuryakov]

Comment on attachment 21425 [details]
Patch for >>, ==, ===, !=, !==

Clearing, because I landed this.

(In reply to comment #24)
> And assuming you tested perf on current ToT :D

It was a larger speedup today - because r34204 was a random regression, and this patch brings absolute numbers back to what I saw before.

[Alexey Proskuryakov]

Created attachment 21430 [details]
Patch for << and >>>

Randomness strikes again, but a win overall.

Many of these above cases can be further improved by using inline assembly - it is faster to try an arithmetic operation and check carry and overflow flags than to check if it can be run.

[Darin Adler]

Comment on attachment 21430 [details]
Patch for << and >>>

If you fixed a regression on PowerPC, was there a regression test indicating it?

+        (KJS::JSImmediate::toTruncatedUInt32): Added. Same as getTruncatedInt32, but casts the result
+        to unsigned.

I'm concerned about the "to" vs. "get" in the name here. This class seems we use "get" to mean "convert with no possibility of error" and "to" to mean "convert with a possible exception or error".

r=me

[Alexey Proskuryakov]

Comment on attachment 21430 [details]
Patch for << and >>>

Clearing review flag, since I landed this.

(In reply to comment #27)
> If you fixed a regression on PowerPC, was there a regression test indicating
> it?

Yes, it's one of the three Mozilla tests that I claimed to have fixed in the previous patch. Only two were fixed, in fact.

> I'm concerned about the "to" vs. "get" in the name here. This class seems we
> use "get" to mean "convert with no possibility of error" and "to" to mean
> "convert with a possible exception or error".

I'm rather confused about the naming in JSImmediate.h - some "get" functions also return a bool to indicate success. That said, this new function just implements "ToUInt32" algorithm from ECMA-232 for immediates as far as I can tell, and is not really a "get" in the sense that -1 turns into 4294967296. The "get" functions that we have do not cast the result like that. I wasn't sure what the word "truncated" meant here, but since this function does almost the same work as getTruncatedInt32, I kept it.

[Darin Adler]

(In reply to comment #28)
> I'm rather confused about the naming in JSImmediate.h - some "get" functions
> also return a bool to indicate success. That said, this new function just
> implements "ToUInt32" algorithm from ECMA-232 for immediates as far as I can
> tell, and is not really a "get" in the sense that -1 turns into 4294967296. The
> "get" functions that we have do not cast the result like that. I wasn't sure
> what the word "truncated" meant here, but since this function does almost the
> same work as getTruncatedInt32, I kept it.

I can live with this. It's worth redoing the names at some point.

The "truncated" is left over from when immediate numbers could be non-integers. "Truncated" meant that the function was free to discard the fractional portion or values that didn't fit in a 32-bit integer.

[Alexey Proskuryakov]

I think I'm done with this for now - do we need to keep the bug open?

[Darin Adler]

Fine to close this if there are no more obvious cases to optimize.

But <= and >= haven't been done yet. Maybe there are others.

[Gavin Barraclough]

The JITs generate immediate arithmetic ops & compares.