[MERGE #1368 @kunalspathak] Improve CheckIfTypeIsEquivalent

Merge pull request #1368 from kunalspathak:typecheck

`CheckIfTypeIsEquivalent` helper is called from JITted code to perform equivalent type check.
It validates if the object's type is present in one of the 8 cached equivalent
types and if yes, updates the guard value and return back to JITted code.
If not, it takes slow path and verifies the prototype of object to that of cached
type. However if guard is not valid, we would just bailout `BailOutFailedEquivalentTypeCheck`.

Today in `PreSweepCallback` we would invalidate the guard if the underline type is not marked.
However we could have invalid guard value but if there are alive types present in equivTypes[], we could
use them to check if `type` in question matches any of them and if yes, update the guard value, so next
time JIT code will be able to take advantage of it next time.

On instrumenting code, noticed that there are high frequency that this happens (at least in acmeair) where
we have valid equivTypes[], but we don'take take its advantage just because guard value is invalid.
This gives approx. 2.5 % win in acmeair. Other benchmarks are flat.

Also, I noticed that the cache eviction policy was kicking out valid caches
despite of having empty slots in types[] where new type could have been
stored. Modified the eviction logic to take advantage of empty slots before
going to existing eviction logic.

I have also added logic to compact the equivTypes by moving non-null types towards front of array.

Test: Internal tools pass
Perf: Standard benchmarks are almost flat with minor improvement in Octane, Kraken. Acme-air shows 2.5% win.

Author: kunalspathak

lib/Runtime/Base/FunctionBody.cpp

@@ -8775,14 +8775,17 @@ namespace Js
             Type *type = reinterpret_cast<Type*>(this->guard->GetValue());
             if (!recycler->IsObjectMarked(type))
             {
-                this->guard->Invalidate();
+                this->guard->InvalidateDuringSweep();
             }
             else
             {
                 isAnyTypeLive = true;
             }
         }
-
+        uint16 nonNullIndex = 0;
+#if DBG
+        bool isGuardValuePresent = false;
+#endif
         for (int i = 0; i < EQUIVALENT_TYPE_CACHE_SIZE; i++)
         {
             Type *type = this->types[i];
@@ -8794,11 +8797,29 @@ namespace Js
                 }
                 else
                 {
+                    // compact the types array by moving non-null types
+                    // at the beginning.
+                    this->types[nonNullIndex++] = type;
                     isAnyTypeLive = true;
+#if DBG
+                    isGuardValuePresent = this->guard->GetValue() == reinterpret_cast<intptr_t>(type) ? true : isGuardValuePresent;
+#endif
                 }
             }
         }
+        if (nonNullIndex > 0)
+        {
+            memset((void*)(this->types + nonNullIndex), 0, sizeof(Js::Type*) * (EQUIVALENT_TYPE_CACHE_SIZE - nonNullIndex));
+        }
+        else if(guard->IsInvalidatedDuringSweep())
+        {
+            // just mark this as actual invalidated since there are no types
+            // present
+            guard->Invalidate();
+        }
 
+        // verify if guard value is valid, it is present in one of the types
+        AssertMsg(!this->guard->IsValid() || isGuardValuePresent, "After ClearUnusedTypes, valid guard value should be one of the cached equivalent types.");
         return isAnyTypeLive;
     }
 

lib/Runtime/Base/FunctionBody.h

