PropertyRecord: Improve creating PropertyRecord

Improves the performance of PropertyRecord by combining tasks. Improves AcmeAir aprox 2%. Downside, `(!isSymbol && length <= 10 && length > 0)` might turn out to be false positive and we allocate extra sizeof(uint32) space for string length 2, 3 and 10.
chakra-core · Jul 31, 2017 · 92adfa7 · 92adfa7
1 parent d7c0607
commit 92adfa7
Show file tree

Hide file tree

Showing 6 changed files with 48 additions and 31 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/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();
@@ -1741,7 +1722,7 @@ ThreadContext::ProbeStack(size_t size, Js::ScriptContext *scriptContext, PVOID r
         PlatformAgnostic::PerfTrace::WritePerfMap();
     }
 #endif
-    
+
     // BACKGROUND-GC TODO: If we're stuck purely in JITted code, we should have the
     // background GC thread modify the threads stack limit to trigger the runtime stack probe
     if (this->callDispose && this->recycler->NeedDispose())