implement Array.prototype.filter in javascript#5137

sigatrev · 2018-05-11T18:50:12Z

Implementing built-ins in JavaScript allows the JIT to optimize calls that must otherwise behave generically. This can provide significant performance improvements, particularly for array operations that loop over the array.

In a microbenchmark calling filter 1000 times on a 10000 element array, the JS implementation was 13x faster in the best case, and 20% faster in the worst case.

for (var i = 0; i < 1000; ++i) {
  a.filter(()=>false) // best case, array is never grown
}
// js: 30 ms
// c++: 400ms

for (var i = 0; i < 1000; ++i) {
  a.filter(()=>true) // worst case
}
// js: 550 ms
// c++: 700 ms

Adding a JIT fast path for __chakraLibrary.arrayCreateDataPropertyOrThrow similar to normal array assignment would drop the always-true case to 230 ms.

zenparsing

JS implementation looks good.

zenparsing · 2018-05-11T20:14:11Z

+
+                let kValue = o[k];
+                if (boundCallback(kValue, k, o))
+                {


nit: curly should probably go on previous line.

meg-gupta

LGTM. Would be good to run test262.

akroshg · 2018-05-11T21:26:09Z

+
+        let o;
+        let len
+        if (__chakraLibrary.isArray(this)) {


why not do Array.isArray()?

This calls the same function, but from a safe location so that we are unaffected if the user replaces Array.isArray

ah - thanks,

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

akroshg · 2018-05-11T21:30:06Z

+    Var JsBuiltInEngineInterfaceExtensionObject::EntryJsBuiltIn_Internal_ArraySpeciesCreate(RecyclableObject *function, CallInfo callInfo, ...)
+    {
+        EngineInterfaceObject_CommonFunctionProlog(function, callInfo);
+        Assert(args.Info.Count == 3);


Assert [](start = 8, length = 6)

AssertOfFailFast

I chose a normal Assert because the function is callable only through the JS built-ins and not in any user controlled way. If you think it needs to be a FailFast I'll change it.

It should be better be failfast it is just a check.

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

akroshg · 2018-05-11T21:31:50Z

+
+        if (newObj == nullptr)
+        {
+            newArr = scriptContext->GetLibrary()->CreateArray(0);


0 [](start = 62, length = 1)

should this be args.Values[2]?

yeah, it should be. thanks

sethbrenith · 2018-05-11T21:33:59Z

+            len = o.length;
        } else {
            o = __chakraLibrary.Object(this);
+            len = __chakraLibrary.GetLenth(o);


GetLenth [](start = 34, length = 8)

+g

akroshg · 2018-05-11T21:35:37Z

+                let kValue = o[k];
+                if (boundCallback(kValue, k, o))
+                {
+                    a[to] = kValue;


this is CreateDataPropertyOrThrow? will it perform the same as the spec wants it to?

I believe this will, but @zenparsing can hopefully confirm that.

@akroshg Good call, it's not quite the same. Assignment will utilize the prototype chain, whereas filter creates a data property directly on the output array.

Object.defineProperty(Array.prototype, '0', { get() { return 980 }, set(value) { console.log('called setter') }, }); // Does not log "called setter" [1].filter(() => true); // Logs "called setter" let a = []; a[0] = 1;

I think the best course of action here is to use the CreateDataPropertyOrThrow in the case when species returns anything other than an Array.

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

akroshg · 2018-05-11T21:38:20Z

+        let boundCallback = callbackfn;
+        if (thisArg === undefined) {
+            boundCallback = callbackfn;
+        } else {


should we remove the if part, as you have initialized boundCallback already?

akroshg · 2018-05-11T21:49:15Z

+                var errorBuiltIn = "Assignment to read-only properties is not allowed in strict mode"
                assert.throws(function () { Array.prototype.map.call(arr, function (a) { return a;}); }, TypeError, desc + "map", error);
-                assert.throws(function () { Array.prototype.filter.call(arr, function (a) { return true;}); }, TypeError,desc + "filter", error);
+                assert.throws(function () { Array.prototype.filter.call(arr, function (a) { return true;}); }, TypeError,desc + "filter", errorBuiltIn);


this shouldn't change, no? This may be happening due to CreateDataPropertyOrThrow in not there?

akroshg · 2018-05-11T21:57:18Z

        return obj;
    }

+    Var JsBuiltInEngineInterfaceExtensionObject::EntryJsBuiltIn_Internal_ArraySpeciesCreate(RecyclableObject *function, CallInfo callInfo, ...)


future todo : I think ArraySpeciesCreate should be implemented entirely in javascript. May be in future releases

akroshg · 2018-05-11T22:03:07Z

I believe the description should stats what is the motivation behind the change. Please share improvements on the micro-benchmark (crafted filter scenario should be fine).

jackhorton · 2018-05-11T23:04:44Z

+            }
+        }
+
+        return newObj;


Should this be returning newArr? if not, whats the point of newArr?

This code is essentially copied from JavascriptArray::FilterHelper. I don't have a good understanding of under what circumstances ArraySpeciesCreate would return a JavascriptCopyOnAccessNativeIntArray that needs conversion so I left that part of the logic the same. On closer inspection though, yeah, newArr is redundant and unnecessary.

jackhorton · 2018-05-11T23:08:17Z

    __chakraLibrary.raiseThis_NullOrUndefined = platform.raiseThis_NullOrUndefined;
+    __chakraLibrary.raiseFunctionArgument_NeedFunction = platform.raiseFunctionArgument_NeedFunction;
+    __chakraLibrary.callInstanceFunc = platform.builtInCallInstanceFunction;
+    __chakraLibrary.functionBind = platform.builtInJavascriptFunctionEntryBind;


What exactly is the goal here? Why does __chakraLibrary need these if you already have access to them through platform

We don't allow access to any functions that are not on __chakraLibrary from within the built in functions.

why not? Intl definitely doesn't have this restriction since it doesn't use __chakraLibrary at all.

rajatd · 2018-05-11T23:12:39Z

+        let to = 0;
+
+        // Binding: If thisArg is provided, we need to do the equivalent of callbackfn.call(thisArg, ...), but we cannot use .call because Function.prototype.call can be overriden.
+        // Unfortinately, locally recording the original Function.protoype.call would need to call originalCall.call(callbackfn, thisArg, ...), which has the same .call problem. 


Unfortinately [](start = 11, length = 13)

nit: spelling.

jackhorton · 2018-05-12T01:21:07Z

+  REM pushd %_reporoot%\lib\Runtime\Library\InJavascript
+  REM   call GenByteCode.cmd
+  REM   call GenByteCode.cmd -nojit
+  REM popd


jackhorton · 2018-05-12T01:23:10Z

+                if (k in o) {
+                    let kValue = o[k];
+                    if (callbackfn(kValue, k, o)) {
+                        a[to] = kValue;


Could you not still have an array that passes Array.isArray but has a non-writable property?

I believe you are correct, at the very least through modifications of Array.prototype.

rhuanjl · 2018-05-12T15:31:59Z

Thought on the slow path is it functionally equivalent to the fast path when in strict mode? Could there be a strict mode only optimisation to always use the fast path? Possibly blocked by: #5046

akroshg · 2018-05-14T18:14:06Z

+                if (k in o) {
+                    let kValue = o[k];
+                    if (callbackfn(kValue, k, o)) {
+                        a[to] = kValue;


this will still have the problem as Kevin mentioned before - it will call setter when we execute this, no?

Yeah, I'm about to update the PR with a version that always calls the __chakraLibrary helper, but I'm running test262 and perf results for it first

Merge pull request #5137 from sigatrev:jsFilter Implementing built-ins in JavaScript allows the JIT to optimize calls that must otherwise behave generically. This can provide significant performance improvements, particularly for array operations that loop over the array. In a microbenchmark calling filter 1000 times on a 10000 element array, the JS implementation was 13x faster in the best case, and 20% faster in the worst case. ``` for (var i = 0; i < 1000; ++i) { a.filter(()=>false) // best case, array is never grown } // js: 30 ms // c++: 400ms for (var i = 0; i < 1000; ++i) { a.filter(()=>true) // worst case } // js: 550 ms // c++: 700 ms ``` Adding a JIT fast path for __chakraLibrary.arrayCreateDataPropertyOrThrow similar to normal array assignment would drop the always-true case to 230 ms.

sigatrev requested review from jackhorton, meg-gupta and zenparsing May 11, 2018 18:56

zenparsing approved these changes May 11, 2018

View reviewed changes

meg-gupta approved these changes May 11, 2018

View reviewed changes

akroshg reviewed May 11, 2018

View reviewed changes

sethbrenith reviewed May 11, 2018

View reviewed changes

akroshg reviewed May 11, 2018

View reviewed changes

jackhorton reviewed May 11, 2018

View reviewed changes

rajatd reviewed May 11, 2018

View reviewed changes

sigatrev force-pushed the jsFilter branch from ef8df16 to 733049e Compare May 12, 2018 01:20

jackhorton reviewed May 12, 2018

View reviewed changes

sigatrev force-pushed the jsFilter branch from 733049e to d7d52e5 Compare May 12, 2018 04:20

akroshg reviewed May 14, 2018

View reviewed changes

sigatrev force-pushed the jsFilter branch 4 times, most recently from d849073 to 1ff7409 Compare May 15, 2018 01:58

implement Array.prototype.filter in javascript

c648792

sigatrev force-pushed the jsFilter branch from 1ff7409 to c648792 Compare May 15, 2018 02:21

chakrabot merged commit c648792 into chakra-core:master May 15, 2018

Uh oh!

Conversation

sigatrev commented May 11, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

zenparsing left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

meg-gupta left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

akroshg commented May 11, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

rhuanjl commented May 12, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

sigatrev commented May 11, 2018 •

edited

Loading