@@ -73,20 +73,53 @@ namespace Js
     {
         friend class PropertyGuardValidator;
     private:
+        enum GuardValue : intptr_t {
+            Invalidated = 0,
+            Uninitialized = 1,
+            Invalidated_DuringSweep = 2
+        };
         intptr_t value;
+#if DBG
+        bool wasReincarnated = false;
+#endif
     public:
         static PropertyGuard* New(Recycler* recycler) { return RecyclerNewLeaf(recycler, Js::PropertyGuard); }
-        PropertyGuard() : value(1) {}
-        PropertyGuard(intptr_t value) : value(value) { Assert(this->value != 0); }
+        PropertyGuard() : value(GuardValue::Uninitialized) {}
+        PropertyGuard(intptr_t value) : value(value) 
+        { 
+            // GuardValue::Invalidated and GuardValue::Invalidated_DuringSweeping can only be set using
+            // Invalidate() and InvalidatedDuringSweep() methods respectively.
+            Assert(this->value != GuardValue::Invalidated && this->value != GuardValue::Invalidated_DuringSweep); 
+        }
 
         inline static size_t const GetSizeOfValue() { return sizeof(((PropertyGuard*)0)->value); }
         inline static size_t const GetOffsetOfValue() { return offsetof(PropertyGuard, value); }
 
         intptr_t GetValue() const { return this->value; }
-        bool IsValid() { return this->value != 0; }
-        void SetValue(intptr_t value) { Assert(value != 0); this->value = value; }
+        bool IsValid()
+        {
+            return this->value != GuardValue::Invalidated && this->value != GuardValue::Invalidated_DuringSweep;
+        }
+        bool IsInvalidatedDuringSweep() { return this->value == GuardValue::Invalidated_DuringSweep; }
+        void SetValue(intptr_t value)
+        { 
+            // GuardValue::Invalidated and GuardValue::Invalidated_DuringSweeping can only be set using
+            // Invalidate() and InvalidatedDuringSweep() methods respectively.
+            Assert(value != GuardValue::Invalidated && value != GuardValue::Invalidated_DuringSweep);
+            this->value = value;
+        }
         intptr_t const* GetAddressOfValue() { return &this->value; }
-        void Invalidate() { this->value = 0; }
+        void Invalidate() { this->value = GuardValue::Invalidated; }
+        void InvalidateDuringSweep()
+        { 
+#if DBG
+            wasReincarnated = true;
+#endif
+            this->value = GuardValue::Invalidated_DuringSweep;
+        }
+#if DBG
+        bool WasReincarnated() { return this->wasReincarnated; }
+#endif
     };
 
     class PropertyGuardValidator
@@ -181,15 +214,34 @@ namespace Js
         // so as to keep the cached types alive.
         EquivalentTypeCache* cache;
         uint32 objTypeSpecFldId;
+#if DBG
+        // Intentionally have as intptr_t so this guard doesn't hold scriptContext
+        intptr_t originalScriptContextValue = 0;
+#endif
 
     public:
         JitEquivalentTypeGuard(Type* type, int index, uint32 objTypeSpecFldId):
-            JitIndexedPropertyGuard(reinterpret_cast<intptr_t>(type), index), cache(nullptr), objTypeSpecFldId(objTypeSpecFldId) {}
+            JitIndexedPropertyGuard(reinterpret_cast<intptr_t>(type), index), cache(nullptr), objTypeSpecFldId(objTypeSpecFldId)
+        {
+#if DBG
+            originalScriptContextValue = reinterpret_cast<intptr_t>(type->GetScriptContext());
+#endif
+        }
 
         Js::Type* GetType() const { return reinterpret_cast<Js::Type*>(this->GetValue()); }
 
         void SetType(const Js::Type* type)
         {
+#if DBG
+            if (originalScriptContextValue == 0)
+            {
+                originalScriptContextValue = reinterpret_cast<intptr_t>(type->GetScriptContext());
+            }
+            else
+            {
+                AssertMsg(originalScriptContextValue == reinterpret_cast<intptr_t>(type->GetScriptContext()), "Trying to set guard type from different script context.");
+            }
+#endif
             this->SetValue(reinterpret_cast<intptr_t>(type));
         }
 

lib/Runtime/Language/FunctionCodeGenJitTimeData.cpp

