Bug 21541

Summary: Move RegisterFile growth check to callee
Product: WebKit Reporter: Cameron Zwarich (cpst) <zwarich>
Component: JavaScriptCoreAssignee: Cameron Zwarich (cpst) <zwarich>
Status: RESOLVED FIXED    
Severity: Normal CC: barraclough, mjs
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: All   
OS: All   
Bug Depends on:    
Bug Blocks: 20812    
Attachments:
Description Flags
Patch in progress
none
Patch in progress
none
SunSpider harness results
none
Proposed patch
none
Proposed patch ggaren: review+

Cameron Zwarich (cpst)
Reported 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.
Attachments
Patch in progress (7.41 KB, patch)
2008-10-10 19:32 PDT, Cameron Zwarich (cpst) 		no flags
DetailsFormatted DiffDiff
Patch in progress (7.21 KB, patch)
2008-10-10 20:11 PDT, Cameron Zwarich (cpst) 		no flags
DetailsFormatted DiffDiff
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
DetailsFormatted DiffDiff
Proposed patch (10.03 KB, patch)
2008-10-13 14:11 PDT, Cameron Zwarich (cpst) 		ggaren: review+
DetailsFormatted DiffDiff
Show Obsolete (3) View All Add attachment proposed patch, testcase, etc.
Cameron Zwarich (cpst)
Comment 1 2008-10-10 19:32:11 PDT
Created attachment 24287 [details] Patch in progress
Cameron Zwarich (cpst)
Comment 2 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.
Cameron Zwarich (cpst)
Comment 3 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
Cameron Zwarich (cpst)
Comment 4 2008-10-10 20:32:41 PDT
Created attachment 24289 [details] SunSpider harness results
Cameron Zwarich (cpst)
Comment 5 2008-10-13 14:06:34 PDT
Created attachment 24324 [details] Proposed patch
Cameron Zwarich (cpst)
Comment 6 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.
Geoffrey Garen
Comment 7 2008-10-13 17:03:23 PDT
Comment on attachment 24326 [details] Proposed patch r=me
Cameron Zwarich (cpst)
Comment 8 2008-10-13 17:21:48 PDT
Landed in r37570.
Note You need to log in before you can comment on or make changes to this bug.