Bug 21099 - Temporary JSNumberCells are not reused.
Summary: Temporary JSNumberCells are not reused.
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: JavaScriptCore (show other bugs)
Version: 528+ (Nightly build)
Hardware: Mac OS X 10.5
: P2 Normal
Assignee: Gavin Barraclough
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2008-09-25 07:02 PDT by Gavin Barraclough
Modified: 2008-09-26 19:25 PDT (History)
0 users

See Also:


Attachments
The patch (70.64 KB, patch)
2008-09-25 07:02 PDT, Gavin Barraclough
no flags Details | Formatted Diff | Diff
Sunspider results (5.68 KB, text/plain)
2008-09-25 07:07 PDT, Gavin Barraclough
no flags Details
Slight grammar fix. :-) (70.65 KB, patch)
2008-09-25 07:17 PDT, Gavin Barraclough
no flags Details | Formatted Diff | Diff
Fix for bdash's review comments from irc. (70.71 KB, patch)
2008-09-25 09:55 PDT, Gavin Barraclough
no flags Details | Formatted Diff | Diff
Some sort of runtime check for SSE, since Windows and Linux run on CPUs that may not support it. (71.40 KB, patch)
2008-09-25 13:27 PDT, Gavin Barraclough
mjs: review-
Details | Formatted Diff | Diff
New Patch (77.39 KB, patch)
2008-09-26 17:56 PDT, Gavin Barraclough
oliver: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Gavin Barraclough 2008-09-25 07:02:01 PDT
As a result we constantly allocate JSNumberCells to hold temporary result – would be better if we did not do so.
Comment 1 Gavin Barraclough 2008-09-25 07:02:33 PDT
Created attachment 23791 [details]
The patch
Comment 2 Gavin Barraclough 2008-09-25 07:07:33 PDT
Created attachment 23792 [details]
Sunspider results
Comment 3 Gavin Barraclough 2008-09-25 07:13:23 PDT
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.


Comment 4 Gavin Barraclough 2008-09-25 07:17:40 PDT
Created attachment 23793 [details]
Slight grammar fix. :-)
Comment 5 Gavin Barraclough 2008-09-25 09:55:14 PDT
Created attachment 23800 [details]
Fix for bdash's review comments from irc.
Comment 6 Mark Rowe (bdash) 2008-09-25 12:45:06 PDT
I think we need some sort of runtime check for SSE, since Windows and Linux run on CPUs that may not support it.
Comment 7 Gavin Barraclough 2008-09-25 13:27:14 PDT
Created attachment 23818 [details]
Some sort of runtime check for SSE, since Windows and Linux run on CPUs that may not support it.
Comment 8 Maciej Stachowiak 2008-09-25 16:24:23 PDT
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.
Comment 9 Geoffrey Garen 2008-09-25 20:38:23 PDT
+#else
+bool isSSE3Present()
+{
+    return false;
+}
+#endif

Do we need another bug about supporting SSE3 on non-Mac platforms?
Comment 10 Gavin Barraclough 2008-09-26 17:56:17 PDT
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.
Comment 11 Gavin Barraclough 2008-09-26 17:58:07 PDT
Bah, I should have put a better description on that patch, integer results in range are now stored as JSImmediates rather than JSNumberCells.
Comment 12 Oliver Hunt 2008-09-26 18:33:51 PDT
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
Comment 13 Gavin Barraclough 2008-09-26 18:46:54 PDT
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.