Bug 21099 – Temporary JSNumberCells are not reused.

• Component: JavaScriptCore
• Status: RESOLVED FIXED

[Gavin Barraclough]

As a result we constantly allocate JSNumberCells to hold temporary result – would be better if we did not do so.

[Gavin Barraclough]

Created attachment 23791 [details]
The patch

[Gavin Barraclough]

Created attachment 23792 [details]
Sunspider results

[Gavin Barraclough]

There is a couple of ms regression on the bit-ops tests.

This kind of code should also be able to benefit greatly from these techniques (reusing the number cell when an operand has not fitted into a JSImmediate), but this is not yet implemented.  I'd be keen to push this patch forwards as-is, given the progression, and to raise a separate patch to improve the bit-ops tests.

[Gavin Barraclough]

Created attachment 23793 [details]
Slight grammar fix. :-)

[Gavin Barraclough]

Created attachment 23800 [details]
Fix for bdash's review comments from irc.

[Mark Rowe (bdash)]

I think we need some sort of runtime check for SSE, since Windows and Linux run on CPUs that may not support it.

[Gavin Barraclough]

Created attachment 23818 [details]
Some sort of runtime check for SSE, since Windows and Linux run on CPUs that may not support it.

[Maciej Stachowiak]

Comment on attachment 23818 [details]
Some sort of runtime check for SSE, since Windows and Linux run on CPUs that may not support it.

I mentioned some style comments on IRC.

There does appear to be one substantive bug, which is that sometimes a cell will be reused when the result should have been packed in an immediate. For instance, 2 * 0.5 will be left in a cell. Various parts of our engine assume that immediate-sized values can't be in a cell, including switch and ===.

So for example with this patch, 1 === 0.5 + 0.5 would return false.

[Geoffrey Garen]

+#else
+bool isSSE3Present()
+{
+    return false;
+}
+#endif

Do we need another bug about supporting SSE3 on non-Mac platforms?

[Gavin Barraclough]

Created attachment 23863 [details]
New Patch

Similar performance to previous patch.

Geoff: Yes – I should raise a bug for that, and also one to check the -0 safety of the current op_mul / op_div code.  I'll raise new bugs as I land this.

[Gavin Barraclough]

Bah, I should have put a better description on that patch, integer results in range are now stored as JSImmediates rather than JSNumberCells.

[Oliver Hunt]

Comment on attachment 23863 [details]
New Patch

Okay, maciej's okay with this, and the asm looks correct to me.  The rest of the code looks good to me as well, but maciej basically wanted me to look at the asm so that's what i focused on.

r=me and maciej i believe

[Gavin Barraclough]

Sending        JavaScriptCore/ChangeLog
Sending        JavaScriptCore/JavaScriptCore.xcodeproj/project.pbxproj
Sending        JavaScriptCore/VM/CTI.cpp
Sending        JavaScriptCore/VM/CTI.h
Sending        JavaScriptCore/VM/CodeBlock.cpp
Sending        JavaScriptCore/VM/CodeGenerator.cpp
Sending        JavaScriptCore/VM/CodeGenerator.h
Sending        JavaScriptCore/VM/Machine.cpp
Sending        JavaScriptCore/kjs/JSNumberCell.h
Adding         JavaScriptCore/kjs/ResultType.h
Sending        JavaScriptCore/kjs/nodes.cpp
Sending        JavaScriptCore/kjs/nodes.h
Sending        JavaScriptCore/masm/X86Assembler.h
Transmitting file data .............
Committed revision 36976.

[Gavin Barraclough]

Follow on bugs:

https://bugs.webkit.org/show_bug.cgi?id=21162
https://bugs.webkit.org/show_bug.cgi?id=21160