Bug 182440 – [JSC] Implement Array.prototype.flatMap and Array.prototype.flatten

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

[Yusuke Suzuki]

[JSC] Implement Array.prototype.flatMap and Array.prototype.flatten

[Yusuke Suzuki]

Created attachment 332978 [details]
Patch

[EWS Watchlist]

Comment on attachment 332978 [details]
Patch

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

Number of test failures exceeded the failure limit.

[EWS Watchlist]

Created attachment 332980 [details]
Archive of layout-test-results from ews113 for mac-sierra

The attached test failures were seen while running run-webkit-tests on the mac-debug-ews.
Bot: ews113  Port: mac-sierra  Platform: Mac OS X 10.12.6

[Yusuke Suzuki]

Created attachment 332981 [details]
Patch

[EWS Watchlist]

Comment on attachment 332981 [details]
Patch

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

New failing tests:
inspector/model/remote-object-get-properties.html
js/Object-getOwnPropertyNames.html

[EWS Watchlist]

Created attachment 332989 [details]
Archive of layout-test-results from ews102 for mac-sierra

The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: ews102  Port: mac-sierra  Platform: Mac OS X 10.12.6

[EWS Watchlist]

Comment on attachment 332981 [details]
Patch

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

New failing tests:
inspector/model/remote-object-get-properties.html
js/Object-getOwnPropertyNames.html

[EWS Watchlist]

Created attachment 332991 [details]
Archive of layout-test-results from ews106 for mac-sierra-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: ews106  Port: mac-sierra-wk2  Platform: Mac OS X 10.12.6

[EWS Watchlist]

Comment on attachment 332981 [details]
Patch

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

New failing tests:
js/Object-getOwnPropertyNames.html

[EWS Watchlist]

Created attachment 332993 [details]
Archive of layout-test-results from ews124 for ios-simulator-wk2

The attached test failures were seen while running run-webkit-tests on the ios-sim-ews.
Bot: ews124  Port: ios-simulator-wk2  Platform: Mac OS X 10.12.6

[EWS Watchlist]

Comment on attachment 332981 [details]
Patch

Attachment 332981 [details] did not pass win-ews (win):
Output: http://webkit-queues.webkit.org/results/6335019

New failing tests:
js/Object-getOwnPropertyNames.html

[EWS Watchlist]

Created attachment 332995 [details]
Archive of layout-test-results from ews204 for win-future

The attached test failures were seen while running run-webkit-tests on the win-ews.
Bot: ews204  Port: win-future  Platform: CYGWIN_NT-6.1-2.9.0-0.318-5-3-x86_64-64bit

[EWS Watchlist]

Comment on attachment 332981 [details]
Patch

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

New failing tests:
jsc-layout-tests.yaml/js/script-tests/Object-getOwnPropertyNames.js.layout-dfg-eager-no-cjit
jsc-layout-tests.yaml/js/script-tests/Object-getOwnPropertyNames.js.layout-no-llint
jsc-layout-tests.yaml/js/script-tests/Object-getOwnPropertyNames.js.layout-ftl-no-cjit
jsc-layout-tests.yaml/js/script-tests/Object-getOwnPropertyNames.js.layout-ftl-eager-no-cjit
jsc-layout-tests.yaml/js/script-tests/Object-getOwnPropertyNames.js.layout
jsc-layout-tests.yaml/js/script-tests/Object-getOwnPropertyNames.js.layout-no-cjit
jsc-layout-tests.yaml/js/script-tests/Object-getOwnPropertyNames.js.layout-no-ftl

[EWS Watchlist]

Comment on attachment 332981 [details]
Patch

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

Number of test failures exceeded the failure limit.

[EWS Watchlist]

Created attachment 332998 [details]
Archive of layout-test-results from ews116 for mac-sierra

The attached test failures were seen while running run-webkit-tests on the mac-debug-ews.
Bot: ews116  Port: mac-sierra  Platform: Mac OS X 10.12.6

[Yusuke Suzuki]

Created attachment 333030 [details]
Patch

[EWS Watchlist]

