Bug 158769 - Add support for Symbol.isConcatSpreadable (round 2)
Summary: Add support for Symbol.isConcatSpreadable (round 2)
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: New Bugs (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Keith Miller
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2016-06-14 18:07 PDT by Keith Miller
Modified: 2016-06-30 13:25 PDT (History)
7 users (show)

See Also:


Attachments
Patch (102.56 KB, patch)
2016-06-14 19:30 PDT, Keith Miller
no flags Details | Formatted Diff | Diff
JS-benchmarks (78.72 KB, text/plain)
2016-06-14 20:12 PDT, Keith Miller
no flags Details
JS-Bench benchmark (3.00 KB, text/plain)
2016-06-14 20:13 PDT, Keith Miller
no flags Details
Patch (100.48 KB, patch)
2016-06-15 16:59 PDT, Keith Miller
no flags Details | Formatted Diff | Diff
Patch (100.59 KB, patch)
2016-06-15 17:13 PDT, Keith Miller
no flags Details | Formatted Diff | Diff
Patch (100.59 KB, patch)
2016-06-15 17:24 PDT, Keith Miller
no flags Details | Formatted Diff | Diff
Patch for landing (100.58 KB, patch)
2016-06-15 17:34 PDT, Keith Miller
no flags Details | Formatted Diff | Diff
Archive of layout-test-results from ews101 for mac-yosemite (905.13 KB, application/zip)
2016-06-15 18:34 PDT, Build Bot
no flags Details
Archive of layout-test-results from ews125 for ios-simulator-wk2 (654.10 KB, application/zip)
2016-06-15 18:36 PDT, Build Bot
no flags Details
Archive of layout-test-results from ews105 for mac-yosemite-wk2 (1.02 MB, application/zip)
2016-06-15 18:38 PDT, Build Bot
no flags Details
Archive of layout-test-results from ews113 for mac-yosemite (1.65 MB, application/zip)
2016-06-15 18:43 PDT, Build Bot
no flags Details
Patch for landing (101.27 KB, patch)
2016-06-15 19:17 PDT, Keith Miller
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Keith Miller 2016-06-14 18:07:15 PDT
Add support for Symbol.isConcatSpreadable (round 2)
Comment 1 Keith Miller 2016-06-14 19:30:40 PDT
Created attachment 281315 [details]
Patch
Comment 2 Keith Miller 2016-06-14 20:12:58 PDT
Created attachment 281318 [details]
JS-benchmarks
Comment 3 Keith Miller 2016-06-14 20:13:22 PDT
Created attachment 281319 [details]
JS-Bench benchmark
Comment 4 Mark Lam 2016-06-15 11:07:18 PDT
Comment on attachment 281315 [details]
Patch

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

> Source/JavaScriptCore/ChangeLog:9
> +        do so it was necessary to move the Array.prototype.concat function

Please add a comma after "so".

> Source/JavaScriptCore/ChangeLog:39
> +        Two new debugging tools are also added to the jsc cli. One is a

Should "cli" be "CLI"?

> Source/JavaScriptCore/ChangeLog:42
> +        version of the print function with a private name so it can be
> +        used for debugging builtins. The other is dumpDataLog, which takes
> +        a JSValue and runs our dataLog function on it.

You can already do this by using JSC_useDollarVM=1, and in builtin code do:
     @$vm.print("The value of this var is ", $vm.value(myVar), "\n");

Do we really need to add another version of it (especially since this is not code that is supposed to be left in the builtin)?  Using $vm also has the advantage that it will be disabled by default, and if you forget to take the debugging code out, the tests will barf with an undefined $vm error.

NOTE: JSC_useDollarVM is a "Restricted" option.  That means, if you want to use it on release builds, you will need to modify allowRestrictedOptions() in Options.cpp to return true.  It is built in by default on debug builds.
Comment 5 Mark Lam 2016-06-15 15:01:52 PDT
Comment on attachment 281315 [details]
Patch

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

>> Source/JavaScriptCore/ChangeLog:42
>> +        a JSValue and runs our dataLog function on it.
> 
> You can already do this by using JSC_useDollarVM=1, and in builtin code do:
>      @$vm.print("The value of this var is ", $vm.value(myVar), "\n");
> 
> Do we really need to add another version of it (especially since this is not code that is supposed to be left in the builtin)?  Using $vm also has the advantage that it will be disabled by default, and if you forget to take the debugging code out, the tests will barf with an undefined $vm error.
> 
> NOTE: JSC_useDollarVM is a "Restricted" option.  That means, if you want to use it on release builds, you will need to modify allowRestrictedOptions() in Options.cpp to return true.  It is built in by default on debug builds.

Correction: the $vm example should be:
    @$vm.print("The value of this var is ", @$vm.value(myVar), "\n");

I was missing the @ before $vm in the @$vm.value() case.

> Source/JavaScriptCore/builtins/ArrayPrototype.js:712
> +        if (@isArrayConstructor(constructor) && @Array !== constructor)
> +            constructor = @Array;

Perf-wise (for the LLINT and baseline JIT), is it better to do the "@Array !== constructor" check, or to just skip it and just do the assignment?  I suspect its better to skip the second condition.

> Source/JavaScriptCore/bytecompiler/BytecodeGenerator.cpp:-4135
> -

Please revert.

> Source/JavaScriptCore/runtime/ArrayPrototype.cpp:1017
> +                return false;

Please fix indentation.

> Source/JavaScriptCore/runtime/ArrayPrototype.cpp:1050
> +            ASSERT(vm.exception());

Is this assertion correct?  moveElement() can do a getProperty() on the source, and getProperty() can invoke getters.  Hence, an exception can be thrown, no?

> Source/JavaScriptCore/runtime/ArrayPrototype.cpp:1060
> +        RELEASE_ASSERT(result->appendMemcpy(exec, vm, 0, first));

Putting this statement in a RELEASE_ASSERT suggests that it is optional when that is not the case.  Can you express this as "bool appendMemcpySucceeded = ..." and "RELEASE_ASSERT(appendMemcpySucceeded);" instead?

> Source/JavaScriptCore/runtime/ArrayPrototype.cpp:1076
> +    // This code assumes that the neither array has set Symbol.isConcatSpreadable. If the first array

Remove "the" before "neither".

> Source/JavaScriptCore/runtime/ArrayPrototype.cpp:1126
> +    } else if (type != ArrayWithUndecided) {
> +        WriteBarrier<Unknown>* buffer = result->butterfly()->contiguous().data();
> +        memcpy(buffer, firstButterfly->contiguous().data(), sizeof(JSValue) * firstArraySize);
> +        memcpy(buffer + firstArraySize, secondButterfly->contiguous().data(), sizeof(JSValue) * secondArraySize);
> +    }

