Bug 20286 - Load constants all at once instead of using op_load
Summary: Load constants all at once instead of using op_load
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: JavaScriptCore (show other bugs)
Version: 528+ (Nightly build)
Hardware: All All
: P2 Normal
Assignee: Cameron Zwarich (cpst)
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2008-08-05 02:22 PDT by Cameron Zwarich (cpst)
Modified: 2008-08-06 03:38 PDT (History)
2 users (show)

See Also:


Attachments
Preliminary patch (148.30 KB, patch)
2008-08-05 02:50 PDT, Cameron Zwarich (cpst)
no flags Details | Formatted Diff | Diff
SunSpider numbers with r35554 (3.56 KB, text/plain)
2008-08-05 03:33 PDT, Cameron Zwarich (cpst)
no flags Details
SunSpider numbers with r35555 (3.59 KB, text/plain)
2008-08-05 03:34 PDT, Cameron Zwarich (cpst)
no flags Details
Proposed patch (153.05 KB, patch)
2008-08-05 20:06 PDT, Cameron Zwarich (cpst)
mjs: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Cameron Zwarich (cpst) 2008-08-05 02:22:02 PDT
Maciej and Geoff suggested loading constants all at once instead of using op_load individually. I have a patch that implements this, and I will post it in this bug, along with SunSpider results.
Comment 1 Cameron Zwarich (cpst) 2008-08-05 02:50:51 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.
Comment 2 Cameron Zwarich (cpst) 2008-08-05 03:33:52 PDT
Created attachment 22651 [details]
SunSpider numbers with r35554
Comment 3 Cameron Zwarich (cpst) 2008-08-05 03:34:59 PDT
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.
Comment 4 Cameron Zwarich (cpst) 2008-08-05 04:40:29 PDT
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.
Comment 5 Cameron Zwarich (cpst) 2008-08-05 20:06:02 PDT
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?
Comment 6 Maciej Stachowiak 2008-08-06 02:13:54 PDT
(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.
Comment 7 Cameron Zwarich (cpst) 2008-08-06 03:38:53 PDT
Landed in r35593, using an open-coded copy instead of memcpy() for a 2.6% speedup.