Issue3035: Memory leak in Array.splice() #3211

suwc · 2017-06-23T00:09:19Z

Memory leak occurs when the entire inline segment is removed
from the array's linked list, because the inline segment is
not collected like other segments. Fix by clearing the elements
of the inline segment when it is replaced.

suwc · 2017-06-23T00:12:24Z

Addresses #3035

agarwal-sandeep · 2017-06-23T00:22:04Z

lib/Runtime/Library/JavascriptArray.cpp

+            {
+                inlineHeadSegment = DetermineInlineHeadSegmentPointer<JavascriptNativeFloatArray, 0, true>((JavascriptNativeFloatArray*)pArr);
+            }
+            else if (JavascriptNativeIntArray::Is(pArr))


if (JavascriptNativeIntArray::Is(pArr) [](start = 17, length = 38)

This can be converted to an assert instead as JavascriptNativeArray::Is already verified that it is either a NativeFloat or NativeInt array #Resolved

This should be NativeIntArray. FailFast would be good.

In reply to: 123650724 [](ancestors = 123650724)

agarwal-sandeep · 2017-06-23T00:41:16Z

lib/Runtime/Library/JavascriptArray.cpp

@@ -2033,6 +2064,7 @@ namespace Js
        SparseArraySegmentBase *seg, *nextSeg, *prevSeg = nullptr;
        for (seg = fArray->head; seg; seg = nextSeg)
        {
+            bool isInlineSegment = JavascriptArray::IsInlineSegment(seg, fArray);


isInlineSegment [](start = 17, length = 15)

Have isInlineSegment as false and move isInlineSegment = JavascriptArray::IsInlineSegment(seg, fArray); in if part where we are allocating newSeg that way when we call ClearElements we need to check only isInlineSegment #WontFix

Point taken and updated in other occurrences. Decided JavascriptNativeFloatArray::ConvertToVarArray() does not need to call clearElements() because its inlined head segment only contains native data. Same thing for JavascriptNativeIntArray::ConvertToVarArray().

In reply to: 123652599 [](ancestors = 123652599)

agarwal-sandeep · 2017-06-23T00:54:58Z

lib/Runtime/Library/JavascriptArray.cpp

@@ -5615,6 +5633,11 @@ namespace Js
                head->length = offset + next->length;
                head->CheckLengthvsSize();
                pArr->head = head;
+
+                if (oldHead != head && isInlineSegment)


if (oldHead != head && isInlineSegment) [](start = 16, length = 39)

I think if we reverse the && to have isInlineSegment first compiler can do a test for bool and then a pointer comparison which will be faster #Resolved

agarwal-sandeep · 2017-06-23T00:59:03Z

lib/Runtime/Library/JavascriptArray.cpp

@@ -6500,7 +6523,13 @@ namespace Js
            uint32 newLength = head->length + countUndefined;
            if (newLength > head->size)
            {
+                SparseArraySegmentBase *oldHead = head;
+                bool isInlineSegment = JavascriptArray::IsInlineSegment(head, this);
                head = SparseArraySegment<Var>::From(head)->GrowByMin(recycler, newLength - head->size);


GrowByMin [](start = 60, length = 9)

Seems like this will always allocate a new segment so the check oldHead != head is not needed #Closed

agarwal-sandeep · 2017-06-23T01:06:17Z

lib/Runtime/Library/JavascriptArray.cpp

-            hasInlineSegment = HasInlineHeadSegment(pArr->head->length);
-        }
-
+        bool hasInlineSegment = IsInlineSegment(startSeg, pArr);


startSeg [](start = 48, length = 8)

Seems like we can have scenario where startSeg is nullptr (check below), after your change for those scenario we will have AV in FromVar during JavascriptNativeArray::Is #Resolved

Added nullptr checking in JavascriptArray::IsInlineSegment().

In reply to: 123654814 [](ancestors = 123654814)

agarwal-sandeep · 2017-06-23T01:07:49Z

lib/Runtime/Library/JavascriptArray.cpp

@@ -7137,6 +7149,11 @@ namespace Js
                    {
                        segInsert = segInsert->GrowByMinMax(recycler, insertLen - spaceLeft, segInsert->next->left - segInsert->left - segInsert->size);
                    }
+
+                    if (oldSegInsert != segInsert && isInlineSegment)


oldSegInsert != segInsert [](start = 24, length = 25)

Redundant GrowBy* will always allocate a new segment #Resolved

agarwal-sandeep · 2017-06-23T16:25:21Z

lib/Runtime/Library/JavascriptArray.inl

+        if (isInlineSegment)
+        {
+            this->ClearElements(seg, 0);
+        }


Instead of clearing the elements can't we free it? next/previous is already adjusted so no one should be using seg. Will save recycler some work at same cost of clearing elements. #Closed

I think the inline segment is a plus alloc, it's part of the array structure, you can't free only a part of memory of one allocation #Closed

correct.

In reply to: 123792263 [](ancestors = 123792263)

akroshg · 2017-06-23T17:30:53Z

lib/Runtime/Library/JavascriptArray.cpp

@@ -529,6 +529,31 @@ namespace Js
                VirtualTableInfo<JavascriptNativeFloatArray>::HasVirtualTable(aValue));
    }

