JSArray::shiftCountWith* could be more efficient

• Component: JavaScriptCore
• Status: RESOLVED FIXED

[Mark Hahnenberg]

Our current implementations of shiftCountWithAnyIndexingType and shiftCountWithArrayStorage are scared of the presence of any holes in the array. We can mitigate this somewhat by enabling them to correctly handle holes, thus avoiding the slowest of slow paths in most cases.

[Mark Hahnenberg]

Created attachment 231592 [details]
Patch

[Geoffrey Garen]

Comment on attachment 231592 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=231592&action=review

r=me

> Source/JavaScriptCore/ChangeLog:10
> +        them to correctly handle holes, thus avoiding the slowest of slow paths in most cases.

What is the speedup?

Do we need a js/regress test?

> Source/JavaScriptCore/runtime/JSArray.cpp:680
> +        || inSparseIndexingMode() 
> +        || shouldUseSlowPut(indexingType())) {

I would move these checks into canUseFastShift, or rename canUseFastShift to something like mightInterceptIndexedAccess.

> Source/JavaScriptCore/runtime/JSArray.cpp:1700
> +bool JSArray::canUseFastShift(VM& vm) const

Alternatively, maybe this function should go next to Structure::anyObjectInChainMayInterceptIndexedAccesses in Structure.

[Filip Pizlo]

Comment on attachment 231592 [details]
Patch

r=me too

[Mark Hahnenberg]

Created attachment 231609 [details]
Patch

[Mark Hahnenberg]

(In reply to comment #4)
> Created an attachment (id=231609) [details]
> Patch

Reuploaded with new test and refactored JSArray::canUseFastShift into Structure::holesRequireSpecialBehavior.

[Mark Hahnenberg]

Comment on attachment 231609 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=231609&action=review

> Source/JavaScriptCore/runtime/JSArray.cpp:678
> +    if ((oldLength != storage->m_numValuesInVector && this->structure(vm)->holesRequireSpecialBehavior(vm)) 

Use storage->hasHoles()

[Mark Hahnenberg]

Created attachment 231611 [details]
Patch

[Geoffrey Garen]

Comment on attachment 231611 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=231611&action=review

> Source/JavaScriptCore/runtime/JSArray.cpp:789
> +        if (this->structure(vm)->holesRequireSpecialBehavior(vm)) {

Are you missing a ! here?

[Mark Hahnenberg]

Created attachment 231729 [details]
Patch

[Mark Hahnenberg]

Comment on attachment 231729 [details]
Patch

Forgot to make all the changes for doubles.

[Mark Hahnenberg]

Created attachment 231730 [details]
Patch

[Mark Hahnenberg]

(In reply to comment #11)
> Created an attachment (id=231730) [details]
> Patch

Found some bugs, added a bunch of tests for those particular bugs.

[Geoffrey Garen]

Comment on attachment 231730 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=231730&action=review

r=me

> Source/JavaScriptCore/runtime/Structure.cpp:390
> +bool Structure::holesRequireSpecialBehavior(VM& vm) const

Maybe we can be more specific and call this something like "holesMustForwardToPrototype".

[Mark Hahnenberg]

Committed r169121: <http://trac.webkit.org/changeset/169121>