Bug 158769 – Add support for Symbol.isConcatSpreadable (round 2)

• Status: RESOLVED
• Resolution: FIXED
• Component: New Bugs

[Keith Miller]

Add support for Symbol.isConcatSpreadable (round 2)

[Keith Miller]

Created attachment 281315 [details]
Patch

[Keith Miller]

Created attachment 281318 [details]
JS-benchmarks

[Keith Miller]

Created attachment 281319 [details]
JS-Bench benchmark

[Mark Lam]

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.

[Mark Lam]

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?

[Keith Miller]

Created attachment 281404 [details]
Patch

[Keith Miller]

Created attachment 281407 [details]
Patch

[WebKit Commit Bot]

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.

[Keith Miller]

Created attachment 281410 [details]
Patch

[Mark Lam]

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.

[Keith Miller]

Created attachment 281415 [details]
Patch for landing

[Build Bot]

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

[Build Bot]

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

[Build Bot]

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

[Build Bot]

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

[Build Bot]

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

[Build Bot]

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

[Build Bot]

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

[Build Bot]

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

[Keith Miller]

Created attachment 281426 [details]
Patch for landing

[Keith Miller]

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

[Mark Lam]

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

[WebKit Commit Bot]

Comment on attachment 281426 [details]
Patch for landing

Clearing flags on attachment: 281426

Committed r202125: <http://trac.webkit.org/changeset/202125>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Chris Dumez]

I am working on confirming but this looks like a 1% PLT regression on iOS :(

[Ryosuke Niwa]

This also appears to be 1% JSBench regression on iPhone 6 Plus.

[Chris Dumez]

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