@@ -667,20 +667,17 @@ namespace Js
 
         if (PHASE_TRACE(Js::ObjTypeSpecPhase, topFunctionBody) || PHASE_TRACE(Js::EquivObjTypeSpecPhase, topFunctionBody))
         {
-            if (PHASE_TRACE(Js::ObjTypeSpecPhase, topFunctionBody) || PHASE_TRACE(Js::EquivObjTypeSpecPhase, topFunctionBody))
+            if (typeSet)
             {
-                if (typeSet)
+                const PropertyRecord* propertyRecord = scriptContext->GetPropertyName(propertyId);
+                Output::Print(_u("Created ObjTypeSpecFldInfo: id %u, property %s(#%u), slot %u, type set: "),
+                              id, propertyRecord->GetBuffer(), propertyId, slotIndex);
+                for (uint16 ti = 0; ti < typeCount - 1; ti++)
                 {
-                    const PropertyRecord* propertyRecord = scriptContext->GetPropertyName(propertyId);
-                    Output::Print(_u("Created ObjTypeSpecFldInfo: id %u, property %s(#%u), slot %u, type set: "),
-                        id, propertyRecord->GetBuffer(), propertyId, slotIndex);
-                    for (uint16 ti = 0; ti < typeCount - 1; ti++)
-                    {
-                        Output::Print(_u("0x%p, "), typeSet->GetType(ti));
-                    }
-                    Output::Print(_u("0x%p\n"), typeSet->GetType(typeCount - 1));
-                    Output::Flush();
+                    Output::Print(_u("0x%p, "), typeSet->GetType(ti));
                 }
+                Output::Print(_u("0x%p\n"), typeSet->GetType(typeCount - 1));
+                Output::Flush();
             }
         }
 

lib/Runtime/Language/JavascriptOperators.cpp

@@ -8197,24 +8197,52 @@ namespace Js
             return false;
         }
 
-        if (guard->GetType()->GetScriptContext() != type->GetScriptContext())
+        if (!guard->IsInvalidatedDuringSweep() && guard->GetType()->GetScriptContext() != type->GetScriptContext())
         {
-            // Can't cache cross-context objects
+            // For valid guard value, can't cache cross-context objects
             return false;
         }
 
         // CONSIDER : Add stats on how often the cache hits, and simply force bailout if
         // the efficacy is too low.
 
         EquivalentTypeCache* cache = guard->GetCache();
-
         // CONSIDER : Consider emitting o.type == equivTypes[hash(o.type)] in machine code before calling
         // this helper, particularly if we want to handle polymorphism with frequently changing types.
         Assert(EQUIVALENT_TYPE_CACHE_SIZE == 8);
         Type** equivTypes = cache->types;
+
+        Type* refType = equivTypes[0];
+        if (refType == nullptr || refType->GetScriptContext() != type->GetScriptContext())
+        {
+            // We could have guard that was invalidated while sweeping and now we have type coming from
+            // different scriptContext. Make sure that it matches the scriptContext in cachedTypes.
+            // If not, return false because as mentioned above, we don't cache cross-context objects.
+#if DBG
+            if (refType == nullptr)
+            {
+                for (int i = 1;i < EQUIVALENT_TYPE_CACHE_SIZE;i++)
+                {
+                    AssertMsg(equivTypes[i] == nullptr, "In equiv typed caches, if first element is nullptr, all others should be nullptr");
+                }
+            }
+#endif
+            return false;
+        }
+
         if (type == equivTypes[0] || type == equivTypes[1] || type == equivTypes[2] || type == equivTypes[3] ||
             type == equivTypes[4] || type == equivTypes[5] || type == equivTypes[6] || type == equivTypes[7])
         {
+#if DBG
+            if (PHASE_TRACE1(Js::EquivObjTypeSpecPhase))
+            {
+                if (guard->WasReincarnated())
+                {
+                    Output::Print(_u("EquivObjTypeSpec: Guard 0x%p was reincarnated and working now \n"), guard);
+                    Output::Flush();
+                }
+            }
+#endif
             guard->SetType(type);
             return true;
         }
@@ -8230,12 +8258,6 @@ namespace Js
         //    same prototype, any of the equivalent fixed properties will match. If any has been overwritten, the
         //    corresponding guard would have been invalidated and we would bail out (as above).
 
-        Type* refType = equivTypes[0];
-        if (refType == nullptr)
-        {
-            return false;
-        }
-
         if (cache->IsLoadedFromProto() && type->GetPrototype() != refType->GetPrototype())
         {
             if (PHASE_TRACE1(Js::EquivObjTypeSpecPhase))
@@ -8292,38 +8314,56 @@ namespace Js
             return false;
         }
 
