Bug 133011 - JSArray::shiftCountWith* could be more efficient
Summary: JSArray::shiftCountWith* could be more efficient
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: JavaScriptCore (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Mark Hahnenberg
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2014-05-16 14:35 PDT by Mark Hahnenberg
Modified: 2014-05-20 11:08 PDT (History)
0 users

See Also:


Attachments
Patch (11.02 KB, patch)
2014-05-16 14:37 PDT, Mark Hahnenberg
no flags Details | Formatted Diff | Diff
Patch (17.55 KB, patch)
2014-05-16 18:29 PDT, Mark Hahnenberg
no flags Details | Formatted Diff | Diff
Patch (18.00 KB, patch)
2014-05-16 18:43 PDT, Mark Hahnenberg
no flags Details | Formatted Diff | Diff
Patch (33.87 KB, patch)
2014-05-19 15:51 PDT, Mark Hahnenberg
no flags Details | Formatted Diff | Diff
Patch (34.69 KB, patch)
2014-05-19 15:57 PDT, Mark Hahnenberg
ggaren: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Mark Hahnenberg 2014-05-16 14:35:35 PDT
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.
Comment 1 Mark Hahnenberg 2014-05-16 14:37:07 PDT
Created attachment 231592 [details]
Patch
Comment 2 Geoffrey Garen 2014-05-16 14:58:59 PDT
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.
Comment 3 Filip Pizlo 2014-05-16 15:00:48 PDT
Comment on attachment 231592 [details]
Patch

r=me too
Comment 4 Mark Hahnenberg 2014-05-16 18:29:02 PDT
Created attachment 231609 [details]
Patch
Comment 5 Mark Hahnenberg 2014-05-16 18:29:38 PDT
(In reply to comment #4)
> Created an attachment (id=231609) [details]
> Patch

Reuploaded with new test and refactored JSArray::canUseFastShift into Structure::holesRequireSpecialBehavior.
Comment 6 Mark Hahnenberg 2014-05-16 18:32:53 PDT
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()
Comment 7 Mark Hahnenberg 2014-05-16 18:43:51 PDT
Created attachment 231611 [details]
Patch
Comment 8 Geoffrey Garen 2014-05-16 19:09:49 PDT
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?
Comment 9 Mark Hahnenberg 2014-05-19 15:51:15 PDT
Created attachment 231729 [details]
Patch
Comment 10 Mark Hahnenberg 2014-05-19 15:52:41 PDT
Comment on attachment 231729 [details]
Patch

Forgot to make all the changes for doubles.
Comment 11 Mark Hahnenberg 2014-05-19 15:57:57 PDT
Created attachment 231730 [details]
Patch
Comment 12 Mark Hahnenberg 2014-05-19 15:59:29 PDT
(In reply to comment #11)
> Created an attachment (id=231730) [details]
> Patch

Found some bugs, added a bunch of tests for those particular bugs.
Comment 13 Geoffrey Garen 2014-05-19 19:28:25 PDT
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".
Comment 14 Mark Hahnenberg 2014-05-20 11:08:21 PDT
Committed r169121: <http://trac.webkit.org/changeset/169121>