Implement array.prototype.flat and flatMap #5573

rhuanjl · 2018-08-04T23:11:16Z

This PR implements the stage 3 proposal Array methods: Array.prototype.flat and Array.prototype.flatMap as JsBuiltIns.

Draft specification text is here: https://tc39.github.io/proposal-flatMap/

Notes:

3 test262 failures:
- length is not configurable (for both methods), this is due to ScriptFunction lengths in CC all not being configurable and should be fixed if Fix configurability of function.length and boundFunction.length #5405 is merged
- the case using null as this fails -- EDIT: FIXED
I can't find a feasible way to test the length is too big error - not sure if CC can actually handle arrays that big anyway so maybe not worth implementing that case?
Should probably have a few more CI tests

MSLaguana · 2018-08-05T00:15:08Z

lib/Runtime/Library/JsBuiltIn/JsBuiltIn.js

+            // b. Let exists be ? HasProperty(source, P).
+            if (sourceIndex in source) {
+                // c. If exists is true, then
+                //  i. Let element be ? Get(source, P).


Does the code as written work correctly in the presence of proxies / getters? The spec seems to say that the element is accessed once, while below you access it multiple times.

Good point - that's not in the test262 tests and I didn't think of it - currently this will certainly not work correctly with proxies/getters.

Should now be fixed - I've also added a case using a Proxy to test this.

It’d be great to contribute those tests back to test262 :-)

@ljharb I'll endeavour to contribute some equivalent tests to test262 later this week - I've not written a test262 test before so will take me a bit of time to understand how to - I can see immediately though that I can't simply submit the same there as I have here as the test262 harness is very different to CC's UTF.

MSLaguana · 2018-08-05T00:21:15Z

lib/Runtime/Library/JsBuiltIn/JsBuiltIn.js

