Implement Array Iterators in javascript #4095

meg-gupta · 2017-10-30T21:05:59Z

This change adds implementation of Javascript's array iterators in Javascript.
This combined with optimization of try finally functions and enabling inlining within functions have try-catch/try-finally improves performance of es6 for-of loops.
For large for-of loops the speedup is more than 2.5x.

meg-gupta · 2017-10-30T21:07:35Z

Commits on top of "Merge conflict errors " are new code. Previous commits have been reviewed before in the builtins branch.

akroshg · 2017-10-31T00:19:24Z

lib/Runtime/Library/JsBuiltIn/JsBuiltIn.js

+                }
+
+                let a = o.arrayObj;
+                var value, done;


var value, done; [](start = 16, length = 16)

initialize this as undefined and true, then you don't need to have it set two places below?

Nit: use let instead of var

@akroshg The jit will copy prop the values in either cases. I don't want to add an unconditional write which need not happen.

akroshg · 2017-10-31T01:30:04Z

lib/Runtime/Library/JsBuiltIn/JsBuiltIn.js

+                    throw new TypeError("Array Iterator.prototype.next: 'this' is not an Array Iterator object");
+                }
+
+                let a = o.arrayObj;


arrayObj [](start = 26, length = 8)

these are enumerable and visible to the script.

Changed to non - enumerable for now.

obastemur · 2017-10-31T13:07:57Z

Looks like, there are changes related to OS9049104

MikeHolman · 2017-10-31T17:36:32Z

lib/Runtime/Library/JsBuiltIn/JsBuiltIn.js

+                }
+                else {
+                    let index = o.nextIndex;
+                    let len = Array.isArray(a) ? a.length : __chakraLibrary.GetArrayLength(a);


What happens if Array.isArray is overridden?

That'd be bad. Will need an alternative. Thanks.

MikeHolman · 2017-10-31T17:40:06Z

lib/Runtime/Library/JsBuiltIn/JsBuiltIn.js

+                }
+
+                let a = o.arrayObj;
+                var value, done;


Nit: use let instead of var

MikeHolman · 2017-10-31T17:46:47Z

lib/Runtime/Library/JsBuiltIn/JsBuiltIn.js

