Summary: | Load constants all at once instead of using op_load | ||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Cameron Zwarich (cpst) <zwarich> | ||||||||||
Component: | JavaScriptCore | Assignee: | Cameron Zwarich (cpst) <zwarich> | ||||||||||
Status: | RESOLVED FIXED | ||||||||||||
Severity: | Normal | CC: | mjs, oliver | ||||||||||
Priority: | P2 | ||||||||||||
Version: | 528+ (Nightly build) | ||||||||||||
Hardware: | All | ||||||||||||
OS: | All | ||||||||||||
Attachments: |
|
Description
Cameron Zwarich (cpst)
2008-08-05 02:22:02 PDT
Created attachment 22650 [details]
Preliminary patch
Here is a preliminary patch. It passes all tests but fast/js/deep-recursion-test (I need to change the limit due to larger stack frames). I also need to fix the debugging output, but maybe I will only do a bit in this patch and then do more in a later patch.
Created attachment 22651 [details] SunSpider numbers with r35554 Created attachment 22652 [details] SunSpider numbers with r35555 The small change in r35555 somehow causes my patch to go from being a 1.8% speedup to a 0.5% speedup (or a slowdown when I originally tested it). I don't have any clue why. Since r35555 was rolled out, this patch is a 1.4% improvement on SunSpider, but still not a 1.8% improvement like it was before. The code is identical, so I will chalk this discrepancy up to randomness. Created attachment 22672 [details]
Proposed patch
Here's the patch. Here are a few comments:
1) I don't like that I have the same code
m_codeBlock->numConstants = programNode->numConstants() + 3;
in each of the CodeGenerator constructors, without any explanation of what it does. Should I have a comment explaining why we possibly need 3 more registers in each spot, or write it once and refer to it twice?
2) The way that I make some RegisterIDs in the temporary range return false for isTemporary() seems like a bad idea, but since there is no copy constructor, the only other way I can do it is by inheriting. What should I do here?
3) I changed CodeBlock::registers to be CodeBlock::constants, but perhaps the name constantRegisters is better. What do you think?
(In reply to comment #5) > Created an attachment (id=22672) [edit] > Proposed patch > > Here's the patch. Here are a few comments: > > 1) I don't like that I have the same code > > m_codeBlock->numConstants = programNode->numConstants() + 3; > > in each of the CodeGenerator constructors, without any explanation of what it > does. Should I have a comment explaining why we possibly need 3 more registers > in each spot, or write it once and refer to it twice? I'd advise a method on programNode that adds 3 and explains why. > > 2) The way that I make some RegisterIDs in the temporary range return false for > isTemporary() seems like a bad idea, but since there is no copy constructor, > the only other way I can do it is by inheriting. What should I do here? I think this is ok as-is for a first cut. Maybe Geoff has a better idea. > 3) I changed CodeBlock::registers to be CodeBlock::constants, but perhaps the > name constantRegisters is better. What do you think? I like that name. Also, please test if maybe open-coding the copy loop is faster than memcpy. r=me after addressing this. |