Implemented every, some, includes, and reduce for array.prototype as JsBuiltIn #5723

yullin-ms · 2018-09-22T04:41:09Z

Implemented every, some, includes, and reduce for array.prototype as JsBuiltIn

jackhorton · 2018-09-22T05:02:21Z

Oh no, I wrote all of my initial comments on your other PR. Please cross-reference as needed :)

jackhorton · 2018-09-22T05:05:23Z

PS, for rebasing, I find it easiest to make the bytecode updates completely separate. So, when you have local changes, git add . && git reset *.bc.* to stage all of the non-bytecode changes. Then, when rebasing (specifically with interactive mode, git rebase -i), you can just drop the bytecode commit because it will almost certainly conflict.

rhuanjl · 2018-09-22T16:21:08Z

Just thought I should note; I experimented with an Array.prototype.every implementation ages ago: rhuanjl@71ee608

That was in response to this discussion on node-chakracore: nodejs/node-chakracore#527

But I did not submit it as it actually showed a performance regression - I tried re-running the case from that issue today but it doesn't run at all with the latest node-chakracore with cc master so can't check the performance impact of this on it anymore.

yullin-ms · 2018-09-24T00:42:13Z

@rhuanjl Thank you for the input, do you know of a another way to repro the testing in that previous discussion?

kfarnung · 2018-09-24T18:12:21Z

@rhuanjl I don't think I'd block on that. I can take a look separately and see why it's not working with the latest node-chakracore, it did build successfully with 10.6.0, but I haven't tried with a newer version.

sigatrev

LGTM. have you run test262 to confirm there are no regressions in spec compliance?

lib/Runtime/Library/JsBuiltIn/JsBuiltIn.js

sigatrev · 2018-09-24T18:20:29Z

lib/Runtime/Library/JsBuiltIn/JsBuiltIn.js

@@ -363,27 +365,245 @@
    platform.registerFunction(platform.FunctionKind.Array_forEach, function (callbackfn, thisArg = undefined) {
        // ECMAScript 2017 #sec-array.prototype.foreach

+        //Let O be ? ToObject(this value).


+1 for adding the spec comments.

akroshg · 2018-09-24T18:26:05Z

    CHAKRATEL_LANGSTATS_INC_BUILTINCOUNT(Array_Prototype_reduce);

Not sure - but do we put assert here that we should not reach here unless we have disabled the jsbuiltins?

Refers to: lib/Runtime/Library/JavascriptArray.cpp:9916 in e2a493e. [](commit_id = e2a493e, deletion_comment = False)

lib/Runtime/Library/JsBuiltInEngineInterfaceExtensionObject.cpp

rhuanjl · 2018-09-24T18:32:28Z

@kfarnung I just wanted to flag it as whilst that test doesn't run at all now when I experimented with a Jsbuiltin Array.prototype.every before - intending to improve its performance it actually made it worse.

akroshg · 2018-09-24T18:55:06Z

lib/Runtime/Library/JsBuiltIn/JsBuiltIn.js

        return undefined;
    });

