Fix bookkeeping of unregisteredInlineCacheCount

kunalspathak · kunalspathak · commit ddd183be0d60 · 2017-06-20T16:06:07.000-07:00
We were removing inlineCache from invalidation list but were not
updating the count at `ThreadContext` level. In between we were calling
compacting the invalidation list and find that there are more null
entries in the invalidation list than we thought (as per `threadContext-&gt;unregisteredInlineCacheCount`).

Fix by calling compact only after we update the variable at threadContext.
diff --git a/lib/Runtime/Base/FunctionBody.cpp b/lib/Runtime/Base/FunctionBody.cpp
@@ -7723,7 +7723,7 @@ namespace Js
                         // and doesn't get released to the allocator until there are no more outstanding references.  Thus we don't need
                         // to (and, in fact, cannot) remove it from the invalidation list here.  Instead, we'll do it in ReleaseInlineCache
                         // when there are no more outstanding references.
-                        rootObjectBase->ReleaseInlineCache(this->GetPropertyIdFromCacheId(i), false, false, IsScriptContextShutdown);
+                        unregisteredInlineCacheCount += rootObjectBase->ReleaseInlineCache(this->GetPropertyIdFromCacheId(i), false, false, IsScriptContextShutdown);
                     }
                 }
             }
@@ -7743,7 +7743,7 @@ namespace Js
                         // and doesn't get released to the allocator until there are no more outstanding references.  Thus we don't need
                         // to (and, in fact, cannot) remove it from the invalidation list here.  Instead, we'll do it in ReleaseInlineCache
                         // when there are no more outstanding references.
-                        rootObjectBase->ReleaseInlineCache(this->GetPropertyIdFromCacheId(i), true, false, IsScriptContextShutdown);
+                        unregisteredInlineCacheCount += rootObjectBase->ReleaseInlineCache(this->GetPropertyIdFromCacheId(i), true, false, IsScriptContextShutdown);
                     }
                 }
             }
@@ -7763,7 +7763,7 @@ namespace Js
                         // and doesn't get released to the allocator until there are no more outstanding references.  Thus we don't need
                         // to (and, in fact, cannot) remove it from the invalidation list here.  Instead, we'll do it in ReleaseInlineCache
                         // when there are no more outstanding references.
-                        rootObjectBase->ReleaseInlineCache(this->GetPropertyIdFromCacheId(i), false, true, IsScriptContextShutdown);
+                        unregisteredInlineCacheCount += rootObjectBase->ReleaseInlineCache(this->GetPropertyIdFromCacheId(i), false, true, IsScriptContextShutdown);
                     }
                 }
             }
diff --git a/lib/Runtime/Library/RootObjectBase.cpp b/lib/Runtime/Library/RootObjectBase.cpp
@@ -96,7 +96,7 @@ namespace Js
 
     // TODO: Switch to take PropertyRecord instead once we clean up the function body to hold onto propertyRecord
     // instead of propertyId.
-    void
+    uint
     RootObjectBase::ReleaseInlineCache(Js::PropertyId propertyId, bool isLoadMethod, bool isStore, bool isShutdown)
     {
         uint unregisteredInlineCacheCount = 0;
@@ -129,10 +129,7 @@ namespace Js
             }
         );
         Assert(found);
-        if (unregisteredInlineCacheCount > 0)
-        {
-            this->GetScriptContext()->GetThreadContext()->NotifyInlineCacheBatchUnregistered(unregisteredInlineCacheCount);
-        }
+        return unregisteredInlineCacheCount;
     }
 
     BOOL
diff --git a/lib/Runtime/Library/RootObjectBase.h b/lib/Runtime/Library/RootObjectBase.h
@@ -27,7 +27,7 @@ namespace Js
 
         Js::InlineCache * GetInlineCache(Js::PropertyRecord const* propertyRecord, bool isLoadMethod, bool isStore);
         Js::RootObjectInlineCache * GetRootInlineCache(Js::PropertyRecord const* propertyRecord, bool isLoadMethod, bool isStore);