+                }
+
+                if (!(o instanceof __chakraLibrary.ArrayIterator)) {
+                    throw new TypeError("Array Iterator.prototype.next: 'this' is not an Array Iterator object");


Can user override TypeError?

Yes. Will need an alternative here. thanks.

jackhorton · 2017-10-31T20:57:43Z

lib/Runtime/Library/JsBuiltIn/JsBuiltIn.js

+
+                        o.nextIndex = index + 1;
+
+                        if (itemKind === 0 /*ArrayIterationKind.Key*/) { // TODO (megupta) : Use clean enums here ?


Using enums is the approach @dilijev and I have taken on the Intl.js side of things, I think its much cleaner.

jackhorton · 2017-10-31T21:04:48Z

lib/Runtime/Library/JsBuiltIn/JsBuiltIn.js

+    var ObjectDefineProperty = function (obj, prop, attributes) {
+        _objectDefineProperty(obj, prop, setPrototype(attributes, null));
+    };
+    var CreateObject = platform.builtInJavascriptObjectCreate;


Is there a way that we can simplify these aliases for all JsBuiltIns going forward? Like, perhaps we could make the names shorter or have a global standard library written in Javascript that all of these implementation files could use? Something along the lines of https://github.com/Microsoft/ChakraCore/pull/4083/files#diff-fa23cc60499db424efee4a57ac9bafddR105 would be my hope. Having everyone use _.keys or _.defineProperty rather than each files own (hopefully not but possibly) unique alias seems like it would make writing these JsBuiltIns much easier, especially for newer contributors.

jackhorton · 2017-10-31T21:06:37Z

lib/Runtime/Library/JavascriptLibrary.cpp

+#pragma warning(disable:4309) // truncation of constant value
+#pragma warning(disable:4838) // conversion from 'int' to 'const char' requires a narrowing conversion
+
+#pragma warning(pop)


Is this intentional? it looks like the warning setup for including the bytecode, but as far as I know this is a no-op right now?

Inherited this from the builtins branch and forgot to clean it up! Thanks.

jackhorton · 2017-10-31T21:12:36Z

lib/Runtime/Library/JsBuiltInEngineInterfaceExtensionObject.cpp

+    /*
+    * First parameter is the object onto which prototype should be set; second is the value
+    */
+    Var JsBuiltInEngineInterfaceExtensionObject::EntryJsBuiltIn_Internal_SetPrototype(RecyclableObject *function, CallInfo callInfo, ...)


Is there any way we can share native helpers like this between JsBuiltIns and Intl.js? I know there is a similar function in the Intl extension object.

I can try moving it to EngineInterfaceObject::InitializeCommonNativeInterfaces.

I am going to address this as a separate PR. Also - #3962 is incomplete and requires some special handling for EnginerInterfaceObject's deferred type handler.

akroshg · 2017-10-31T21:44:08Z

lib/Runtime/Library/JsBuiltIn/JsBuiltIn.js

+
+                        o.nextIndex = index + 1;
+
+                        if (itemKind === 0 /*ArrayIterationKind.Key*/) { // TODO (megupta) : Use clean enums here ?


give .Value more priority over .Key

akroshg · 2017-10-31T21:47:09Z

test/DebuggerCommon/parentedDynamicCode3.js.dbg.baseline

        "scriptType": "eval code",
        "lineCount": 1,
-        "sourceLength": 11
+        "sourceLength": 11,
+        "scriptId": 5,


why this change? Is it due to ChakraLib.js?

Due to JsBuiltIn.js

akroshg · 2017-10-31T21:51:40Z

lib/Runtime/Library/JavascriptFunction.h

@@ -42,6 +42,8 @@ namespace Js
        // Need a constructor cache on every function (script and native) to avoid extra checks on the fast path, if the function isn't fixed.
        Field(ConstructorCache*) constructorCache;

+        Field(bool) isJsBuiltInCode;


isJsBuiltInCode [](start = 20, length = 15)

I don't see this field is initialized anywhere? did you do that somewhere?

This was inherited from the builtins branch. The constructor initialization was missing. It is set in the register* functions and used in a few places. Thanks for catching this!

akroshg · 2017-10-31T21:55:38Z

lib/Runtime/Library/JsBuiltInEngineInterfaceExtensionObject.cpp

+
+        if (callInfo.Count < 2)
+        {
+            return scriptContext->GetLibrary()->GetUndefined();


We don't expect this one right?, the count will always be greater then 2? Should we assert and return 0 instead of undefined?

Right, converted to assert.

akroshg · 2017-10-31T21:57:38Z

lib/Runtime/Library/JsBuiltInEngineInterfaceExtensionObject.cpp

+
+    bool JsBuiltInEngineInterfaceExtensionObject::InitializeJsBuiltInNativeInterfaces(DynamicObject * builtInNativeInterfaces, DeferredTypeHandlerBase * typeHandler, DeferredInitializeMode mode)
+    {
+        typeHandler->Convert(builtInNativeInterfaces, mode, 16);


16 [](start = 60, length = 2)

curious why 16?

Its the initSlotCapacity for the SimpleDictionaryTypeHandler. Changed it to 4, I think that suffices.

akroshg · 2017-10-31T21:58:53Z

lib/Runtime/Library/JsBuiltInEngineInterfaceExtensionObject.h

+        public:
+            static NoProfileFunctionInfo JsBuiltIn_RegisterChakraLibraryFunction;
+            static NoProfileFunctionInfo JsBuiltIn_RegisterFunction;
+            static NoProfileFunctionInfo JsBuiltIn_Internal_GetArrayLengthFunction;


GetLength instead of GetArrayLength (as we are fetching the 'length' property).

meg-gupta · 2017-11-03T17:25:14Z

I have removed the bug fix at #4051 from the PR's commits.

akroshg · 2017-11-03T17:32:12Z

lib/Runtime/Library/JsBuiltInEngineInterfaceExtensionObject.cpp

+        DynamicObject* obj = DynamicObject::FromVar(args.Values[1]);
+        unsigned propCount = TaggedInt::ToUInt32(args.Values[2]);
+
+        Assert(callInfo.Count == 3 + propCount);


you don't need this, right? this function can be used for other functions in the future.

Assert will stay the same for all new functions as well. 3 here is this + object on which we want to set property attributes + count of properties

akroshg · 2017-11-03T17:41:46Z

lib/Runtime/Library/JsBuiltIn/JsBuiltIn.js

+    platform.registerChakraLibraryFunction("ArrayIterator", function (arrayObj, iterationKind) {
+        "use strict";
+        __chakraLibrary.InitInternalProperties(this, 4, "__$arrayObj$__", "__$nextIndex$__", "__$kind$__", "__$internalDone$__");
+        this.__$arrayObj$__ = arrayObj;


this.$arrayObj$ [](start = 8, length = 19)

make sure you speak with Louis/Ed about this.

Done. Got approval.

akroshg

…tests.

…ebugger trace. Also removed the "arguments" and "caller" property to be spec compliant. I opened an issue about the fact that the built ins methods in C++ are not spec compliant : chakra-core#3043

… the code from the following issue : chakra-core#3140 So BEFORE merging this Pull Request with the Master branch WE MUST fix the issue 3140.

…tensionObject.

…onfiguration. - fixed the source context id of the JS built ins.

- Remove IndexOf - Remove helper C++ functions toLength and toInteger - Remove IsIn fastpath : found errors in exprgen

dilijev · 2017-11-03T22:43:16Z

lib/Runtime/Library/JsBuiltIn/JsBuiltIn.js

+        return __chakraLibrary.CreateArrayIterator(o, 2 /* ArrayIterationKind.KeyAndValue*/);
+    });
+
+});


; [](start = 2, length = 1)

nit: newline at end of file and unnecessary blank line above

dilijev · 2017-11-03T23:00:43Z

lib/Runtime/Library/JsBuiltIn/JsBuiltIn.js

+            throw new TypeError("Array.prototype.keys: 'this' is null or undefined");
+        }
+        let o = __chakraLibrary.Object(this);
+        return __chakraLibrary.CreateArrayIterator(o, 0 /* ArrayIterationKind.Key*/);


0 /* ArrayIterationKind.Key*/ [](start = 54, length = 29)

Can these magic constants be turned into named constants or and/or members of the Enum?

I cannot declare these constants within the global function we use to set up the library. It should be stored on the chakraLib object. Its not neat, but I want to avoid storing constants on the chakraLib object.

dilijev · 2017-11-03T23:06:31Z

lib/Runtime/Library/JsBuiltIn/JsBuiltIn.js

+                        if (itemKind === 1 /*ArrayIterationKind.Value*/) {
+                            value = a[index];
+                        }
+                        else if (itemKind === 0 /*ArrayIterationKind.Key*/) { // TODO (megupta) : Use clean enums here ?


At this point we probably need to write a JS coding guideline. FWIW the prevailing pattern in library JS is to do } else if (...) { on a single line (no newline between } and else).

Done. I am squashing CR suggestions with my prior commit, please bear with it!

Merge pull request #4095 from meg-gupta:jsarrayitnodelay This change adds implementation of Javascript's array iterators in Javascript. This combined with optimization of try finally functions and enabling inlining within functions have try-catch/try-finally improves performance of es6 for-of loops. For large for-of loops the speedup is more than 2.5x.

akroshg reviewed Oct 31, 2017

View reviewed changes

meg-gupta requested review from akroshg and MikeHolman October 31, 2017 00:21

akroshg reviewed Oct 31, 2017

View reviewed changes

MikeHolman reviewed Oct 31, 2017

View reviewed changes

jackhorton reviewed Oct 31, 2017

View reviewed changes

akroshg reviewed Oct 31, 2017

View reviewed changes

meg-gupta force-pushed the jsarrayitnodelay branch 9 times, most recently from 4772c90 to f971233 Compare November 3, 2017 17:23

akroshg reviewed Nov 3, 2017

View reviewed changes

akroshg approved these changes Nov 3, 2017

View reviewed changes

Etienne BAUDOUX added 3 commits November 3, 2017 14:29

Implement the prototype of JS built in Array.prototype.indexOf

a969f6a

regenerated all byte code.

c13342a

bug fixed with jshost => the baseline was wrong in the heapenum unit …

93109a7

…tests.

Etienne BAUDOUX and others added 13 commits November 3, 2017 14:29

Fixed a bug where the length property was showing up 2 times in the d…

8e6b81f

…ebugger trace. Also removed the "arguments" and "caller" property to be spec compliant. I opened an issue about the fact that the built ins methods in C++ are not spec compliant : chakra-core#3043

Making the JS Built-in inlining.

f49682b

Refactoring and minor bug fixed. Performances only good if we comment…

de8facf

… the code from the following issue : chakra-core#3140 So BEFORE merging this Pull Request with the Master branch WE MUST fix the issue 3140.

Fixed a bug where indexOf was failing when it was called by Intl.

f9365f5

Removed deprecated code.

4703b09

Bug fixed + refactoring

2b6e8dc

Changed a few Assert to AsserOrFailFast in JsBuiltInEngineInterfaceEx…

ee7ccae

…tensionObject.

- added the support of "forceInline" parameter in the JS Built In's c…

1cf6ca4

…onfiguration. - fixed the source context id of the JS built ins.

Fixed a build error on GitHub.

516671f

Refactoring

66ec420

Removed a useless cast.

61cfb35

Merge conflict errors

c654bef

Remove functions that will not be used

fc4d5f3

- Remove IndexOf - Remove helper C++ functions toLength and toInteger - Remove IsIn fastpath : found errors in exprgen

meg-gupta force-pushed the jsarrayitnodelay branch 2 times, most recently from b664d1e to 5b5df92 Compare November 3, 2017 21:42

dilijev reviewed Nov 3, 2017

View reviewed changes

JS implementation for array iterator

32196f0

meg-gupta force-pushed the jsarrayitnodelay branch from 5b5df92 to 32196f0 Compare November 3, 2017 23:20

chakrabot merged commit 32196f0 into chakra-core:master Nov 4, 2017


		o.nextIndex = index + 1;

		if (itemKind === 0 /ArrayIterationKind.Key/) { // TODO (megupta) : Use clean enums here ?

Implement Array Iterators in javascript #4095

Implement Array Iterators in javascript #4095

Conversation

meg-gupta commented Oct 30, 2017 • edited Loading

meg-gupta commented Oct 30, 2017

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

obastemur commented Oct 31, 2017

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

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

meg-gupta commented Nov 3, 2017

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

meg-gupta commented Oct 30, 2017 •

edited

Loading