+    bool JavascriptArray::IsInlineSegment(SparseArraySegmentBase *seg, JavascriptArray *pArr)


seg [](start = 66, length = 3)

return false when seg is null? could be a valid case. #Resolved

agarwal-sandeep · 2017-06-23T17:58:00Z

lib/Runtime/Library/JavascriptArray.cpp

@@ -7579,6 +7605,10 @@ namespace Js
            pArr->head = SparseArraySegment<T>::From(pArr->head)->GrowByMinMax(recycler, unshiftElements, ((nextToHeadSeg->left + unshiftElements) - pArr->head->left - pArr->head->size));
        }

+        if (oldHead != pArr->head && isInlineSegment)


oldHead != pArr->head [](start = 12, length = 21)

Redundant? #Resolved

agarwal-sandeep · 2017-06-23T18:00:38Z

                    current = current->GrowByMin(recycler, growby);

Only in this case current is changed, we can prevent oldCurrent != current below by initializing isInlineSegment to false and moving IsInlineSegment here #Resolved

Refers to: lib/Runtime/Library/JavascriptArray.inl:919 in 05b1641. [](commit_id = 05b1641, deletion_comment = False)

suwc · 2017-06-26T17:29:18Z

Any other questions or comments?

agarwal-sandeep · 2017-06-26T17:52:02Z

lib/Runtime/Library/JavascriptArray.inl

@@ -1357,6 +1397,8 @@ SECOND_PASS:
                    const bool currentWasFull = current->length == current->size;

                    Assert(itemIndex == current->left + current->size);
+                    SparseArraySegmentBase* oldSegment = current;
+                    bool isInlineSegment = JavascriptArray::IsInlineSegment(oldSegment, this);
                    current = SparseArraySegment<T>::CopySegment(recycler, (SparseArraySegment<T>*)current, next->left, next, next->left, next->length);


SparseArraySegment::CopySegment [](start = 30, length = 34)

CopySegment doesn't always allocate a new segment, can we assert before clearing that current != oldSegment #Resolved

agarwal-sandeep · 2017-06-26T18:02:16Z

Can we add the test case you have as a snippet in Exprgen? #Resolved

agarwal-sandeep · 2017-06-26T18:02:28Z

Rest LGTM

suwc · 2017-06-26T18:02:58Z

Sure

In reply to: 311135664 [](ancestors = 311135664)

Memory leak occurs when the entire inline segment is removed from the array's linked list, because the inline segment is not collected like other segments. Fix by clearing the elements of the inline segment when it is replaced.

Merge pull request #3211 from suwc:build/suwc/Issue3035 Memory leak occurs when the entire inline segment is removed from the array's linked list, because the inline segment is not collected like other segments. Fix by clearing the elements of the inline segment when it is replaced.

…ce() Merge pull request #3211 from suwc:build/suwc/Issue3035 Memory leak occurs when the entire inline segment is removed from the array's linked list, because the inline segment is not collected like other segments. Fix by clearing the elements of the inline segment when it is replaced.

The assertions are intended to future-proof the assumption that isInlineSegment being true implies segment swap. chakra-core#3211

Merge pull request #3322 from suwc:build/suwc/OS12507864 The assertions are intended to future-proof the assumption that isInlineSegment being true implies segment swap. #3211

…d in PR#3211 Merge pull request #3322 from suwc:build/suwc/OS12507864 The assertions are intended to future-proof the assumption that isInlineSegment being true implies segment swap. #3211

…ion introduced in PR#3211 Merge pull request #3322 from suwc:build/suwc/OS12507864 The assertions are intended to future-proof the assumption that isInlineSegment being true implies segment swap. #3211

@suwc

…GE #3322 @suwc] OS12507864: Correct assertion introduced in PR#3211 Merge pull request #3322 from suwc:build/suwc/OS12507864 The assertions are intended to future-proof the assumption that isInlineSegment being true implies segment swap. chakra-core/ChakraCore#3211

msftclas added the cla-not-required label Jun 23, 2017

suwc requested review from leirocks, digitalinfinity and akroshg June 23, 2017 00:12

agarwal-sandeep reviewed Jun 23, 2017

View reviewed changes

akroshg reviewed Jun 23, 2017

View reviewed changes

agarwal-sandeep reviewed Jun 23, 2017

View reviewed changes

suwc force-pushed the build/suwc/Issue3035 branch from 05b1641 to bde3c38 Compare June 23, 2017 23:45

agarwal-sandeep reviewed Jun 26, 2017

View reviewed changes

Issue3035: Memory leak in Array.splice()

baf6fcb

