Bug 182440 - [JSC] Implement Array.prototype.flatMap and Array.prototype.flatten
Summary: [JSC] Implement Array.prototype.flatMap and Array.prototype.flatten
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: Yusuke Suzuki
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2018-02-02 09:56 PST by Yusuke Suzuki
Modified: 2018-02-08 02:45 PST (History)
11 users (show)

See Also:


Attachments
Patch (5.49 KB, patch)
2018-02-02 09:56 PST, Yusuke Suzuki
no flags Details | Formatted Diff | Diff
Archive of layout-test-results from ews113 for mac-sierra (440.82 KB, application/zip)
2018-02-02 10:49 PST, EWS Watchlist
no flags Details
Patch (9.03 KB, patch)
2018-02-02 10:58 PST, Yusuke Suzuki
no flags Details | Formatted Diff | Diff
Archive of layout-test-results from ews102 for mac-sierra (2.23 MB, application/zip)
2018-02-02 12:00 PST, EWS Watchlist
no flags Details
Archive of layout-test-results from ews106 for mac-sierra-wk2 (2.54 MB, application/zip)
2018-02-02 12:08 PST, EWS Watchlist
no flags Details
Archive of layout-test-results from ews124 for ios-simulator-wk2 (2.28 MB, application/zip)
2018-02-02 12:39 PST, EWS Watchlist
no flags Details
Archive of layout-test-results from ews204 for win-future (11.49 MB, application/zip)
2018-02-02 12:43 PST, EWS Watchlist
no flags Details
Archive of layout-test-results from ews116 for mac-sierra (409.63 KB, application/zip)
2018-02-02 13:52 PST, EWS Watchlist
no flags Details
Patch (28.99 KB, patch)
2018-02-03 02:19 PST, Yusuke Suzuki
no flags Details | Formatted Diff | Diff
Archive of layout-test-results from ews103 for mac-sierra (2.20 MB, application/zip)
2018-02-03 03:21 PST, EWS Watchlist
no flags Details
Archive of layout-test-results from ews106 for mac-sierra-wk2 (2.57 MB, application/zip)
2018-02-03 03:28 PST, EWS Watchlist
no flags Details
Patch (31.32 KB, patch)
2018-02-03 03:48 PST, Yusuke Suzuki
no flags Details | Formatted Diff | Diff
Patch (31.56 KB, patch)
2018-02-03 08:54 PST, Yusuke Suzuki
no flags Details | Formatted Diff | Diff
Archive of layout-test-results from ews102 for mac-sierra (2.28 MB, application/zip)
2018-02-03 09:57 PST, EWS Watchlist
no flags Details
Patch (31.50 KB, patch)
2018-02-05 00:14 PST, Yusuke Suzuki
no flags Details | Formatted Diff | Diff
Patch (34.11 KB, patch)
2018-02-06 02:47 PST, Yusuke Suzuki
darin: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Yusuke Suzuki 2018-02-02 09:56:10 PST
[JSC] Implement Array.prototype.flatMap and Array.prototype.flatten
Comment 1 Yusuke Suzuki 2018-02-02 09:56:25 PST
Created attachment 332978 [details]
Patch
Comment 2 EWS Watchlist 2018-02-02 10:49:55 PST
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.
Comment 3 EWS Watchlist 2018-02-02 10:49:56 PST
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
Comment 4 Yusuke Suzuki 2018-02-02 10:58:55 PST
Created attachment 332981 [details]
Patch
Comment 5 EWS Watchlist 2018-02-02 12:00:34 PST
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
Comment 6 EWS Watchlist 2018-02-02 12:00:35 PST
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
Comment 7 EWS Watchlist 2018-02-02 12:08:04 PST
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
Comment 8 EWS Watchlist 2018-02-02 12:08:05 PST
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
Comment 9 EWS Watchlist 2018-02-02 12:39:57 PST
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
Comment 10 EWS Watchlist 2018-02-02 12:39:59 PST
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
Comment 11 EWS Watchlist 2018-02-02 12:43:41 PST
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
Comment 12 EWS Watchlist 2018-02-02 12:43:52 PST
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
Comment 13 EWS Watchlist 2018-02-02 13:04:10 PST
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
Comment 14 EWS Watchlist 2018-02-02 13:52:49 PST
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.
Comment 15 EWS Watchlist 2018-02-02 13:52:50 PST
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
Comment 16 Yusuke Suzuki 2018-02-03 02:19:44 PST
Created attachment 333030 [details]
Patch
Comment 17 EWS Watchlist 2018-02-03 03:21:45 PST
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
Comment 18 EWS Watchlist 2018-02-03 03:21:47 PST
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
Comment 19 EWS Watchlist 2018-02-03 03:28:23 PST
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
Comment 20 EWS Watchlist 2018-02-03 03:28:24 PST
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
Comment 21 Yusuke Suzuki 2018-02-03 03:48:33 PST
Created attachment 333034 [details]
Patch
Comment 22 Michael Ficarra 2018-02-03 07:53:13 PST
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.
Comment 23 Yusuke Suzuki 2018-02-03 08:54:41 PST
Created attachment 333036 [details]
Patch
Comment 24 Yusuke Suzuki 2018-02-03 08:56:44 PST
(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.
Comment 25 EWS Watchlist 2018-02-03 09:57:18 PST
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
Comment 26 EWS Watchlist 2018-02-03 09:57:19 PST
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
Comment 27 Yusuke Suzuki 2018-02-04 06:42:41 PST
(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.
Comment 28 Yusuke Suzuki 2018-02-05 00:14:53 PST
Created attachment 333064 [details]
Patch
Comment 29 EWS Watchlist 2018-02-05 01:20:04 PST
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
Comment 30 Saam Barati 2018-02-05 11:44:29 PST
(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?
Comment 31 Yusuke Suzuki 2018-02-05 17:53:15 PST
(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.
Comment 32 Yusuke Suzuki 2018-02-05 17:53:49 PST
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.
Comment 33 Yusuke Suzuki 2018-02-06 02:47:32 PST
Created attachment 333164 [details]
Patch
Comment 34 Darin Adler 2018-02-07 09:43:53 PST
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.
Comment 35 Yusuke Suzuki 2018-02-08 02:40:08 PST
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
Comment 36 Yusuke Suzuki 2018-02-08 02:43:19 PST
Committed r228266: <https://trac.webkit.org/changeset/228266>
Comment 37 Radar WebKit Bug Importer 2018-02-08 02:45:26 PST
<rdar://problem/37347060>