Bug 20286 – Load constants all at once instead of using op_load

• Status: RESOLVED
• Resolution: FIXED
• Component: JavaScriptCore

[Cameron Zwarich (cpst)]

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.

[Cameron Zwarich (cpst)]

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.

[Cameron Zwarich (cpst)]

Created attachment 22651 [details]
SunSpider numbers with r35554

[Cameron Zwarich (cpst)]

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.

[Cameron Zwarich (cpst)]

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.

[Cameron Zwarich (cpst)]

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?

[Maciej Stachowiak]

(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.

[Cameron Zwarich (cpst)]

Landed in r35593, using an open-coded copy instead of memcpy() for a 2.6% speedup.