-        // CONSIDER (EquivObjTypeSpec): Invent some form of least recently used eviction scheme.
-        uintptr_t index = (reinterpret_cast<uintptr_t>(type) >> 4) & (EQUIVALENT_TYPE_CACHE_SIZE - 1);
-        if (cache->nextEvictionVictim == EQUIVALENT_TYPE_CACHE_SIZE)
+        int emptySlotIndex = -1;
+        for (int i = 0;i < EQUIVALENT_TYPE_CACHE_SIZE;i++)
         {
-            __analysis_assume(index < EQUIVALENT_TYPE_CACHE_SIZE);
-            if (equivTypes[index] != nullptr)
+            if (equivTypes[i] == nullptr)
             {
-                uintptr_t initialIndex = index;
-                index = (initialIndex + 1) & (EQUIVALENT_TYPE_CACHE_SIZE - 1);
-                for (; index != initialIndex; index = (index + 1) & (EQUIVALENT_TYPE_CACHE_SIZE - 1))
-                {
-                    if (equivTypes[index] == nullptr) break;
-                }
-            }
-            __analysis_assume(index < EQUIVALENT_TYPE_CACHE_SIZE);
-            if (equivTypes[index] != nullptr)
+                emptySlotIndex = i;
+                break;
+            };
+        }
+
+        // We have some empty slots, let us use those first
+        if (emptySlotIndex != -1)
+        {
+            if (PHASE_TRACE1(Js::EquivObjTypeSpecPhase))
             {
-                cache->nextEvictionVictim = 0;
+                Output::Print(_u("EquivObjTypeSpec: Saving type in unused slot of equiv types cache. \n"));
+                Output::Flush();
             }
+            equivTypes[emptySlotIndex] = type;
         }
         else
         {
-            Assert(cache->nextEvictionVictim < EQUIVALENT_TYPE_CACHE_SIZE);
-            __analysis_assume(cache->nextEvictionVictim < EQUIVALENT_TYPE_CACHE_SIZE);
-            equivTypes[cache->nextEvictionVictim] = equivTypes[index];
-            cache->nextEvictionVictim = (cache->nextEvictionVictim + 1) & (EQUIVALENT_TYPE_CACHE_SIZE - 1);
-        }
-
-        Assert(index < EQUIVALENT_TYPE_CACHE_SIZE);
-        __analysis_assume(index < EQUIVALENT_TYPE_CACHE_SIZE);
-        equivTypes[index] = type;
+            // CONSIDER (EquivObjTypeSpec): Invent some form of least recently used eviction scheme.
+            uintptr_t index = (reinterpret_cast<uintptr_t>(type) >> 4) & (EQUIVALENT_TYPE_CACHE_SIZE - 1);
+            
+            if (cache->nextEvictionVictim == EQUIVALENT_TYPE_CACHE_SIZE)
+            {
+                __analysis_assume(index < EQUIVALENT_TYPE_CACHE_SIZE);
+                // If nextEvictionVictim was never set, set it to next element after index
+                cache->nextEvictionVictim = (index + 1) & (EQUIVALENT_TYPE_CACHE_SIZE - 1);
+            }
+            else
+            {
+                Assert(cache->nextEvictionVictim < EQUIVALENT_TYPE_CACHE_SIZE);
+                __analysis_assume(cache->nextEvictionVictim < EQUIVALENT_TYPE_CACHE_SIZE);
+                equivTypes[cache->nextEvictionVictim] = equivTypes[index];
+                // Else, set it to next element after current nextEvictionVictim index
+                cache->nextEvictionVictim = (cache->nextEvictionVictim + 1) & (EQUIVALENT_TYPE_CACHE_SIZE - 1);
+            }
 
+            if (PHASE_TRACE1(Js::EquivObjTypeSpecPhase))
+            {
+                Output::Print(_u("EquivObjTypeSpec: Saving type in used slot of equiv types cache at index = %d. NextEvictionVictim = %d. \n"), index, cache->nextEvictionVictim);
+                Output::Flush();
+            }
+            Assert(index < EQUIVALENT_TYPE_CACHE_SIZE);
+            __analysis_assume(index < EQUIVALENT_TYPE_CACHE_SIZE);
+            equivTypes[index] = type;
+        }
+        
         // Fixed field checks allow us to assume a specific type ID, but the assumption is only
         // valid if we lock the type. Otherwise, the type ID may change out from under us without
         // evolving the type.