Bug 20815

Summary: 'arguments' object creation is non-optimal
Product: WebKit Reporter: Cameron Zwarich (cpst) <zwarich>
Component: JavaScriptCoreAssignee: Cameron Zwarich (cpst) <zwarich>
Status: RESOLVED FIXED    
Severity: Normal CC: mjs
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: All   
OS: All   
Bug Depends on:    
Bug Blocks: 20813    
Attachments:
Description Flags
Patch in progress
none
Proposed patch mjs: review+

Cameron Zwarich (cpst)
Reported 2008-09-12 16:42:36 PDT
The V8 Raytrace benchmark depends quite a bit on the creation of the arguments object. The V8 implementors deliberately deviated from the language spec on this issue, so that writing to the arguments object does not affect the actual function arguments, only other uses of the arguments object. This would allow us to avoid creating an activation for every function that uses the arguments object, so we should consider going with them on this spec deviation.
Attachments
Patch in progress (8.39 KB, patch)
2008-09-17 00:31 PDT, Cameron Zwarich (cpst)
no flags
Proposed patch (30.22 KB, patch)
2008-09-21 01:10 PDT, Cameron Zwarich (cpst)
mjs: review+
Cameron Zwarich (cpst)
Comment 1 2008-09-12 22:22:00 PDT
I'm assigning this to myself. I'll probably just replace the arguments object with a plain array and see what kind of speedup we can get, and then worry about the semantics. I will hold off on eliminating the activation for now.
Cameron Zwarich (cpst)
Comment 2 2008-09-17 00:31:28 PDT
Created attachment 23497 [details] Patch in progress Here is a patch in progress. It passes all tests and works fine, except when run on the V8 tests it crashes in the register array marking portion of JSVariableObject. This seems to suggest that I copied too many registers. When I comment out that code, I get about a 25% speedup on Raytrace. Another 25% or so is available by making it a JSArray (which is unsound, of course) due to the fast path for array access in numerous places. I'll try to fix this so it can land soon, but it may not be tonight.
Cameron Zwarich (cpst)
Comment 3 2008-09-17 03:26:34 PDT
So, it turns out that I misunderstood the calling convention in the case of extra arguments. I was copying the original arguments vector, the call frame, and the locals. The call frame was probably causing problems in the garbage collector. Since the arguments aren't necessarily stored contiguously when there are extra arguments, it is impossible to use array storage naively in the arguments object. It is probably possible to do something involving a separate array for the extra arguments. I will try that tomorrow. I apologize for my misunderstanding, although our calling convention is a bit weird.
Cameron Zwarich (cpst)
Comment 4 2008-09-21 01:10:25 PDT
Created attachment 23621 [details] Proposed patch
Maciej Stachowiak
Comment 5 2008-09-21 01:23:30 PDT
Comment on attachment 23621 [details] Proposed patch r=me
Cameron Zwarich (cpst)
Comment 6 2008-09-21 05:47:44 PDT
Landed in r36735, with a followup in r36736 to correct the fact that the extraArguments data is never freed. I'll close this bug, since the initial problem of fixing our braindead approach to the 'arguments' object was fixed, and open new bugs for individual issues that now show up on Shark.
Darin Adler
Comment 7 2008-09-21 08:39:20 PDT
IndexToNameMap is not a good data structure -- we end up doing an additional malloc/free even though it's just a short vector. And its interface is in terms of identifiers for no good reason -- there's no reason to convert indices to identifiers all the time! I think I can speed things up further by resolving those things.
David Kilzer (:ddkilzer)
Comment 8 2008-09-21 18:53:26 PDT
(In reply to comment #7) > IndexToNameMap is not a good data structure -- we end up doing an additional > malloc/free even though it's just a short vector. And its interface is in terms > of identifiers for no good reason -- there's no reason to convert indices to > identifiers all the time! I think I can speed things up further by resolving > those things. Sounds like Bug 3965.
Note You need to log in before you can comment on or make changes to this bug.