chakrashim: Improve AddLocal codepath

`HandleScope` was simulated in chakrashim by saving references
in `JavascriptArray` which involved calling `JsSetIndexedProperty`
API. Although this API is not slow, but there is an overhead of
calling JSRT APIs. It regress performance if `AddLocal` is called
often. To avoid that, gathered elements in `_locals` stack and
then bulk pushed them to array.

* Earlier we called `JsCreateArray` with length 1 to create the
above array. This creates an array of head segment size 4
(rounding) but since we are pushing `kOnStackLocals`, we would
fill up the head segment and allocate newer segment. Instead if we
pass length 0 to `JsCreateArray` it creates a head segment of size
16 and we don't have to initialize another segment till count reaches
16.

* Also, I converted `Object::GetPropertyAttributes` to javascript
function which should save 6 JSRT calls. I have also added
`CHAKRA-TODO` comments for functions that should be easily
converted to javascript functions.

PR-URL: #101
Reviewed-By: Jianchun Xu <Jianchun.Xu@microsoft.com>

Author: kunalspathak

deps/chakrashim/include/v8.h

@@ -734,10 +734,11 @@ class V8_EXPORT HandleScope {
  private:
   friend class EscapableHandleScope;
   template <class T> friend class Local;
-  static const int kOnStackLocals = 5;  // Arbitrary number of refs on stack
+  static const int kOnStackLocals = 8;  // Arbitrary number of refs on stack
 
-  JsValueRef _locals[kOnStackLocals];   // Save some refs on stack
-  JsValueRef _refs;                     // More refs go to a JS array
+  // Save some refs on stack. 1st element on stack
+  // is the JavascriptArray where other refs go.
+  JsValueRef _locals[kOnStackLocals + 1];
   int _count;
   HandleScope *_prev;
   JsContextRef _contextRef;

deps/chakrashim/lib/chakra_shim.js

@@ -568,6 +568,26 @@
       // CHAKRA-TODO: Need to add JSRT API to detect this
       return false;
     };
+    utils.getPropertyAttributes = function(object, value) {
+      var descriptor = Object_getOwnPropertyDescriptor(object, value);
+      if (descriptor == undefined) {
+        return -1;
+      }
+
+      var attributes = 0;
+      // taken from v8.h. Update if this changes in future
+      const ReadOnly = 1, DontEnum = 2, DontDelete = 4;
+      if (!descriptor.writable) {
+        attributes |= ReadOnly;
+      }
+      if (!descriptor.enumerable) {
+        attributes |= DontEnum;
+      }
+      if (!descriptor.configurable) {
+        attributes |= DontDelete;
+      }
+      return attributes;
+    };
   }
 
   patchErrorTypes();

deps/chakrashim/src/jsrtcachedpropertyidref.inc

@@ -32,6 +32,7 @@
 
 DEF(apply)
 DEF(concat)
+DEF(push)
 DEF(construct)
 DEF(constructor)
 DEF(defineProperty)
@@ -72,6 +73,8 @@ DEF(getSymbolFor)
 DEF(ensureDebug)
 DEF(enqueueMicrotask)
 DEF(dequeueMicrotask)
+DEF(saveInHandleScope)
+DEF(getPropertyAttributes)
 DEF(getFunctionName)
 DEF(getFileName)
 DEF(getColumnNumber)

deps/chakrashim/src/jsrtcontextcachedobj.inc

@@ -33,6 +33,7 @@ DEFTYPE(Number)
 DEFTYPE(Object)
 DEFTYPE(Proxy)
 DEFTYPE(String)
+DEFTYPE(Array)
 DEFTYPE(Uint8Array)
 DEFTYPE(Uint8ClampedArray)
 DEFTYPE(Int8Array)
@@ -46,9 +47,12 @@ DEFTYPE(Float64Array)
 
 
 // These prototype functions will be cached/shimmed
+DEFMETHOD(Object,         getOwnPropertyDescriptor)
 DEFMETHOD(Object,         hasOwnProperty)
 DEFMETHOD(Object,         toString)
 DEFMETHOD(String,         concat)
+DEFMETHOD(Array,          push)
+
 
 #undef DEFTYPE
 #undef DEFMETHOD

deps/chakrashim/src/jsrtcontextshim.cc