I presume (type == ArrayWithUndecided) means that the length of both are 0.  Can you make that explicit here with an else case that asserts the following?

       ASSERT(!firstArraySize && !secondArraySize);

> Source/JavaScriptCore/runtime/JSArray.cpp:411
> +            ASSERT(copyType == ArrayWithUndecided);
> +            return true;

Please add an assertion here that makes it clear that ArrayWithUndecided means 0 length:
    ASSERT(!otherArray->length());

> Source/JavaScriptCore/runtime/JSArray.cpp:427
> +    if (length() < newLength) {
> +        throwOutOfMemoryError(exec);
> +        return false;
> +    }

Shouldn't ensureLength() already guarantee that (length() >= newLength)?  You should move this throwOutOfMemoryError() into the fail case for ensureLength() above.

> Source/JavaScriptCore/runtime/JSArrayInlines.h:29
> +IndexingType JSArray::memCopyWithIndexingType(IndexingType other)

This name "memCopyWithIndexingType" is a bit misleading because it implies that it will do a memcpy.  How about naming it something like "resultIndexingTypeForMemCopy" or "computeResultIndexingTypeForMemCopy"instead?
Comment 6 Keith Miller 2016-06-15 16:59:09 PDT
Created attachment 281404 [details]
Patch
Comment 7 Keith Miller 2016-06-15 17:13:46 PDT
Created attachment 281407 [details]
Patch
Comment 8 WebKit Commit Bot 2016-06-15 17:15:27 PDT
Attachment 281407 [details] did not pass style-queue:


ERROR: Source/JavaScriptCore/runtime/ArrayPrototype.cpp:1064:  Code inside a namespace should not be indented.  [whitespace/indent] [4]
Total errors found: 1 in 60 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 9 Keith Miller 2016-06-15 17:24:36 PDT
Created attachment 281410 [details]
Patch
Comment 10 Mark Lam 2016-06-15 17:29:29 PDT
Comment on attachment 281410 [details]
Patch

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

r=me with one fix (assuming EWS bots will be happy).

