Bug 42857 – Inefficient Handling of Shift and Unshift

• Component: JavaScriptCore
• Status: RESOLVED FIXED

[Michael Saboff]

The array code sometimes re-allocates and always copies data when performing shift and unshift operations.

This defect is related to  <rdar://problem/8086123> 12x slower than Chrome on Dromaeo JS arrays test.

[Michael Saboff]

Created attachment 62360 [details]
Patch file

Patch to improve performance of shift, unshift and special cases of splice and slice.  Also includes some optimizations to array initialization and growth.

[Gavin Barraclough]

Comment on attachment 62360 [details]
Patch file

Please revert unnecessary change to JavaScriptCore/Configurations/JavaScriptCore.xcconfig.

There are a couple of these:
    BaseIndex(regT2, regT1, ScalePtr, 0)
You should just remove the last ",0", this is not necessary.

In JSArray.h, typo: "qucik"
    "return (ArrayStorage *)((char*)" - these should be c++ style casts.

in ArrayPrototype.cpp:
    We have a coding style rule that we don't use abbreviations, unless it is canonically understood.  I don't think the 'nr' in nrArgs matches the style in the codebase well, and I think it falls foul of this rule.  We do commonly use the abbreviations 'args' and 'num' for number.  I'd suggest numArgs, numberOfArgs, argsCount, etc could all be acceptable, but I think 'nr' should change.  I'd also prefer 'index' to 'k' as the loop counter, but single character loop index names clearly has precedent.

Otherwise, all looks great – normally I'd r+, but I'll r- despite the small number of changes since you'll be needing to put up a new patch for the commit-queue anyway.

[Gavin Barraclough]

Hrrrrm.  I missed that this was from existing code.  I don't think nrArgs is great, so personally I'd prefer these were changed – but since it's existing code I won't require this for an r+, I'll leave this up to your judgement.

[Michael Saboff]

Created attachment 62757 [details]
Updated patch

Fixed issues from review.  Did not change variables named nrArgs as this is from prior code.

[Gavin Barraclough]

Transmitting file data ......
Committed revision 64177.

[Gavin Barraclough]

QT ARM/Win builds are running into the following problem:
    "..\..\..\JavaScriptCore\runtime\ArrayPrototype.cpp:41: error: size of array 'dummyclass_fits_in_cell' is negative"

Temporarily reverting this patch (in http://trac.webkit.org/changeset/64184 ) while we work out what our options are here.

[Gavin Barraclough]

reopening

[Gavin Barraclough]

Relanded in revision 64320.