+                //      1. Let elementLen be ? ToLength(? Get(element, "length")).
+                //      2. Set targetIndex to ? FlattenIntoArray(target, element, elementLen, targetIndex, depth - 1).
+                if (__chakraLibrary.isArray(element)) {
+                    targetIndex = __chakraLibrary.FlattenIntoArray(target, element, element.length |0, targetIndex, 0);


Looks like you set the depth in the recursive call to 0 rather than depth -1, is that intentional? I also saw that you don't check for depth > 0 here either.

Oh, you don't have a depth at all... Should this be recursively calling into FlattenIntoArrayMapped and passing the mapper function down as well?

This is intentional - I should probably comment it. flatMap always uses depth = 1 and this point is only reached from flatMap (and 1 -1 = 0).

The deviation from spec text is because I split the spec's FlattenIntoArray function into 2 separate functions one for where a callback function is needed and one for where it's not.

Huh, just read the spec. I didn't expect that from my intuition about these functions, but I agree with you. While adding the comment, I'd also appreciate one stating that the thisArg of the spec is expected to be bound to the mapperFunction by the caller.

MSLaguana · 2018-08-05T00:37:04Z

Regarding testing the length-too-long error; I'd be surprised if any engine today can hit that case... 2^53-1 is a lot of items! If each just took up 1 bit, you'd still need ~2^50 bytes of memory... I guess if you were dedicated enough and willing to use swap space then you could get there... But for now I think we're ok without a test for that specific case.

MSLaguana · 2018-08-05T00:46:23Z

Regarding the failure with passing null as thisArg, looks like that might be due to a spec change; see the note at https://tc39.github.io/ecma262/#sec-function.prototype.call

@zenparsing do you know if binding null or undefined is supposed to keep that value as-is in the called function, rather than substituting the global object?

MSLaguana · 2018-08-05T00:50:49Z

Oh; null and undefined aren't replaced if the function is a strict function. I see now that the test case says that it should only be run in strict mode, how were you running it @rhuanjl ? Were you using the test 262 test harness, or just running that one file?

rhuanjl · 2018-08-05T00:58:26Z

I made my own mini-harness to run the test files. That case still failed with strict mode though - without strict mode most of the cases in the file failed, with strict mode only that case (the one with null) failed.

boingoing · 2018-08-05T00:59:45Z

lib/Runtime/Library/JsBuiltIn/JsBuiltIn.js

+                    //  1. Let elementLen be ? ToLength(? Get(element, "length")).
+                    //  2. Set targetIndex to ? FlattenIntoArray(target, element, elementLen, targetIndex, depth - 1).
+                    element = source[sourceIndex];
+                    targetIndex = __chakraLibrary.FlattenIntoArray(target, element, element.length |0, targetIndex, depth - 1);


I believe this operation (element.length |0) is not doing the same thing as ToLength. Seems the operation is closer to ToInteger since it does not clamp negative values to 0 like ToLength does.

boingoing · 2018-08-05T01:02:38Z

lib/Runtime/Library/JsBuiltIn/JsBuiltIn.js

+                //  iii. Let shouldFlatten be false.
+                //  iv. If depth > 0, then
+                //      1. Set shouldFlatten to ? IsArray(element).
+                if (depth > 0 && __chakraLibrary.isArray(source[sourceIndex])) {


As mentioned above, we should not be doing this property get for source[sourceIndex] here as well as another lookup in the conditional branches.

MSLaguana · 2018-08-05T01:03:51Z

lib/Runtime/Library/JsBuiltIn/JsBuiltIn.js

+        //5. Let A be ? ArraySpeciesCreate(O, 0).
+        const A = __chakraLibrary.arraySpeciesCreate(o, 0);
+        //6. Perform ? FlattenIntoArray(A, O, sourceLen, 0, depthNum).
+        if (!thisArg) {


I think this is the reason the test is failing; you are just checking the truthiness. Undefined works because when you call a strict function foo() it will get undefined as its this, but with this check passing false or 0 or the empty string or any other falsy value would fail I'd suggest changing this to thisArg === undefined, and in all other cases going with the bind in the else.

boingoing · 2018-08-05T01:04:40Z

lib/Runtime/Library/JsBuiltIn/JsBuiltIn.js

+                    targetIndex = __chakraLibrary.FlattenIntoArray(target, element, element.length |0, targetIndex, 0);
+                } else {
+                    //  vi. Else,
+                    //      1. If targetIndex >= 253-1, throw a TypeError exception.


Little typo here, should probably be 2^53-1 not 253-1.

MSLaguana · 2018-08-05T01:06:38Z

Apart from those small comments, this looks reasonable to me. Thanks for taking this on!

I'm not sure why the no_icu_static_ubuntu_linux_debug test is failing, it looks like somehow the two functions haven't been added to the array prototype, but in the normal static_ubuntu_linux_debug CI run the test did run and pass.

MSLaguana · 2018-08-05T01:07:34Z

Ah, and just saw that windows 10 ci_lite_x64_debug also failed in the same way.

rhuanjl · 2018-08-05T01:11:02Z

Do the windows lite build and ubuntu no_icu builds build without JsBuiltins? (these functions are the first JsBuiltins without a fall back C++ implementation so if JsBuiltins are disabled these won't work)

MSLaguana · 2018-08-05T01:22:35Z

Looks like that might be the case: https://github.com/Microsoft/ChakraCore/blob/0fe9eb90a69fa4d148013241276d1824676f628e/lib/Common/CommonDefines.h#L133-L136

lite or non-intl-non-windows builds don't enable js builtins it seems. It looks like we don't have a specific flag to avoid just lite runs, but you could add the Intl tag to your test rlexe.xml file for now, I think that will limit it to appropriate flavors

rhuanjl · 2018-08-05T11:46:28Z

Thank you for the reviews @MSLaguana @boingoing I think I've fixed all the points you've raised - I'll hold off on squashing for now as I anticipate that some further reviews/changes will be required.

I'm nervous about non-icu/lite builds not supporting this - could be confusing for some users, should/could we either:
a) enable JsBuiltIns for non-icu/lite builds - they don't need icu etc. OR
b) put in a stub that throws a message saying that these methods aren't supported in a non-icu/lite build. OR
c) Make a c++ implentation

MSLaguana · 2018-08-06T15:20:59Z

Just spoke with @curtisman about this, he recommends option 1, although we need to investigate whether that will require another flavor of checked in generated bytecode if the lite build of chakracore is sufficiently different to the nojit flavor.

rhuanjl · 2018-08-06T20:11:49Z

Lite build seems to work with the "normal" no-jit bytecode - tested offline and CI runs with it - as can be seen. I figure that a change of that kind should perhaps be a separate PR though? (I added it here just to test the idea)

jackhorton · 2018-08-06T20:21:41Z

lib/Common/CommonDefines.h

 #endif

+#define ENABLE_JS_BUILTINS                          // Built In functions support


@sigatrev @meg-gupta If we enable JS Builtins for Lite builds, do you know if there is any reason aside from historical reference to keep native implementations of JS Builtins around?

I assume it's historical, so that we had a way to back out if needed. Js Builtins are relatively new with the first one, indexOf, it was not immediately clear if the benefits were significant.

sigatrev · 2018-08-06T22:22:35Z

lib/Runtime/Library/JsBuiltIn/JsBuiltIn.js

+            sourceLen =  __chakraLibrary.GetLength(o);
+        }
+        //3. If IsCallable(mapperFunction) is false throw a TypeError exception
+        if (!__chakraLibrary.isCallable(mapperFunction)) {


When implementing Array.prototype.filter we concluded that if (typeof callbackfn != "function") was sufficient to check IsCallable.

I believe that, per spec, typeof x === 'function' is exactly equivalent to IsCallable(x).

For an engine, this is correct. In libraries that need to account for older engines where this does not hold, they'd need a more intensive check - but chakra shouldn't need to worry about that.

sigatrev · 2018-08-06T22:30:55Z

Thanks for taking the time to implement this.

I took a lot at the JIT output and it seems to be doing a reasonable job, but recussions isn't handled very precisely when it comes to inlining. It'd be interesting to see how perf would compare to an implementation using a stack instead of recursion, but that's not strictly necessary for this PR.

rhuanjl · 2018-08-07T00:34:32Z

@sigatrev Thanks for the review. I've:

removed the IsCallable function AND
as a separate commit I've refactored FlattenIntoArray to use a stack - kept as a separate commit so can be easily dropped if not wanted AND
I've added an extra loop to FlattenIntoArrayMapped so it no longer has to call FlattenIntoArray

sigatrev · 2018-08-07T01:15:36Z

@rhuanjl I did some microbenchmarking and the original recursive case runs ~30% faster. Thanks for giving that a shot, and the quick turn-around.

rhuanjl · 2018-08-07T08:00:07Z

OK, I've updated to remove the stack based mechanism, I've kept the inner loop for FlattenIntoArrayMapped though as I assume that has minimal performance impact and if anything makes the code clearer.

sethbrenith · 2018-08-07T19:38:00Z

lib/Runtime/Library/JsBuiltIn/JsBuiltIn.js

+            sourceLen =  __chakraLibrary.GetLength(o);
+        }
+        //3. If IsCallable(mapperFunction) is false throw a TypeError exception
+        if (typeof mapperFunction != "function") {


Nit: !== is generally preferable

I copied that from Array.prototype.filter above - so if changing this one will change filter as well for consistency.

Seems reasonable to me, thanks!

In reply to: 208360161 [](ancestors = 208360161)

sethbrenith

boingoing

Thanks for taking our feedback. Looks good to me.

dilijev · 2018-08-10T00:57:42Z

lib/Runtime/Library/JsBuiltIn/JsBuiltIn.js

+        // depth = 1 and the presence of a mapperFunction guaranteed
+        // both these conditions are always met when this is called from flatMap
+        // Additionally this is slightly refactored rather than taking a thisArg
+        // the calling funciton binds the thisArg if it's required


nit: spelling in these comment lines (funciton -> function and FlatternIntoArray -> FlattenIntoArray)

dilijev

LGTM

dilijev · 2018-08-10T01:09:24Z

lib/Runtime/Library/JsBuiltIn/JsBuiltIn.js

+                    while (innerIndex < innerLength) {
+                        if (innerIndex in element) {
+                            //  1. If targetIndex >= 2^53-1, throw a TypeError exception.
+                            if (targetIndex >= 9007199254740991 /* 2^53 -1 */) {


nit: remove space before -1 for consistency

dilijev · 2018-08-10T01:09:45Z

lib/Runtime/Library/JsBuiltIn/JsBuiltIn.js

+                } else {
+                    //  vi. Else,
+                    //      1. If targetIndex >= 2^53-1, throw a TypeError exception.
+                    if (targetIndex >= 9007199254740991 /* 2^53 -1 */) {


nit: remove space before -1 for consistency

rhuanjl · 2018-08-10T22:32:28Z

@dotnet-bot test OSX static_osx_osx_debug please

rhuanjl · 2018-08-14T07:51:16Z

Please let me know what's needed for this to be merged.

I assume it will require a GUID update as it changes the byte code, anything else?

sigatrev · 2018-08-15T00:12:27Z

@rhuanjl I think this is pretty much ready. I should be able to get it merged in the next couple of days, sorry for the delay.

sigatrev · 2018-08-17T21:12:12Z

@rhuanjl can you enable us to update the PR so I update bytecode headers and GUID?

rhuanjl · 2018-08-17T21:20:02Z

@sigatrev Allow edits from maintainers is already ticked not sure if there's anything else I can do to give you additional access?

(let me know if you'd like me to regen the byte code)

Merge pull request #5573 from rhuanjl:flat This PR implements the stage 3 proposal Array methods: Array.prototype.flat and Array.prototype.flatMap as JsBuiltIns. Draft specification text is here: https://tc39.github.io/proposal-flatMap/ Notes: 1. 3 test262 failures: - length is not configurable (for both methods), this is due to ScriptFunction lengths in CC all not being configurable and should be fixed if #5405 is merged - the case using null as this fails -- EDIT: FIXED 2. I can't find a feasible way to test the length is too big error - not sure if CC can actually handle arrays that big anyway so maybe not worth implementing that case? 3. Should probably have a few more CI tests Fixes #5543

rhuanjl · 2018-08-17T23:08:24Z

Thanks for testing and merging this @sigatrev

And thanks everyone for the reviews.

sigatrev · 2018-08-17T23:09:20Z

@rhuanjl Thanks for your patience. We really appreciate the contributions.

rhuanjl force-pushed the flat branch from 7fd4d71 to 900c040 Compare August 4, 2018 23:13

MSLaguana reviewed Aug 5, 2018

View reviewed changes

boingoing reviewed Aug 5, 2018

View reviewed changes

MSLaguana reviewed Aug 5, 2018

View reviewed changes

boingoing reviewed Aug 5, 2018

View reviewed changes

rhuanjl force-pushed the flat branch from 2162b4b to 28f9328 Compare August 5, 2018 01:33

rhuanjl force-pushed the flat branch from 7cfdc56 to da97fe3 Compare August 6, 2018 19:27

jackhorton reviewed Aug 6, 2018

View reviewed changes

sigatrev reviewed Aug 6, 2018

View reviewed changes

rhuanjl force-pushed the flat branch from 7b57f77 to 5553a38 Compare August 7, 2018 00:41

rhuanjl force-pushed the flat branch from 5553a38 to ff3fed6 Compare August 7, 2018 07:58

rhuanjl force-pushed the flat branch from ff3fed6 to 6d3048f Compare August 7, 2018 08:00

sethbrenith reviewed Aug 7, 2018

View reviewed changes

sethbrenith approved these changes Aug 7, 2018

View reviewed changes

rhuanjl force-pushed the flat branch from 6d3048f to 0d04273 Compare August 7, 2018 20:23

boingoing approved these changes Aug 7, 2018

View reviewed changes

sigatrev approved these changes Aug 7, 2018

View reviewed changes

chicoxyzzy mentioned this pull request Aug 8, 2018

Implementation status tc39/proposal-flatMap#71

Open

4 tasks

kfarnung approved these changes Aug 9, 2018

View reviewed changes

dilijev reviewed Aug 10, 2018

View reviewed changes

dilijev approved these changes Aug 10, 2018

View reviewed changes

rhuanjl force-pushed the flat branch from 0d04273 to d95fab3 Compare August 10, 2018 20:58

rhuanjl force-pushed the flat branch from d95fab3 to d871da7 Compare August 12, 2018 00:10

dilijev added the External Contributor label Aug 17, 2018

rhuanjl and others added 3 commits August 17, 2018 14:48

array.prototype.flat AND array.prototype.flatMap

819ebeb

Enable JsBuiltIns on Lite and no_icu builds

0b023ba

update bytecode headers

9de922b

sigatrev force-pushed the flat branch from d871da7 to 9de922b Compare August 17, 2018 22:13

chakrabot merged commit 9de922b into chakra-core:master Aug 17, 2018

rhuanjl deleted the flat branch August 17, 2018 23:04

dilijev added the Bytecode-Update This PR updates bytecode and will cause merge conflicts with other PRs with this label label Sep 5, 2018

		#endif

		#define ENABLE_JS_BUILTINS // Built In functions support

Implement array.prototype.flat and flatMap #5573

Implement array.prototype.flat and flatMap #5573

Conversation

rhuanjl commented Aug 4, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rhuanjl Aug 16, 2018 • edited Loading

Choose a reason for hiding this comment

MSLaguana Aug 5, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

MSLaguana commented Aug 5, 2018

MSLaguana commented Aug 5, 2018

MSLaguana commented Aug 5, 2018

rhuanjl commented Aug 5, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

MSLaguana Aug 5, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

MSLaguana commented Aug 5, 2018

MSLaguana commented Aug 5, 2018

rhuanjl commented Aug 5, 2018

MSLaguana commented Aug 5, 2018

rhuanjl commented Aug 5, 2018

MSLaguana commented Aug 6, 2018

rhuanjl commented Aug 6, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sigatrev commented Aug 6, 2018

rhuanjl commented Aug 7, 2018 • edited Loading

sigatrev commented Aug 7, 2018

rhuanjl commented Aug 7, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sethbrenith left a comment

Choose a reason for hiding this comment

boingoing left a comment

Choose a reason for hiding this comment

dilijev Aug 10, 2018 • edited Loading

Choose a reason for hiding this comment

dilijev left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rhuanjl commented Aug 10, 2018

rhuanjl commented Aug 14, 2018

sigatrev commented Aug 15, 2018

sigatrev commented Aug 17, 2018

rhuanjl commented Aug 17, 2018

rhuanjl commented Aug 17, 2018

sigatrev commented Aug 17, 2018

rhuanjl commented Aug 4, 2018 •

edited

Loading

rhuanjl Aug 16, 2018 •

edited

Loading

MSLaguana Aug 5, 2018 •

edited

Loading

MSLaguana Aug 5, 2018 •

edited

Loading

rhuanjl commented Aug 7, 2018 •

edited

Loading

dilijev Aug 10, 2018 •

edited

Loading