> Source/JavaScriptCore/builtins/ArrayPrototype.js:713
> +        if (@isObject(constructor)) {

I think you should make this an "else if" since @isObject() should always fail for @undefined.
Comment 11 Keith Miller 2016-06-15 17:34:05 PDT
Created attachment 281415 [details]
Patch for landing
Comment 12 Build Bot 2016-06-15 18:34:10 PDT
Comment on attachment 281415 [details]
Patch for landing

Attachment 281415 [details] did not pass mac-ews (mac):
Output: http://webkit-queues.webkit.org/results/1509985

New failing tests:
js/array-species-different-globalobjects.html
Comment 13 Build Bot 2016-06-15 18:34:13 PDT
Created attachment 281419 [details]
Archive of layout-test-results from ews101 for mac-yosemite

The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: ews101  Port: mac-yosemite  Platform: Mac OS X 10.10.5
Comment 14 Build Bot 2016-06-15 18:36:49 PDT
Comment on attachment 281415 [details]
Patch for landing

Attachment 281415 [details] did not pass ios-sim-ews (ios-simulator-wk2):
Output: http://webkit-queues.webkit.org/results/1509969

New failing tests:
js/array-species-different-globalobjects.html
Comment 15 Build Bot 2016-06-15 18:36:53 PDT
Created attachment 281420 [details]
Archive of layout-test-results from ews125 for ios-simulator-wk2

The attached test failures were seen while running run-webkit-tests on the ios-sim-ews.
Bot: ews125  Port: ios-simulator-wk2  Platform: Mac OS X 10.11.4
Comment 16 Build Bot 2016-06-15 18:37:58 PDT
Comment on attachment 281415 [details]
Patch for landing

Attachment 281415 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://webkit-queues.webkit.org/results/1509990

New failing tests:
js/array-species-different-globalobjects.html
Comment 17 Build Bot 2016-06-15 18:38:00 PDT
Created attachment 281421 [details]
Archive of layout-test-results from ews105 for mac-yosemite-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: ews105  Port: mac-yosemite-wk2  Platform: Mac OS X 10.10.5
Comment 18 Build Bot 2016-06-15 18:43:02 PDT
Comment on attachment 281415 [details]
Patch for landing

Attachment 281415 [details] did not pass mac-debug-ews (mac):
Output: http://webkit-queues.webkit.org/results/1509987

New failing tests:
js/array-species-different-globalobjects.html
js/kde/Array.html
Comment 19 Build Bot 2016-06-15 18:43:05 PDT
Created attachment 281422 [details]
Archive of layout-test-results from ews113 for mac-yosemite

The attached test failures were seen while running run-webkit-tests on the mac-debug-ews.
Bot: ews113  Port: mac-yosemite  Platform: Mac OS X 10.10.5
Comment 20 Keith Miller 2016-06-15 19:17:59 PDT
Created attachment 281426 [details]
Patch for landing
Comment 21 Keith Miller 2016-06-15 19:55:53 PDT
> I presume (type == ArrayWithUndecided) means that the length of both are 0. 
> Can you make that explicit here with an else case that asserts the following?
> 
>        ASSERT(!firstArraySize && !secondArraySize);
> 
> > Source/JavaScriptCore/runtime/JSArray.cpp:411
> > +            ASSERT(copyType == ArrayWithUndecided);
> > +            return true;
> 
> Please add an assertion here that makes it clear that ArrayWithUndecided
> means 0 length:
>     ASSERT(!otherArray->length());
> 

These assertions are not right. We use ArrayWithUndecided for arrays with non-zero length but no elements in them. For instance, Array(5).
Comment 22 Mark Lam 2016-06-15 20:49:09 PDT
(In reply to comment #21)
> > Please add an assertion here that makes it clear that ArrayWithUndecided
> > means 0 length:
> >     ASSERT(!otherArray->length());
> > 
> 
> These assertions are not right. We use ArrayWithUndecided for arrays with
> non-zero length but no elements in them. For instance, Array(5).

You're right.  Sorry for my mistake.  Please remove those asserts.
Comment 23 WebKit Commit Bot 2016-06-15 23:01:47 PDT
Comment on attachment 281426 [details]
Patch for landing

Clearing flags on attachment: 281426

Committed r202125: <http://trac.webkit.org/changeset/202125>
Comment 24 WebKit Commit Bot 2016-06-15 23:01:52 PDT
All reviewed patches have been landed.  Closing bug.
Comment 25 Chris Dumez 2016-06-16 09:34:33 PDT
I am working on confirming but this looks like a 1% PLT regression on iOS :(
Comment 26 Ryosuke Niwa 2016-06-16 17:01:36 PDT
This also appears to be 1% JSBench regression on iPhone 6 Plus.
Comment 27 Chris Dumez 2016-06-30 13:25:36 PDT
(In reply to comment #25)
> I am working on confirming but this looks like a 1% PLT regression on iOS :(

Confirmed slight regression on iOS.