Bug 21541 - Move RegisterFile growth check to callee
Summary: Move RegisterFile growth check to callee
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: 20812
  Show dependency treegraph
 
Reported: 2008-10-10 19:30 PDT by Cameron Zwarich (cpst)
Modified: 2008-10-13 17:21 PDT (History)
2 users (show)

See Also:


Attachments
Patch in progress (7.41 KB, patch)
2008-10-10 19:32 PDT, Cameron Zwarich (cpst)
no flags Details | Formatted Diff | Diff
Patch in progress (7.21 KB, patch)
2008-10-10 20:11 PDT, Cameron Zwarich (cpst)
no flags Details | Formatted Diff | Diff
SunSpider harness results (4.36 KB, text/plain)
2008-10-10 20:32 PDT, Cameron Zwarich (cpst)
no flags Details
Proposed patch (10.03 KB, patch)
2008-10-13 14:06 PDT, Cameron Zwarich (cpst)
no flags Details | Formatted Diff | Diff
Proposed patch (10.03 KB, patch)
2008-10-13 14:11 PDT, Cameron Zwarich (cpst)
ggaren: 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-10-10 19:30:57 PDT
We currently do the RegisterFile growth check in the caller, when it should really be in the callee, except in the case where there are too few arguments and you need to fill the unpassed ones with jsUndefined().

If we do this, then we can conceivably inline the "correct number of arguments" case of op_call in the generated machine code. The only thing preventing this is the check for the existence of the CodeBlock on the JSFunction, but this could possibly be done in assembly with a jump to a slow case.

I have a patch to only move the check to the callee that is a wash on V8's benchmark suite and a slight speedup on SunSpider, but I might be able to speed it up a bit more. I will post it.
Comment 1 Cameron Zwarich (cpst) 2008-10-10 19:32:11 PDT
Created attachment 24287 [details]
Patch in progress
Comment 2 Cameron Zwarich (cpst) 2008-10-10 20:11:04 PDT
Created attachment 24288 [details]
Patch in progress

This is a 1.1% progression on SunSpider and a wash on V8. However, it is a significant speedup on DeltaBlue and Earley-Boyer, and only a wash because of a massive (hopefully random) slowdown on Richards.

I still need to implement the exception-handling correctly, and there might be a better way to do the branch to the slow case at the start of a function.
Comment 3 Cameron Zwarich (cpst) 2008-10-10 20:18:17 PDT
Here are the results on the V8 harness. Before:

Richards: 1530
DeltaBlue: 1165
Crypto: 2140
RayTrace: 1439
EarleyBoyer: 2894
----
Score (version 2): 1739

After:

Richards: 1640
DeltaBlue: 1216
Crypto: 2109
RayTrace: 1454
EarleyBoyer: 2939
----
Score (version 2): 1782
Comment 4 Cameron Zwarich (cpst) 2008-10-10 20:32:41 PDT
Created attachment 24289 [details]
SunSpider harness results
Comment 5 Cameron Zwarich (cpst) 2008-10-13 14:06:34 PDT
Created attachment 24324 [details]
Proposed patch
Comment 6 Cameron Zwarich (cpst) 2008-10-13 14:11:33 PDT
Created attachment 24326 [details]
Proposed patch

Oops. I accidentally removed a comment that is useful in cti_op_construct_JSConstruct.
Comment 7 Geoffrey Garen 2008-10-13 17:03:23 PDT
Comment on attachment 24326 [details]
Proposed patch

r=me
Comment 8 Cameron Zwarich (cpst) 2008-10-13 17:21:48 PDT
Landed in r37570.