Changeset 155395 in webkit

Timestamp:

Sep 9, 2013, 3:41:00 PM (13 years ago)

Author:

mhahnenberg@apple.com

Message:

JSArray::shiftCountWithArrayStorage doesn't change indexBias when shifting the last element in m_vector
​https://bugs.webkit.org/show_bug.cgi?id=120389

Reviewed by Michael Saboff.

Went through and cleaned up shiftCountWithArrayStorage. Gave meaningful variable names
and commented the confusing parts. This led to realizing how to fix this bug, which has
been done. The issue was that we were modifying the vector length unconditionally, even
when we weren't logically changing the length of the vector. Instead, we should only modify
the vector length when we modify the index bias.

• runtime/JSArray.cpp:

(JSC::JSArray::shiftCountWithArrayStorage):

Location:

trunk/Source/JavaScriptCore

Files:

• ChangeLog (modified) (1 diff)
• heap/Heap.cpp (modified) (1 diff)
• runtime/JSArray.cpp (modified) (1 diff)

trunk/Source/JavaScriptCore/ChangeLog

@@ +1 @@
+2013-08-29  Mark Hahnenberg  <mhahnenberg@apple.com>
+
+        JSArray::shiftCountWithArrayStorage doesn't change indexBias when shifting the last element in m_vector
+        https://bugs.webkit.org/show_bug.cgi?id=120389
+
+        Reviewed by Michael Saboff.
+
+        Went through and cleaned up shiftCountWithArrayStorage. Gave meaningful variable names
+        and commented the confusing parts. This led to realizing how to fix this bug, which has
+        been done. The issue was that we were modifying the vector length unconditionally, even
+        when we weren't logically changing the length of the vector. Instead, we should only modify
+        the vector length when we modify the index bias.
+
+        * runtime/JSArray.cpp:
+        (JSC::JSArray::shiftCountWithArrayStorage):
+
 2013-09-08  Anders Carlsson  <andersca@apple.com>
 

trunk/Source/JavaScriptCore/heap/Heap.cpp

@@ -55 +55 @@
 static const size_t largeHeapSize = 32 * MB; // About 1.5X the average webpage.
 static const size_t smallHeapSize = 1 * MB; // Matches the FastMalloc per-thread cache.
+
+#define ENABLE_GC_LOGGING 1
 
 #if ENABLE(GC_LOGGING)

trunk/Source/JavaScriptCore/runtime/JSArray.cpp

@@ -701 +701 @@
     unsigned usedVectorLength = min(vectorLength, oldLength);
     
-    vectorLength -= count;
-    storage->setVectorLength(vectorLength);
-    
-    if (vectorLength) {
-        if (startIndex < usedVectorLength - (startIndex + count)) {
-            if (startIndex) {
-                memmove(
-                    storage->m_vector + count,
-                    storage->m_vector,
-                    sizeof(JSValue) * startIndex);
-            }
-            m_butterfly = m_butterfly->shift(structure(), count);
-            storage = m_butterfly->arrayStorage();
-            storage->m_indexBias += count;
-        } else {
+    unsigned numElementsBeforeShiftRegion = startIndex;
+    unsigned firstIndexAfterShiftRegion = startIndex + count;
+    unsigned numElementsAfterShiftRegion = usedVectorLength - firstIndexAfterShiftRegion;
+    ASSERT(numElementsBeforeShiftRegion + count + numElementsAfterShiftRegion == usedVectorLength);
+
+    // The point of this comparison seems to be to minimize the amount of elements that have to 
+    // be moved during a shift operation.
+    if (numElementsBeforeShiftRegion < numElementsAfterShiftRegion) {
+        // The number of elements before the shift region is less than the number of elements
+        // after the shift region, so we move the elements before to the right.
+        if (numElementsBeforeShiftRegion) {
+            RELEASE_ASSERT(count + startIndex <= vectorLength);
             memmove(
-                storage->m_vector + startIndex,
-                storage->m_vector + startIndex + count,
-                sizeof(JSValue) * (usedVectorLength - (startIndex + count)));
-            for (unsigned i = usedVectorLength - count; i < usedVectorLength; ++i)
-                storage->m_vector[i].clear();
-        }
-    }
+                storage->m_vector + count,
+                storage->m_vector,
+                sizeof(JSValue) * startIndex);
+        }
+        // Adjust the Butterfly and the index bias. We only need to do this here because we're changing
+        // the start of the Butterfly, which needs to point at the first indexed property in the used
+        // portion of the vector.
+        m_butterfly = m_butterfly->shift(structure(), count);
+        storage = m_butterfly->arrayStorage();
+        storage->m_indexBias += count;
+
+        // Since we're consuming part of the vector by moving its beginning to the left,
+        // we need to modify the vector length appropriately.
+        storage->setVectorLength(vectorLength - count);
+    } else {
+        // The number of elements before the shift region is greater than or equal to the number 
+        // of elements after the shift region, so we move the elements after the shift region to the left.
+        memmove(
+            storage->m_vector + startIndex,
+            storage->m_vector + firstIndexAfterShiftRegion,
+            sizeof(JSValue) * numElementsAfterShiftRegion);
+        // Clear the slots of the elements we just moved.
+        unsigned startOfEmptyVectorTail = usedVectorLength - count;
+        for (unsigned i = startOfEmptyVectorTail; i < usedVectorLength; ++i)
+            storage->m_vector[i].clear();
+        // We don't modify the index bias or the Butterfly pointer in this case because we're not changing 
+        // the start of the Butterfly, which needs to point at the first indexed property in the used 
+        // portion of the vector. We also don't modify the vector length because we're not actually changing
+        // its length; we're just using less of it.
+    }
+    
     return true;
 }