[MERGE #3454 @obastemur] Performance Improvements.

Merge pull request #3454 from obastemur:imps Total AcmeAir (LTO) perf. improvement 3% Changes: - Changes to JsHasProperty: This is a very hot method and ContextAPIWrapper is quite expensive. This change improves AcmeAir, 1% - Changes to PropertyRecord creation: Improves the performance of PropertyRecord by combining tasks. Improves AcmeAir 2%. Downside, `! (!isSymbol && length <= 10 && length > 0)` might turn out to be false positive and we allocate extra sizeof(uint32) space. - Centralize hashing logic
chakra-core · Aug 4, 2017 · 4559734 · 4559734
2 parents ac21f6c + 92adfa7
commit 4559734
Show file tree

Hide file tree

Showing 7 changed files with 69 additions and 32 deletions.
diff --git a/lib/Common/DataStructures/CharacterBuffer.h b/lib/Common/DataStructures/CharacterBuffer.h
@@ -68,8 +68,7 @@ namespace JsUtil
             }
             for (charcount_t i = 0; i < hashLength; i++)
             {
-                hash = _rotl(hash, 7);
-                hash ^= s[i];
+                CC_HASH_LOGIC(hash, s[i]);
             }
             return hash;
         }

diff --git a/lib/Common/DataStructures/Comparer.h b/lib/Common/DataStructures/Comparer.h
@@ -114,6 +114,10 @@ struct RecyclerPointerComparer
     }
 };
 
+#define CC_HASH_LOGIC(hash, byte) \
+    hash  = _rotl(hash, 7);        \
+    hash ^= byte;
+
 template <>
 struct DefaultComparer<GUID>
 {
@@ -128,8 +132,7 @@ struct DefaultComparer<GUID>
         int hash = 0;
         for (int i = 0; i < sizeof(GUID); i++)
         {
-            hash = _rotl(hash, 7);
-            hash ^= (uint32)(p[i]);
+            CC_HASH_LOGIC(hash, (uint32)(p[i]));
         }
         return hash;
      }
@@ -148,8 +151,7 @@ struct StringComparer
         int hash = 0;
         while (*str)
         {
-            hash = _rotl(hash, 7);
-            hash ^= *str;
+            CC_HASH_LOGIC(hash, *str);
             str++;
         }
         return hash;

diff --git a/lib/Common/DataStructures/InternalStringNoCaseComparer.cpp b/lib/Common/DataStructures/InternalStringNoCaseComparer.cpp
@@ -20,8 +20,7 @@ namespace JsUtil
         uint hash = 0;
         for (size_t i = 0; i < length; i++)
         {
-            hash = _rotl(hash, 7);
-            hash ^= tolower(s[i]);
+            CC_HASH_LOGIC(hash, tolower(s[i]));
         }
         return ((hash & 0x7fffffff) << 1) | 1;
     }