@@ -73,7 +73,6 @@ ContextShim::ContextShim(IsolateShim * isolateShim,
 #undef DEF_IS_TYPE
       cloneObjectFunction(JS_INVALID_REFERENCE),
       getPropertyNamesFunction(JS_INVALID_REFERENCE),
-      getOwnPropertyDescriptorFunction(JS_INVALID_REFERENCE),
       getEnumerableNamedPropertiesFunction(JS_INVALID_REFERENCE),
       getEnumerableIndexedPropertiesFunction(JS_INVALID_REFERENCE),
       createEnumerationIteratorFunction(JS_INVALID_REFERENCE),
@@ -86,7 +85,8 @@ ContextShim::ContextShim(IsolateShim * isolateShim,
       getSymbolForFunction(JS_INVALID_REFERENCE),
       ensureDebugFunction(JS_INVALID_REFERENCE),
       enqueueMicrotaskFunction(JS_INVALID_REFERENCE),
-      dequeueMicrotaskFunction(JS_INVALID_REFERENCE) {
+      dequeueMicrotaskFunction(JS_INVALID_REFERENCE),
+      getPropertyAttributesFunction(JS_INVALID_REFERENCE) {
   memset(globalConstructor, 0, sizeof(globalConstructor));
   memset(globalPrototypeFunction, 0, sizeof(globalPrototypeFunction));
 }
@@ -302,16 +302,6 @@ bool ContextShim::InitializeBuiltIns() {
     return false;
   }
 
-  if (!InitializeBuiltIn(&getOwnPropertyDescriptorFunction,
-                         [this](JsValueRef * value) {
-                           return GetProperty(
-                             GetObjectConstructor(),
-                             CachedPropertyIdRef::getOwnPropertyDescriptor,
-                             value);
-                         })) {
-    return false;
-  }
-
   if (DefineProperty(globalObject,
                      GetIsolateShim()->GetKeepAliveObjectSymbolPropertyIdRef(),
                      PropertyDescriptorOptionValues::False,
@@ -516,10 +506,14 @@ DECLARE_GETOBJECT(DateConstructor,
 DECLARE_GETOBJECT(ProxyConstructor,
                   globalConstructor[GlobalType::Proxy])
 DECLARE_GETOBJECT(GetOwnPropertyDescriptorFunction,
-                  getOwnPropertyDescriptorFunction)
+                  globalPrototypeFunction[GlobalPrototypeFunction
+                      ::Object_getOwnPropertyDescriptor])
 DECLARE_GETOBJECT(StringConcatFunction,
                   globalPrototypeFunction[GlobalPrototypeFunction
                     ::String_concat])
+DECLARE_GETOBJECT(ArrayPushFunction,
+                  globalPrototypeFunction[GlobalPrototypeFunction
+                    ::Array_push])
 
 
 JsValueRef ContextShim::GetProxyOfGlobal() {
@@ -571,6 +565,7 @@ CHAKRASHIM_FUNCTION_GETTER(getSymbolFor)
 CHAKRASHIM_FUNCTION_GETTER(ensureDebug)
 CHAKRASHIM_FUNCTION_GETTER(enqueueMicrotask);
 CHAKRASHIM_FUNCTION_GETTER(dequeueMicrotask);
+CHAKRASHIM_FUNCTION_GETTER(getPropertyAttributes);
 
 #define DEF_IS_TYPE(F) CHAKRASHIM_FUNCTION_GETTER(F)
 #include "jsrtcachedpropertyidref.inc"

deps/chakrashim/src/jsrtcontextshim.h

@@ -72,6 +72,7 @@ class ContextShim {
   JsValueRef GetGlobalType(GlobalType index);
   JsValueRef GetGetOwnPropertyDescriptorFunction();
   JsValueRef GetStringConcatFunction();
+  JsValueRef GetArrayPushFunction();
   JsValueRef GetGlobalPrototypeFunction(GlobalPrototypeFunction index);
   JsValueRef GetProxyOfGlobal();
 
@@ -124,10 +125,10 @@ class ContextShim {
   std::vector<void*> embedderData;
 
 #define DECLARE_CHAKRASHIM_FUNCTION_GETTER(F) \
- public: \
-   JsValueRef Get##F##Function(); \
- private: \
-   JsValueRef F##Function; \
+public: \
+  JsValueRef Get##F##Function(); \
+private: \
+  JsValueRef F##Function; \
 
   DECLARE_CHAKRASHIM_FUNCTION_GETTER(cloneObject);
   DECLARE_CHAKRASHIM_FUNCTION_GETTER(getPropertyNames);
@@ -144,6 +145,7 @@ class ContextShim {
   DECLARE_CHAKRASHIM_FUNCTION_GETTER(ensureDebug);
   DECLARE_CHAKRASHIM_FUNCTION_GETTER(enqueueMicrotask);
   DECLARE_CHAKRASHIM_FUNCTION_GETTER(dequeueMicrotask);
+  DECLARE_CHAKRASHIM_FUNCTION_GETTER(getPropertyAttributes);
 
 #define DEF_IS_TYPE(F) DECLARE_CHAKRASHIM_FUNCTION_GETTER(F)
 #include "jsrtcachedpropertyidref.inc"

deps/chakrashim/src/jsrtutils.cc

@@ -282,102 +282,6 @@ JsErrorCode HasOwnProperty(JsValueRef object,
   return JsCallFunction(hasOwnPropertyFunction, args, _countof(args), result);
 }
 
-JsErrorCode IsValueInArray(
-    JsValueRef arrayRef,
-    JsValueRef valueRef,
-    std::function<JsErrorCode(JsValueRef, JsValueRef, bool*)> comparator,
-    bool* result) {
-  JsErrorCode error;
-  unsigned int length;
-  *result = false;
-  error = GetArrayLength(arrayRef, &length);
-  if (error != JsNoError) {
-    return error;
-  }
-
-  for (unsigned int index = 0; index < length; index++) {
-    JsValueRef indexValue;
-    error = JsIntToNumber(index, &indexValue);
-    if (error != JsNoError) {
-      return error;
-    }
-
-    JsValueRef itemRef;
-    error = JsGetIndexedProperty(arrayRef, indexValue, &itemRef);
-    if (error != JsNoError) {
-      return error;
-    }
-
-    if (comparator != nullptr) {
-      error = comparator(valueRef, itemRef, result);
-    } else {
-      error = JsEquals(itemRef, valueRef, result);
-    }
-
-    if (error != JsNoError) {
-      return error;
-    }
-
-    if (*result)
-      return JsNoError;
-  }
-
-  return JsNoError;
-}
-
-JsErrorCode IsValueInArray(JsValueRef arrayRef,
-                           JsValueRef valueRef,
-                           bool* result) {
-  return IsValueInArray(arrayRef, valueRef, nullptr, result);
-}
-
-JsErrorCode IsCaseInsensitiveStringValueInArray(JsValueRef arrayRef,
-                                                JsValueRef valueRef,
-                                                bool* result) {
-  return IsValueInArray(arrayRef, valueRef, [=](
-      JsValueRef first, JsValueRef second, bool* areEqual) -> JsErrorCode {
-    JsValueType type;
-    *areEqual = false;
-
-    JsErrorCode error = JsGetValueType(first, &type);
-    if (error != JsNoError) {
-      return error;
-    }
-    if (type != JsString) {
-      return JsNoError;
-    }
-
-    error = JsGetValueType(second, &type);
-    if (error != JsNoError) {
-      return error;
-    }
-    if (type != JsString) {
-      return JsNoError;
-    }
-
-    const wchar_t* firstPtr;
-    size_t firstLength;
-
-    error = JsStringToPointer(first, &firstPtr, &firstLength);
-    if (error != JsNoError) {
-      return error;
-    }
-
-    const wchar_t* secondPtr;
-    size_t secondLength;
-
-    error = JsStringToPointer(second, &secondPtr, &secondLength);
-    if (error != JsNoError) {
-      return error;
-    }
-
-    size_t maxCount = min(firstLength, secondLength);
-    *areEqual = (_wcsnicmp(firstPtr, secondPtr, maxCount) == 0);
-    return JsNoError;
-  },
-                        result);
-}
-
 JsErrorCode GetOwnPropertyDescriptor(JsValueRef ref,
                                      JsValueRef prop,
                                      JsValueRef* result) {
@@ -571,6 +475,7 @@ PropertyDescriptorOptionValues GetPropertyDescriptorOptionValue(bool b) {
     PropertyDescriptorOptionValues::False;
 }
 
+// CHAKRA-TODO: Convert this function to javascript
 JsErrorCode CreatePropertyDescriptor(
     PropertyDescriptorOptionValues writable,
     PropertyDescriptorOptionValues enumerable,
@@ -911,6 +816,7 @@ bool DeletePrivate(JsValueRef object, JsValueRef key) {
   return hasDeleted;
 }
 
+// CHAKRA-TODO: Convert this function to javascript
 JsErrorCode GetPrivate(JsValueRef object, JsValueRef key,
                        JsValueRef *result) {
   JsPropertyIdRef hiddenValuesIdRef;

deps/chakrashim/src/jsrtutils.h

@@ -101,20 +101,6 @@ JsErrorCode GetOwnPropertyDescriptor(JsValueRef ref,
                                      JsValueRef prop,
                                      JsValueRef* result);
 
-JsErrorCode IsValueInArray(JsValueRef arrayRef,
-                           JsValueRef valueRef,
-                           bool* result);
-
-JsErrorCode IsValueInArray(
-  JsValueRef arrayRef,
-  JsValueRef valueRef,
-  std::function<JsErrorCode(JsValueRef, JsValueRef, bool*)> comperator,
-  bool* result);
-
-JsErrorCode IsCaseInsensitiveStringValueInArray(JsValueRef arrayRef,
-                                                JsValueRef valueRef,
-                                                bool* result);
-
 JsErrorCode IsZero(JsValueRef value,
                    bool *result);
 

deps/chakrashim/src/v8chakra.cc

@@ -37,6 +37,7 @@ bool TemplateData::Is(ExternalData* data) {
       || data->GetType() == ExternalDataTypes::FunctionTemplateData;
 }
 
+// CHAKRA-TODO: Convert this function to javascript?
 // Clone the template properties into the new instance
 JsErrorCode TemplateData::CopyPropertiesTo(JsValueRef newInstance) {
   if (properties.IsEmpty()) {
@@ -84,7 +85,7 @@ JsErrorCode TemplateData::CopyPropertiesTo(JsValueRef newInstance) {
       TemplateData* templateData = static_cast<TemplateData*>(data);
       value = templateData->NewInstance(value);
       if (value == JS_INVALID_REFERENCE) {
-        return JsErrorFatal; // Just to indicate failed
+        return JsErrorFatal;  // Just to indicate failed
       }
       IfJsErrorRet(JsSetProperty(propertyDescriptor, valueIdRef, value, false));
     }

deps/chakrashim/src/v8handlescope.cc

@@ -28,10 +28,10 @@ __declspec(thread) HandleScope *current = nullptr;
 HandleScope::HandleScope(Isolate* isolate)
     : _prev(current),
       _locals(),
-      _refs(JS_INVALID_REFERENCE),
       _count(0),
       _contextRef(JS_INVALID_REFERENCE),
       _addRefRecordHead(nullptr) {
+  _locals[0] = JS_INVALID_REFERENCE;
   current = this;
 }
 
@@ -54,28 +54,25 @@ HandleScope *HandleScope::GetCurrent() {
 }
 
 bool HandleScope::AddLocal(JsValueRef value) {
-  if (_count < _countof(_locals)) {
-    _locals[_count++] = value;
-    return true;
-  }
-
-  if (_refs == JS_INVALID_REFERENCE) {
-    if (JsCreateArray(1, &_refs) != JsNoError) {
-      return AddLocalAddRef(value);
+  // _locals is full, save them in _locals[0]
+  if (_count == kOnStackLocals) {
+    if (_locals[0] == JS_INVALID_REFERENCE) {
+      if (JsCreateArray(0, &_locals[0]) != JsNoError) {
+        return AddLocalAddRef(value);
+      }
     }
-  }
 
-  JsValueRef index;
+    JsValueRef arrayPushFunction =
+        jsrt::ContextShim::GetCurrent()->GetArrayPushFunction();
 
-  if (JsIntToNumber(_count - _countof(_locals), &index) != JsNoError) {
-    return AddLocalAddRef(value);
-  }
-
-  if (JsSetIndexedProperty(_refs, index, value) != JsNoError) {
-    return AddLocalAddRef(value);
+    JsValueRef result;
+    if (JsCallFunction(arrayPushFunction, _locals,
+                       _countof(_locals), &result) != JsNoError) {
+      return AddLocalAddRef(value);
+    }
+    _count = 0;
   }
-
-  _count++;
+  _locals[1 + _count++] = value;
   return true;
 }