+    platform.registerFunction(platform.FunctionKind.Array_some, function (callbackfn, thisArg = undefined) {


spoken in person, but please check the typedarray performance change on these apis.

lib/Runtime/Library/JsBuiltIn/JsBuiltIn.js

duongnhn · 2018-09-25T04:53:21Z

@dotnet-bot test this please

lib/Runtime/Library/JsBuiltInEngineInterfaceExtensionObject.cpp

sethbrenith

akroshg

yullin-ms · 2018-09-25T17:02:30Z

@dotnet-bot test Windows 10 x64_debug

dilijev · 2018-09-25T17:44:31Z

Temporarily retargeted this PR to master-ci to use a different Jenkins server. Please target back to master before merging.

lib/Runtime/Library/JsBuiltIn/JsBuiltIn.js

lib/Runtime/Library/EngineInterfaceObjectBuiltIns.h

dilijev · 2018-09-26T19:25:20Z

    CHAKRATEL_LANGSTATS_INC_BUILTINCOUNT(Array_Prototype_reduce);

Also, how would we have ENABLE_JS_BUILTINS but !IsJsBuiltInEnabled?

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

Refers to: lib/Runtime/Library/JavascriptArray.cpp:9916 in dc65f46. [](commit_id = dc65f46, deletion_comment = False)

dilijev · 2018-09-26T19:26:10Z

lib/Runtime/Library/JavascriptArray.cpp

-                JavascriptError::ThrowTypeError(scriptContext, VBSERR_ActionNotSupported);
+                if (typedArrayBase)
+                {
+                    JavascriptError::ThrowTypeError(scriptContext, JSERR_EmptyArrayAndInitValueNotPresent, _u("TypedArray.prototype.reduce"));


TypedArray [](start = 111, length = 10)

TypedArray is a synthetic/catch-all type (not a user visible type) -- would this error message be confusing?

It's pretty well-known for a type that never actually shows up in JS code (it's on MDN, for example). Elsewhere in this file we use [TypedArray] to hint at it not being a real type; would that be better?

@jackhorton We spoke on this [TypedArray] vs TypeArray, I'm curious on your thoughts.

V8 doesnt say TypedArray" in its error, but the TypedArray constructor is technically visible. Also, the actual method is TypedArray.prototype.reduce -- all of the actual constructors (UInt8Array, etc) get it from the TypedArray prototype. So I voted in favor of just calling it what MDN calls it, which is TypedArray.prototype.reduce

dilijev

sigatrev · 2018-09-26T20:00:59Z

Also, how would we have ENABLE_JS_BUILTINS but !IsJsBuiltInEnabled?

ENABLE_JS_BUILTINS is a compile-time macro. IsJsBuiltInEnabled is controlled by the -jsBuiltIns flag.

…recursive loop to iterative in MayHaveSideEffectOnNode to avoid a stack overflow. Merge pull request chakra-core#5739 from wyrichte:build/wyrichte/stack_overflow_1

Merge pull request chakra-core#5733 from yullin-ms:argLenAndConstOptPhaseTwo This is to do inline arg optimization when we do argument length and argument constant optimizations

… not number for float type specialization Merge pull request chakra-core#5743 from nhat-nguyen:BailOutFloatTypeSpec

…ackArgsOpt Merge pull request chakra-core#5744 from nhat-nguyen:stackargs `DoStackArgsOpt` takes a `topFunc` parameter but doesn't use it. It seems like the intention is to pass it to `IsStackArgsEnabled`, but `topFunc` is actually referenced inside `IsStackArgsEnabled` through a pointer in m_func, making passing in the argument unnecessary.

…, and reduce for array.prototype as JsBuiltIn Merge pull request chakra-core#5723 from yullin-ms:arrayEverySomeIncludesReduce Implemented every, some, includes, and reduce for array.prototype as JsBuiltIn

yullin-ms requested a review from jackhorton September 22, 2018 04:58

yullin-ms force-pushed the arrayEverySomeIncludesReduce branch 2 times, most recently from a65f6fd to e2a493e Compare September 22, 2018 05:35

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

sigatrev reviewed Sep 24, 2018

View reviewed changes

akroshg reviewed Sep 24, 2018

View reviewed changes

lib/Runtime/Library/JsBuiltInEngineInterfaceExtensionObject.cpp Show resolved Hide resolved

akroshg reviewed Sep 24, 2018

View reviewed changes

lib/Runtime/Library/JsBuiltIn/JsBuiltIn.js Outdated Show resolved Hide resolved

yullin-ms force-pushed the arrayEverySomeIncludesReduce branch 2 times, most recently from d4db7c0 to 53017fb Compare September 24, 2018 22:35

sethbrenith reviewed Sep 25, 2018

View reviewed changes

lib/Runtime/Library/JsBuiltInEngineInterfaceExtensionObject.cpp Outdated Show resolved Hide resolved

sethbrenith approved these changes Sep 25, 2018

View reviewed changes

akroshg approved these changes Sep 25, 2018

View reviewed changes

dilijev closed this Sep 25, 2018

dilijev reopened this Sep 25, 2018

dilijev changed the base branch from master to master-ci September 25, 2018 17:44

dilijev reviewed Sep 25, 2018

View reviewed changes

lib/Runtime/Library/JsBuiltIn/JsBuiltIn.js Outdated Show resolved Hide resolved

yullin-ms changed the base branch from master-ci to master September 25, 2018 18:12

yullin-ms changed the base branch from master to master-ci September 25, 2018 18:13

yullin-ms force-pushed the arrayEverySomeIncludesReduce branch from 318beb1 to dc65f46 Compare September 25, 2018 23:11

yullin-ms changed the base branch from master-ci to master September 26, 2018 16:58

dilijev reviewed Sep 26, 2018

View reviewed changes

lib/Runtime/Library/EngineInterfaceObjectBuiltIns.h Outdated Show resolved Hide resolved

dilijev reviewed Sep 26, 2018

View reviewed changes

dilijev approved these changes Sep 26, 2018

View reviewed changes

kfarnung approved these changes Sep 26, 2018

View reviewed changes

yullin-ms and others added 13 commits September 28, 2018 16:13

Phase Two Changes

8aa44c3

[MERGE chakra-core#5739 @wyrichte] Fixes chakra-core#5699 - Converts …

3321ead

…recursive loop to iterative in MayHaveSideEffectOnNode to avoid a stack overflow. Merge pull request chakra-core#5739 from wyrichte:build/wyrichte/stack_overflow_1

Bail out when either operand is not number for float type specialization

3c0a6a1

Remove unused parameter in DoStackArgsOpt

660e1d5

Added test trace and phase

5d51cf6

[MERGE chakra-core#5733 @yullin-ms] Inline Arg Optimization

0617d9f

Merge pull request chakra-core#5733 from yullin-ms:argLenAndConstOptPhaseTwo This is to do inline arg optimization when we do argument length and argument constant optimizations

[MERGE chakra-core#5743 @nhat-nguyen] Bail out when either operand is…

33c525c

… not number for float type specialization Merge pull request chakra-core#5743 from nhat-nguyen:BailOutFloatTypeSpec

Implemented every, some, includes, and reduce for array.prototype

cf6e8d2

Updated Bytecode

c7be06b

Minor updates

7052e09

Fixed wrong association with array_flat to EntryMap

5f5573e

Updated for build failures

54b0bba

yullin-ms force-pushed the arrayEverySomeIncludesReduce branch from dc65f46 to 540a95d Compare October 4, 2018 17:56

Updated bytecode

45c0553

yullin-ms force-pushed the arrayEverySomeIncludesReduce branch from 540a95d to 45c0553 Compare October 4, 2018 17:59

dilijev merged commit 45c0553 into chakra-core:master Oct 4, 2018

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Implemented every, some, includes, and reduce for array.prototype as JsBuiltIn #5723

Implemented every, some, includes, and reduce for array.prototype as JsBuiltIn #5723

yullin-ms commented Sep 22, 2018

jackhorton commented Sep 22, 2018

jackhorton commented Sep 22, 2018

rhuanjl commented Sep 22, 2018

yullin-ms commented Sep 24, 2018

kfarnung commented Sep 24, 2018

sigatrev left a comment

sigatrev Sep 24, 2018

akroshg commented Sep 24, 2018

rhuanjl commented Sep 24, 2018

akroshg Sep 24, 2018

duongnhn commented Sep 25, 2018

sethbrenith left a comment

akroshg left a comment

yullin-ms commented Sep 25, 2018

dilijev commented Sep 25, 2018

dilijev commented Sep 26, 2018

dilijev Sep 26, 2018 •

edited

Loading

sethbrenith Sep 26, 2018

yullin-ms Sep 26, 2018

jackhorton Sep 26, 2018

dilijev left a comment

sigatrev commented Sep 26, 2018

Implemented every, some, includes, and reduce for array.prototype as JsBuiltIn #5723

Implemented every, some, includes, and reduce for array.prototype as JsBuiltIn #5723

Conversation

yullin-ms commented Sep 22, 2018

jackhorton commented Sep 22, 2018

jackhorton commented Sep 22, 2018

rhuanjl commented Sep 22, 2018

yullin-ms commented Sep 24, 2018

kfarnung commented Sep 24, 2018

sigatrev left a comment

Choose a reason for hiding this comment

sigatrev Sep 24, 2018

Choose a reason for hiding this comment

akroshg commented Sep 24, 2018

rhuanjl commented Sep 24, 2018

akroshg Sep 24, 2018

Choose a reason for hiding this comment

duongnhn commented Sep 25, 2018

sethbrenith left a comment

Choose a reason for hiding this comment

akroshg left a comment

Choose a reason for hiding this comment

yullin-ms commented Sep 25, 2018

dilijev commented Sep 25, 2018

dilijev commented Sep 26, 2018

dilijev Sep 26, 2018 • edited Loading

Choose a reason for hiding this comment

sethbrenith Sep 26, 2018

Choose a reason for hiding this comment

yullin-ms Sep 26, 2018

Choose a reason for hiding this comment

jackhorton Sep 26, 2018

Choose a reason for hiding this comment

dilijev left a comment

Choose a reason for hiding this comment

sigatrev commented Sep 26, 2018

dilijev Sep 26, 2018 •

edited

Loading