Comment on attachment 333030 [details]
Patch

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

New failing tests:
js/Object-getOwnPropertyNames.html

[EWS Watchlist]

Created attachment 333031 [details]
Archive of layout-test-results from ews103 for mac-sierra

The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: ews103  Port: mac-sierra  Platform: Mac OS X 10.12.6

[EWS Watchlist]

Comment on attachment 333030 [details]
Patch

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

New failing tests:
js/Object-getOwnPropertyNames.html

[EWS Watchlist]

Created attachment 333032 [details]
Archive of layout-test-results from ews106 for mac-sierra-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: ews106  Port: mac-sierra-wk2  Platform: Mac OS X 10.12.6

[Yusuke Suzuki]

Created attachment 333034 [details]
Patch

[Michael Ficarra]

The flatMap tests should include at least one test where the receiver has more than one entry. A very incorrect implementation could still pass these tests.

[Yusuke Suzuki]

Created attachment 333036 [details]
Patch

[Yusuke Suzuki]

(In reply to Michael Ficarra from comment #22)
> The flatMap tests should include at least one test where the receiver has
> more than one entry. A very incorrect implementation could still pass these
> tests.

Updated with new tests.

[EWS Watchlist]

Comment on attachment 333036 [details]
Patch

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

New failing tests:
media/modern-media-controls/tracks-support/tracks-support-show-panel-after-dragging-controls.html

[EWS Watchlist]

Created attachment 333038 [details]
Archive of layout-test-results from ews102 for mac-sierra

The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: ews102  Port: mac-sierra  Platform: Mac OS X 10.12.6

[Yusuke Suzuki]

(In reply to Build Bot from comment #26)
> Created attachment 333038 [details]
> Archive of layout-test-results from ews102 for mac-sierra
> 
> The attached test failures were seen while running run-webkit-tests on the
> mac-ews.
> Bot: ews102  Port: mac-sierra  Platform: Mac OS X 10.12.6

It’s unrelated.

[Yusuke Suzuki]

Created attachment 333064 [details]
Patch

[EWS Watchlist]

Comment on attachment 333064 [details]
Patch

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

New failing tests:
exceptionFuzz.yaml/exceptionFuzz/3d-cube.js.exception-fuzz

[Saam Barati]

(In reply to Build Bot from comment #29)
> Comment on attachment 333064 [details]
> Patch
> 
> Attachment 333064 [details] did not pass jsc-ews (mac):
> Output: http://webkit-queues.webkit.org/results/6361756
> 
> New failing tests:
> exceptionFuzz.yaml/exceptionFuzz/3d-cube.js.exception-fuzz

Is this a real failure?

[Yusuke Suzuki]

(In reply to Saam Barati from comment #30)
> (In reply to Build Bot from comment #29)
> > Comment on attachment 333064 [details]
> > Patch
> > 
> > Attachment 333064 [details] did not pass jsc-ews (mac):
> > Output: http://webkit-queues.webkit.org/results/6361756
> > 
> > New failing tests:
> > exceptionFuzz.yaml/exceptionFuzz/3d-cube.js.exception-fuzz
> 
> Is this a real failure?

This is flakiness. I uploaded the complete same patch for rebasing. And in previous upload, this does not show failures.

[Yusuke Suzuki]

Comment on attachment 333064 [details]
Patch

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

> Source/JavaScriptCore/builtins/ArrayPrototype.js:744
> +    var result = @arraySpeciesCreate(array, 0);

Later, I'll add @arraySpeciesCreate for this patch.

> Source/JavaScriptCore/builtins/ArrayPrototype.js:783
> +    var result = @arraySpeciesCreate(array, 0);

Ditto.

[Yusuke Suzuki]

Created attachment 333164 [details]
Patch

[Darin Adler]

Comment on attachment 333164 [details]
Patch

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

> Source/JavaScriptCore/builtins/ArrayPrototype.js:743
> +function arraySpeciesCreate(array, length)

Do we have test coverage for all the special cases in here?

> Source/JavaScriptCore/builtins/ArrayPrototype.js:756
> +    // We have this check so that if some array from a different global object
> +    // calls this map they don't get an array with the Array.prototype of the
> +    // other global object.
> +    if (arrayConstructorInRealm !== constructor && @isArrayConstructor(constructor))
> +        return @newArrayWithSize(length);

Test for this case?

> Source/JavaScriptCore/builtins/ArrayPrototype.js:781
> +                    @throwTypeError("flatten array exceeds 2**53 - 1");

This error string looks different in style from some others.

Is there some way to test this case, or would the test run too slowly?

> Source/JavaScriptCore/builtins/ArrayPrototype.js:800
> +    var depthNum = 1;
> +    var depth = @argument(0);
> +    if (depth !== @undefined)
> +        depthNum = @toInteger(depth);

I would have expected a ? : for this sort of thing:

    var depthInteger = depth === @undefined ? 1 : @toInteger(depth);

Assuming we generate good code for such expressions. Also makes me wonder if we should make a variety of @toInteger where you can pass a default value in rather than separately doing a check.

[Yusuke Suzuki]

Comment on attachment 333164 [details]
Patch

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

>> Source/JavaScriptCore/builtins/ArrayPrototype.js:743
>> +function arraySpeciesCreate(array, length)
> 
> Do we have test coverage for all the special cases in here?

Added

>> Source/JavaScriptCore/builtins/ArrayPrototype.js:756
>> +        return @newArrayWithSize(length);
> 
> Test for this case?

Added

>> Source/JavaScriptCore/builtins/ArrayPrototype.js:781
>> +                    @throwTypeError("flatten array exceeds 2**53 - 1");
> 
> This error string looks different in style from some others.
> 
> Is there some way to test this case, or would the test run too slowly?

Yeah, currently, we do not test it because it requires too much memory & time (as some of test262 tests are skipped due to this reason).
We should explore the effective way to test such things.

>> Source/JavaScriptCore/builtins/ArrayPrototype.js:800
>> +        depthNum = @toInteger(depth);
> 
> I would have expected a ? : for this sort of thing:
> 
>     var depthInteger = depth === @undefined ? 1 : @toInteger(depth);
> 
> Assuming we generate good code for such expressions. Also makes me wonder if we should make a variety of @toInteger where you can pass a default value in rather than separately doing a check.

Interestingly, the above code emits more efficient code (of course, it is a bit).
@toInteger is a bit costly operation. I'm now planning to introduce op_to_integer, but avoiding this is still a good way I think. (And it is well aligned to the spec description).

Before:
[  56] mov               loc8, Int32: 1(const0)
[  59] argument          loc10, 1    predicting None
[  63] nstricteq         loc12, loc10, Undefined(const1)
[  67] jfalse            loc12, 30(->97)
[  70] resolve_scope     loc16, loc3, PrivateSymbol.toInteger(@id3), <GlobalVar>, 0, 0x7f7071fe0000
[  77] get_from_scope    loc12, loc16, PrivateSymbol.toInteger(@id3), 2049<ThrowIfNotFound|GlobalVar|NotInitialization>, -1298534560    predicting None
[  85] mov               loc15, loc10

After:
[  56] argument          loc10, 1    predicting None
[  60] stricteq          loc12, loc10, Undefined(const0)
[  64] jfalse            loc12, 8(->72)
[  67] mov               loc8, Int32: 1(const1)
[  70] jmp               29(->99)
[  72] resolve_scope     loc16, loc3, PrivateSymbol.toInteger(@id3), <GlobalVar>, 0, 0x7fe402be0000
[  79] get_from_scope    loc12, loc16, PrivateSymbol.toInteger(@id3), 2049<ThrowIfNotFound|GlobalVar|NotInitialization>, 1129965920    predicting None
[  87] mov               loc15, loc10
[  90] call              loc8, loc12, 2, 22 (this at loc16) status(Could Take Slow Path)    Original; predicting None

[Yusuke Suzuki]

Committed r228266: <https://trac.webkit.org/changeset/228266>

[Radar WebKit Bug Importer]

<rdar://problem/37347060>