Memory leak occurs when the entire inline segment is removed from the array's linked list, because the inline segment is not collected like other segments. Fix by clearing the elements of the inline segment when it is replaced.

suwc force-pushed the build/suwc/Issue3035 branch from bde3c38 to baf6fcb Compare June 26, 2017 18:52

chakrabot merged commit baf6fcb into chakra-core:release/1.6 Jun 26, 2017

suwc deleted the build/suwc/Issue3035 branch June 26, 2017 19:51

suwc mentioned this pull request Jul 12, 2017

OS12507864: Correct assertion introduced in PR#3211 #3322

Merged

suwc pushed a commit to suwc/ChakraCore that referenced this pull request Jul 12, 2017

OS12507864: Correct assertion introduced in PR#3211

66c97fc

The assertions are intended to future-proof the assumption that isInlineSegment being true implies segment swap. chakra-core#3211

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Issue3035: Memory leak in Array.splice() #3211

Issue3035: Memory leak in Array.splice() #3211

suwc commented Jun 23, 2017

suwc commented Jun 23, 2017

agarwal-sandeep Jun 23, 2017 •

edited by suwc

akroshg Jun 23, 2017

agarwal-sandeep Jun 23, 2017 •

edited by suwc

suwc Jun 23, 2017

agarwal-sandeep Jun 23, 2017 •

edited by suwc

agarwal-sandeep Jun 23, 2017 •

edited

agarwal-sandeep Jun 23, 2017 •

edited by suwc

suwc Jun 23, 2017

agarwal-sandeep Jun 23, 2017 •

edited by suwc

agarwal-sandeep Jun 23, 2017 •

edited

leirocks Jun 23, 2017 •

edited by agarwal-sandeep

akroshg Jun 23, 2017

akroshg Jun 23, 2017 •

edited by suwc

agarwal-sandeep Jun 23, 2017 •

edited by suwc

agarwal-sandeep commented Jun 23, 2017 •

edited by suwc

suwc commented Jun 26, 2017

agarwal-sandeep Jun 26, 2017 •

edited by suwc

agarwal-sandeep commented Jun 26, 2017 •

edited by suwc

agarwal-sandeep commented Jun 26, 2017

suwc commented Jun 26, 2017

Issue3035: Memory leak in Array.splice() #3211

Issue3035: Memory leak in Array.splice() #3211

Conversation

suwc commented Jun 23, 2017

suwc commented Jun 23, 2017

agarwal-sandeep Jun 23, 2017 • edited by suwc

Choose a reason for hiding this comment

akroshg Jun 23, 2017

Choose a reason for hiding this comment

agarwal-sandeep Jun 23, 2017 • edited by suwc

Choose a reason for hiding this comment

suwc Jun 23, 2017

Choose a reason for hiding this comment

agarwal-sandeep Jun 23, 2017 • edited by suwc

Choose a reason for hiding this comment

agarwal-sandeep Jun 23, 2017 • edited

Choose a reason for hiding this comment

agarwal-sandeep Jun 23, 2017 • edited by suwc

Choose a reason for hiding this comment

suwc Jun 23, 2017

Choose a reason for hiding this comment

agarwal-sandeep Jun 23, 2017 • edited by suwc

Choose a reason for hiding this comment

agarwal-sandeep Jun 23, 2017 • edited

Choose a reason for hiding this comment

leirocks Jun 23, 2017 • edited by agarwal-sandeep

Choose a reason for hiding this comment

akroshg Jun 23, 2017

Choose a reason for hiding this comment

akroshg Jun 23, 2017 • edited by suwc

Choose a reason for hiding this comment

agarwal-sandeep Jun 23, 2017 • edited by suwc

Choose a reason for hiding this comment

agarwal-sandeep commented Jun 23, 2017 • edited by suwc

suwc commented Jun 26, 2017

agarwal-sandeep Jun 26, 2017 • edited by suwc

Choose a reason for hiding this comment

agarwal-sandeep commented Jun 26, 2017 • edited by suwc

agarwal-sandeep commented Jun 26, 2017

suwc commented Jun 26, 2017

agarwal-sandeep Jun 23, 2017 •

edited by suwc

agarwal-sandeep Jun 23, 2017 •

edited by suwc

agarwal-sandeep Jun 23, 2017 •

edited by suwc

agarwal-sandeep Jun 23, 2017 •

edited

agarwal-sandeep Jun 23, 2017 •

edited by suwc

agarwal-sandeep Jun 23, 2017 •

edited by suwc

agarwal-sandeep Jun 23, 2017 •

edited

leirocks Jun 23, 2017 •

edited by agarwal-sandeep

akroshg Jun 23, 2017 •

edited by suwc

agarwal-sandeep Jun 23, 2017 •

edited by suwc

agarwal-sandeep commented Jun 23, 2017 •

edited by suwc

agarwal-sandeep Jun 26, 2017 •

edited by suwc

agarwal-sandeep commented Jun 26, 2017 •

edited by suwc