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

lib/Runtime/Library/JsBuiltIn/JsBuiltIn.js

+
+                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

lib/Runtime/Library/JsBuiltIn/JsBuiltIn.js

+
+        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

lib/Runtime/Library/JsBuiltInEngineInterfaceExtensionObject.cpp

+    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

lib/Runtime/Library/JsBuiltInEngineInterfaceExtensionObject.cpp

+
+        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

lib/Runtime/Library/JsBuiltIn/JsBuiltIn.js

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


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

+g

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

lib/Runtime/Library/JsBuiltIn/JsBuiltIn.js

+                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

lib/Runtime/Library/JsBuiltIn/JsBuiltIn.js

+        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

test/es6/ES6ArrayAPI.js

                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

lib/Runtime/Library/JsBuiltInEngineInterfaceExtensionObject.cpp

@@ -453,5 +460,34 @@ namespace Js
        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

lib/Runtime/Library/JsBuiltInEngineInterfaceExtensionObject.cpp

+            }
+        }
+
+        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

lib/Runtime/Library/JsBuiltIn/JsBuiltIn.js

@@ -35,6 +36,9 @@
    __chakraLibrary.ArrayIterator.prototype = CreateObject(iteratorPrototype);
    __chakraLibrary.raiseNeedObjectOfType = platform.raiseNeedObjectOfType;
    __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

lib/Runtime/Library/JsBuiltIn/JsBuiltIn.js

+        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

RegenAllByteCodeNoBuild.cmd

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


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

lib/Runtime/Library/JsBuiltIn/JsBuiltIn.js

+                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

lib/Runtime/Library/JsBuiltIn/JsBuiltIn.js

+                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, zenparsing and meg-gupta 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

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

implement Array.prototype.filter in javascript #5137

implement Array.prototype.filter in javascript #5137

sigatrev commented May 11, 2018 •

edited

Loading

zenparsing left a comment

zenparsing May 11, 2018

meg-gupta left a comment

akroshg May 11, 2018

sigatrev May 11, 2018

akroshg May 11, 2018

akroshg May 11, 2018

sigatrev May 11, 2018

akroshg May 11, 2018

akroshg May 11, 2018

sigatrev May 11, 2018

sethbrenith May 11, 2018

akroshg May 11, 2018

sigatrev May 11, 2018

zenparsing May 11, 2018

akroshg May 11, 2018

akroshg May 11, 2018

akroshg May 11, 2018

akroshg May 11, 2018

akroshg commented May 11, 2018

jackhorton May 11, 2018

sigatrev May 12, 2018

jackhorton May 11, 2018

sigatrev May 11, 2018

jackhorton May 11, 2018

rajatd May 11, 2018

sigatrev May 11, 2018

jackhorton May 12, 2018

jackhorton May 12, 2018

sigatrev May 13, 2018

rhuanjl commented May 12, 2018

akroshg May 14, 2018

sigatrev May 14, 2018

implement Array.prototype.filter in javascript #5137

implement Array.prototype.filter in javascript #5137

Conversation

sigatrev commented May 11, 2018 • edited Loading

zenparsing left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

meg-gupta left a 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

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

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

akroshg commented May 11, 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

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

rhuanjl commented May 12, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sigatrev commented May 11, 2018 •

edited

Loading