diff --git a/lib/Jsrt/Jsrt.cpp b/lib/Jsrt/Jsrt.cpp
@@ -1469,7 +1469,10 @@ CHAKRA_API JsSetProperty(_In_ JsValueRef object, _In_ JsPropertyIdRef propertyId
 
 CHAKRA_API JsHasProperty(_In_ JsValueRef object, _In_ JsPropertyIdRef propertyId, _Out_ bool *hasProperty)
 {
-    return ContextAPIWrapper<JSRT_MAYBE_TRUE>([&] (Js::ScriptContext *scriptContext, TTDRecorder& _actionEntryPopper) -> JsErrorCode {
+    VALIDATE_JSREF(object);
+    if (!Js::JavascriptOperators::IsObject(object)) return JsErrorArgumentNotObject;
+
+    auto internalHasProperty = [&] (Js::ScriptContext *scriptContext, TTDRecorder& _actionEntryPopper) -> JsErrorCode {
         PERFORM_JSRT_TTD_RECORD_ACTION(scriptContext, RecordJsRTHasProperty, (Js::PropertyRecord *)propertyId, object);
 
         VALIDATE_INCOMING_OBJECT(object, scriptContext);
@@ -1480,7 +1483,24 @@ CHAKRA_API JsHasProperty(_In_ JsValueRef object, _In_ JsPropertyIdRef propertyId
         *hasProperty = Js::JavascriptOperators::OP_HasProperty(object, ((Js::PropertyRecord *)propertyId)->GetPropertyId(), scriptContext) != 0;
 
         return JsNoError;
-    });
+    };
+
+    Js::RecyclableObject* robject = Js::RecyclableObject::FromVar(object);
+    Js::TypeId typeId = Js::JavascriptOperators::GetTypeId(robject);
+    while (typeId != Js::TypeIds_Null && typeId != Js::TypeIds_Proxy)
+    {
+        robject = robject->GetPrototype();
+        typeId = Js::JavascriptOperators::GetTypeId(robject);
+    }
+
+    if (typeId == Js::TypeIds_Proxy)
+    {
+        return ContextAPIWrapper<JSRT_MAYBE_TRUE>(internalHasProperty);
+    }
+    else
+    {
+        return ContextAPINoScriptWrapper(internalHasProperty);
+    }
 }
 
 CHAKRA_API JsDeleteProperty(_In_ JsValueRef object, _In_ JsPropertyIdRef propertyId, _In_ bool useStrictRules, _Out_ JsValueRef *result)

diff --git a/lib/Runtime/Base/PropertyRecord.cpp b/lib/Runtime/Base/PropertyRecord.cpp
@@ -21,6 +21,41 @@ namespace Js
     {
     }
 
+    PropertyRecord::PropertyRecord(const WCHAR* buffer, const int length, DWORD bytelength, bool isSymbol)
+        : pid(Js::Constants::NoProperty), isSymbol(isSymbol), byteCount(bytelength)
+    {
+        Assert(length >= 0 && buffer != nullptr);
+
+        WCHAR* target = (WCHAR*)((PropertyRecord*)this + 1);
+        isNumeric = (isSymbol || length > 10 || length <= 0) ? false : true;
+        hash = 0;
+
+        for (int i = 0; i < length; i++)
+        {
+            const WCHAR byte = buffer[i];
+            if (isNumeric)
+            {
+                if (byte < _u('0') || byte > _u('9'))
+                  isNumeric = false;
+            }
+
+            CC_HASH_LOGIC(hash, byte);
+            target[i] = byte;
+        }
+        target[length] = WCHAR(0);
+
+        if (isNumeric)
+        {
+            uint32 numericValue;
+            isNumeric = Js::PropertyRecord::IsPropertyNameNumeric(this->GetBuffer(), this->GetLength(), &numericValue);
+            if (isNumeric)
+            {
+                *(uint32 *)(this->GetBuffer() + this->GetLength() + 1) = numericValue;
+                Assert(GetNumericValue() == numericValue);
+            }
+        }
+    }
+
     void PropertyRecord::Finalize(bool isShutdown)
     {
         if (!isShutdown)

diff --git a/lib/Runtime/Base/PropertyRecord.h b/lib/Runtime/Base/PropertyRecord.h
@@ -43,6 +43,7 @@ namespace Js
 
         PropertyRecord(DWORD bytelength, bool isNumeric, uint hash, bool isSymbol);
         PropertyRecord(PropertyId pid, uint hash, bool isNumeric, DWORD byteCount, bool isSymbol);
+        PropertyRecord(const WCHAR* buffer, const int length, DWORD bytelength, bool isSymbol);
         PropertyRecord() { Assert(false); } // never used, needed by compiler for BuiltInPropertyRecord
 
         static bool IsPropertyNameNumeric(const char16* str, int length, uint32* intVal);

diff --git a/lib/Runtime/Base/ThreadContext.cpp b/lib/Runtime/Base/ThreadContext.cpp
@@ -1035,37 +1035,18 @@ ThreadContext::UncheckedAddPropertyId(JsUtil::CharacterBuffer<WCHAR> const& prop
 
     int length = propertyName.GetLength();
     uint bytelength = sizeof(char16) * length;
-
-    uint32 indexVal = 0;
-
-    // Symbol properties cannot be numeric since their description is not to be used!
-    bool isNumeric = !isSymbol && Js::PropertyRecord::IsPropertyNameNumeric(propertyName.GetBuffer(), propertyName.GetLength(), &indexVal);
-
-    uint hash = JsUtil::CharacterBuffer<WCHAR>::StaticGetHashCode(propertyName.GetBuffer(), propertyName.GetLength());
-
-    size_t allocLength = bytelength + sizeof(char16) + (isNumeric ? sizeof(uint32) : 0);
+    size_t allocLength = bytelength + sizeof(char16) + ( (!isSymbol && length <= 10 && length > 0) ? sizeof(uint32) : 0);
 
     // If it's bound, create it in the thread arena, along with a fake weak ref
     Js::PropertyRecord * propertyRecord;
     if (bind)
     {
-        propertyRecord = AnewPlus(GetThreadAlloc(), allocLength, Js::PropertyRecord, bytelength, isNumeric, hash, isSymbol);
+        propertyRecord = AnewPlus(GetThreadAlloc(), allocLength, Js::PropertyRecord, propertyName.GetBuffer(), length, bytelength, isSymbol);
         propertyRecord->isBound = true;
     }
     else
     {
-        propertyRecord = RecyclerNewFinalizedLeafPlus(recycler, allocLength, Js::PropertyRecord, bytelength, isNumeric, hash, isSymbol);
-    }
-
-    // Copy string and numeric info
-    char16* buffer = (char16 *)(propertyRecord + 1);
-    js_memcpy_s(buffer, bytelength, propertyName.GetBuffer(), bytelength);
-    buffer[length] = _u('\0');
-
-    if (isNumeric)
-    {
-        *(uint32 *)(buffer + length + 1) = indexVal;
-        Assert(propertyRecord->GetNumericValue() == indexVal);
+        propertyRecord = RecyclerNewFinalizedLeafPlus(recycler, allocLength, Js::PropertyRecord, propertyName.GetBuffer(), length, bytelength, isSymbol);
     }
 
     Js::PropertyId propertyId = this->GetNextPropertyId();