-        void ReleaseInlineCache(PropertyId propertyId, bool isLoadMethod, bool isStore, bool isShutdown);
+        uint ReleaseInlineCache(PropertyId propertyId, bool isLoadMethod, bool isStore, bool isShutdown);
 
         virtual BOOL EnsureProperty(PropertyId propertyId) override;
         virtual BOOL EnsureNoRedeclProperty(PropertyId propertyId) override sealed;

Original file line number	Diff line number	Diff line change
`@@ -7723,7 +7723,7 @@ namespace Js`
`7723`	`7723`	`// and doesn't get released to the allocator until there are no more outstanding references. Thus we don't need`
`7724`	`7724`	`// to (and, in fact, cannot) remove it from the invalidation list here. Instead, we'll do it in ReleaseInlineCache`
`7725`	`7725`	`// when there are no more outstanding references.`
`7726`		`- rootObjectBase->ReleaseInlineCache(this->GetPropertyIdFromCacheId(i), false, false, IsScriptContextShutdown);`
	`7726`	`+ unregisteredInlineCacheCount += rootObjectBase->ReleaseInlineCache(this->GetPropertyIdFromCacheId(i), false, false, IsScriptContextShutdown);`
`7727`	`7727`	`}`
`7728`	`7728`	`}`
`7729`	`7729`	`}`
`@@ -7743,7 +7743,7 @@ namespace Js`
`7743`	`7743`	`// and doesn't get released to the allocator until there are no more outstanding references. Thus we don't need`
`7744`	`7744`	`// to (and, in fact, cannot) remove it from the invalidation list here. Instead, we'll do it in ReleaseInlineCache`
`7745`	`7745`	`// when there are no more outstanding references.`
`7746`		`- rootObjectBase->ReleaseInlineCache(this->GetPropertyIdFromCacheId(i), true, false, IsScriptContextShutdown);`
	`7746`	`+ unregisteredInlineCacheCount += rootObjectBase->ReleaseInlineCache(this->GetPropertyIdFromCacheId(i), true, false, IsScriptContextShutdown);`
`7747`	`7747`	`}`
`7748`	`7748`	`}`
`7749`	`7749`	`}`
`@@ -7763,7 +7763,7 @@ namespace Js`
`7763`	`7763`	`// and doesn't get released to the allocator until there are no more outstanding references. Thus we don't need`
`7764`	`7764`	`// to (and, in fact, cannot) remove it from the invalidation list here. Instead, we'll do it in ReleaseInlineCache`
`7765`	`7765`	`// when there are no more outstanding references.`
`7766`		`- rootObjectBase->ReleaseInlineCache(this->GetPropertyIdFromCacheId(i), false, true, IsScriptContextShutdown);`
	`7766`	`+ unregisteredInlineCacheCount += rootObjectBase->ReleaseInlineCache(this->GetPropertyIdFromCacheId(i), false, true, IsScriptContextShutdown);`
`7767`	`7767`	`}`
`7768`	`7768`	`}`
`7769`	`7769`	`}`
Original file line number	Diff line number	Diff line change
`@@ -96,7 +96,7 @@ namespace Js`
`96`	`96`
`97`	`97`	`// TODO: Switch to take PropertyRecord instead once we clean up the function body to hold onto propertyRecord`
`98`	`98`	`// instead of propertyId.`
`99`		`- void`
	`99`	`+ uint`
`100`	`100`	`RootObjectBase::ReleaseInlineCache(Js::PropertyId propertyId, bool isLoadMethod, bool isStore, bool isShutdown)`
`101`	`101`	`{`
`102`	`102`	`uint unregisteredInlineCacheCount = 0;`
`@@ -129,10 +129,7 @@ namespace Js`
`129`	`129`	`}`
`130`	`130`	`);`
`131`	`131`	`Assert(found);`
`132`		`- if (unregisteredInlineCacheCount > 0)`
`133`		`- {`
`134`		`- this->GetScriptContext()->GetThreadContext()->NotifyInlineCacheBatchUnregistered(unregisteredInlineCacheCount);`
`135`		`- }`
	`132`	`+ return unregisteredInlineCacheCount;`
`136`	`133`	`}`
`137`	`134`
`138`	`135`	`BOOL`