Bug 20815 – 'arguments' object creation is non-optimal

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

[Cameron Zwarich (cpst)]

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.

[Cameron Zwarich (cpst)]

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)]

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)]

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)]

Created attachment 23621 [details]
Proposed patch

[Maciej Stachowiak]

Comment on attachment 23621 [details]
Proposed patch

r=me

[Cameron Zwarich (cpst)]

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]

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)]

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