[MERGE chakra-core#2465 @leirocks] Float32Array memory leak problem chakra-core#2328

leirocks · leirocks · commit 7bfaf94557a3 · 2017-02-08T10:20:42.000-08:00
Merge pull request chakra-core#2465 from leirocks:2328-2 the List keeping the weakreference that from ArrayBuffer to TypedArray kept expanding, there's no clean up after the TypedArray is collected. cleanup the list while adding parent once a while.
diff --git a/lib/Runtime/Library/ArrayBuffer.cpp b/lib/Runtime/Library/ArrayBuffer.cpp
@@ -95,7 +95,7 @@ namespace Js
 
         if (this->otherParents != nullptr)
         {
-            this->otherParents->Map([&](int index, RecyclerWeakReference<ArrayBufferParent>* item)
+            this->otherParents->Map([&](RecyclerWeakReference<ArrayBufferParent>* item)
             {
                 this->ClearParentsLength(item->Get());
             });
@@ -114,40 +114,25 @@ namespace Js
         {
             if (this->otherParents == nullptr)
             {
-                this->otherParents = JsUtil::List<RecyclerWeakReference<ArrayBufferParent>*>::New(this->GetRecycler());
+                this->otherParents = RecyclerNew(this->GetRecycler(), OtherParents, this->GetRecycler());
             }
-            this->otherParents->Add(this->GetRecycler()->CreateWeakReferenceHandle(parent));
-        }
-    }
 
-    void ArrayBuffer::RemoveParent(ArrayBufferParent* parent)
-    {
-        if (this->primaryParent != nullptr && this->primaryParent->Get() == parent)
-        {
-            this->primaryParent = nullptr;
-        }
-        else
-        {
-            int foundIndex = -1;
-            bool parentFound = this->otherParents != nullptr && this->otherParents->MapUntil([&](int index, RecyclerWeakReference<ArrayBufferParent>* item)
+            if (this->otherParents->increasedCount >= ParentsCleanupThreshold)
             {
-                if (item->Get() == parent)
+                auto iter = this->otherParents->GetEditingIterator();
+                while (iter.Next())
                 {
-                    foundIndex = index;
-                    return true;
+                    if (iter.Data()->Get() == nullptr)
+                    {
+                        iter.RemoveCurrent();
+                    }
                 }
-                return false;
-
-            });
 
-            if (parentFound)
-            {
-                this->otherParents->RemoveAt(foundIndex);
-            }
-            else
-            {
-                AssertMsg(false, "We shouldn't be clearing a parent that hasn't been set.");
+                this->otherParents->increasedCount = 0;
             }
+
+            this->otherParents->PrependNode(this->GetRecycler()->CreateWeakReferenceHandle(parent));
+            this->otherParents->increasedCount++;
         }
     }
 
diff --git a/lib/Runtime/Library/ArrayBuffer.h b/lib/Runtime/Library/ArrayBuffer.h
@@ -29,7 +29,6 @@ namespace Js
         virtual ArrayBuffer * GetAsArrayBuffer() = 0;
         virtual SharedArrayBuffer * GetAsSharedArrayBuffer() { return nullptr; }
         virtual void AddParent(ArrayBufferParent* parent) { }
-        virtual void RemoveParent(ArrayBufferParent* parent) { }
         virtual bool IsDetached() { return false; }
         virtual uint32 GetByteLength() const = 0;
         virtual BYTE* GetBuffer() const = 0;
@@ -122,14 +121,15 @@ namespace Js
         static int GetBufferOffset() { return offsetof(ArrayBuffer, buffer); }
 
         virtual void AddParent(ArrayBufferParent* parent) override;
-        virtual void RemoveParent(ArrayBufferParent* parent) override;
 #if _WIN64
         //maximum 2G -1  for amd64
         static const uint32 MaxArrayBufferLength = 0x7FFFFFFF;
 #else
         // maximum 1G to avoid arithmetic overflow.
         static const uint32 MaxArrayBufferLength = 1 << 30;
 #endif
+        static const uint32 ParentsCleanupThreshold = 1000;
+
         virtual bool IsValidAsmJsBufferLength(uint length, bool forceCheck = false) { return false; }
         virtual bool IsArrayBuffer() override { return true; }
         virtual bool IsSharedArrayBuffer() override { return false; }
@@ -147,8 +147,17 @@ namespace Js
 
         //In most cases, the ArrayBuffer will only have one parent
         RecyclerWeakReference<ArrayBufferParent>* primaryParent;
-        JsUtil::List<RecyclerWeakReference<ArrayBufferParent>*>* otherParents;
 
+        struct OtherParents :public SList<RecyclerWeakReference<ArrayBufferParent>*, Recycler>
+        {
+            OtherParents(Recycler* recycler)
+                :SList<RecyclerWeakReference<ArrayBufferParent>*, Recycler>(recycler), increasedCount(0)
+            {
+            }
+            uint increasedCount;
+        };
+
+        OtherParents* otherParents;
 
         BYTE  *buffer;             // Points to a heap allocated RGBA buffer, can be null
         uint32 bufferLength;       // Number of bytes allocated
@@ -179,15 +188,6 @@ namespace Js
             arrayBuffer->AddParent(this);
         }
 
-        void ClearArrayBuffer()
-        {
-            if (this->arrayBuffer != nullptr)
-            {
-                this->arrayBuffer->RemoveParent(this);
-                this->arrayBuffer = nullptr;
-            }
-        }
-
     public:
         ArrayBufferBase* GetArrayBuffer() const
         {

Original file line number	Diff line number	Diff line change
`@@ -95,7 +95,7 @@ namespace Js`
`95`	`95`
`96`	`96`	`if (this->otherParents != nullptr)`
`97`	`97`	`{`
`98`		`- this->otherParents->Map([&](int index, RecyclerWeakReference<ArrayBufferParent>* item)`
	`98`	`+ this->otherParents->Map([&](RecyclerWeakReference<ArrayBufferParent>* item)`
`99`	`99`	`{`
`100`	`100`	`this->ClearParentsLength(item->Get());`
`101`	`101`	`});`
`@@ -114,40 +114,25 @@ namespace Js`
`114`	`114`	`{`
`115`	`115`	`if (this->otherParents == nullptr)`
`116`	`116`	`{`
`117`		`- this->otherParents = JsUtil::List<RecyclerWeakReference<ArrayBufferParent>*>::New(this->GetRecycler());`
	`117`	`+ this->otherParents = RecyclerNew(this->GetRecycler(), OtherParents, this->GetRecycler());`
`118`	`118`	`}`
`119`		`- this->otherParents->Add(this->GetRecycler()->CreateWeakReferenceHandle(parent));`
`120`		`- }`
`121`		`- }`
`122`	`119`
`123`		`- void ArrayBuffer::RemoveParent(ArrayBufferParent* parent)`
`124`		`- {`
`125`		`- if (this->primaryParent != nullptr && this->primaryParent->Get() == parent)`
`126`		`- {`
`127`		`- this->primaryParent = nullptr;`
`128`		`- }`
`129`		`- else`
`130`		`- {`
`131`		`- int foundIndex = -1;`
`132`		`- bool parentFound = this->otherParents != nullptr && this->otherParents->MapUntil([&](int index, RecyclerWeakReference<ArrayBufferParent>* item)`
	`120`	`+ if (this->otherParents->increasedCount >= ParentsCleanupThreshold)`
`133`	`121`	`{`
`134`		`- if (item->Get() == parent)`
	`122`	`+ auto iter = this->otherParents->GetEditingIterator();`
	`123`	`+ while (iter.Next())`
`135`	`124`	`{`
`136`		`- foundIndex = index;`
`137`		`- return true;`
	`125`	`+ if (iter.Data()->Get() == nullptr)`
	`126`	`+ {`
	`127`	`+ iter.RemoveCurrent();`
	`128`	`+ }`
`138`	`129`	`}`
`139`		`- return false;`
`140`		`-`
`141`		`- });`
`142`	`130`
`143`		`- if (parentFound)`
`144`		`- {`
`145`		`- this->otherParents->RemoveAt(foundIndex);`
`146`		`- }`
`147`		`- else`
`148`		`- {`
`149`		`- AssertMsg(false, "We shouldn't be clearing a parent that hasn't been set.");`
	`131`	`+ this->otherParents->increasedCount = 0;`
`150`	`132`	`}`
	`133`	`+`
	`134`	`+ this->otherParents->PrependNode(this->GetRecycler()->CreateWeakReferenceHandle(parent));`
	`135`	`+ this->otherParents->increasedCount++;`
`151`	`136`	`}`
`152`	`137`	`}`
`153`	`138`