Changeset 184407 in webkit

Timestamp:

May 15, 2015, 1:02:26 PM (10 years ago)

Author:

mark.lam@apple.com

Message:

JSArray::setLength() should reallocate instead of zero-filling if the reallocation would be small enough.
​https://bugs.webkit.org/show_bug.cgi?id=144622

Reviewed by Geoffrey Garen.

When setting the array to a new length that is shorter, we now check if it is worth
just making a new butterfly instead of clearing out the slots in the old butterfly
that resides beyond the new length. If so, we will make a new butterfly instead.

There is no perf differences in the benchmark results. However, this does benefit
the perf of pathological cases where we need to shorten the length of a very large
array, as is the case in tests/mozilla/js1_5/Array/regress-101964.js. With this
patch, we can expect that test to complete in a short time again.

• runtime/JSArray.cpp:

(JSC::JSArray::setLength):

• runtime/JSObject.cpp:

(JSC::JSObject::reallocateAndShrinkButterfly):

• makes a new butterfly with a new shorter length.
• runtime/JSObject.h:
• tests/mozilla/js1_5/Array/regress-101964.js:
• Undo this test change since this patch will prevent us from spending a lot of time clearing a large butterfly.

Location:

trunk/Source/JavaScriptCore

Files:

• ChangeLog (modified) (1 diff)
• runtime/JSArray.cpp (modified) (3 diffs)
• runtime/JSObject.cpp (modified) (1 diff)
• runtime/JSObject.h (modified) (1 diff)
• tests/mozilla/js1_5/Array/regress-101964.js (modified) (1 diff)

trunk/Source/JavaScriptCore/ChangeLog

@@ +1 @@
+2015-05-15  Mark Lam  <mark.lam@apple.com>
+
+        JSArray::setLength() should reallocate instead of zero-filling if the reallocation would be small enough.
+        https://bugs.webkit.org/show_bug.cgi?id=144622
+
+        Reviewed by Geoffrey Garen.
+
+        When setting the array to a new length that is shorter, we now check if it is worth
+        just making a new butterfly instead of clearing out the slots in the old butterfly
+        that resides beyond the new length.  If so, we will make a new butterfly instead.
+
+        There is no perf differences in the benchmark results.  However, this does benefit
+        the perf of pathological cases where we need to shorten the length of a very large
+        array, as is the case in tests/mozilla/js1_5/Array/regress-101964.js.  With this
+        patch, we can expect that test to complete in a short time again.
+
+        * runtime/JSArray.cpp:
+        (JSC::JSArray::setLength):
+        * runtime/JSObject.cpp:
+        (JSC::JSObject::reallocateAndShrinkButterfly):
+        - makes a new butterfly with a new shorter length.
+        * runtime/JSObject.h:
+        * tests/mozilla/js1_5/Array/regress-101964.js:
+        - Undo this test change since this patch will prevent us from spending a lot of time
+          clearing a large butterfly.
+
 2015-05-15  Basile Clement  <basile_clement@apple.com>
 

trunk/Source/JavaScriptCore/runtime/JSArray.cpp

@@ -405 +405 @@
     case ArrayWithInt32:
     case ArrayWithDouble:
-    case ArrayWithContiguous:
+    case ArrayWithContiguous: {
         if (newLength == m_butterfly->publicLength())
             return true;
@@ -419 +419 @@
             return true;
         }
+
+        unsigned lengthToClear = m_butterfly->publicLength() - newLength;
+        unsigned costToAllocateNewButterfly = 64; // a heuristic.
+        if (lengthToClear > newLength && lengthToClear > costToAllocateNewButterfly) {
+            reallocateAndShrinkButterfly(exec->vm(), newLength);
+            return true;
+        }
+
         if (indexingType() == ArrayWithDouble) {
             for (unsigned i = m_butterfly->publicLength(); i-- > newLength;)
@@ -428 +436 @@
         m_butterfly->setPublicLength(newLength);
         return true;
+    }
         
     case ArrayWithArrayStorage:

trunk/Source/JavaScriptCore/runtime/JSObject.cpp

@@ -2458 +2458 @@
 }
 
+void JSObject::reallocateAndShrinkButterfly(VM& vm, unsigned length)
+{
+    ASSERT(length < MAX_ARRAY_INDEX);
+    ASSERT(length < MAX_STORAGE_VECTOR_LENGTH);
+    ASSERT(hasContiguous(indexingType()) || hasInt32(indexingType()) || hasDouble(indexingType()) || hasUndecided(indexingType()));
+    ASSERT(m_butterfly->vectorLength() > length);
+    ASSERT(!m_butterfly->indexingHeader()->preCapacity(structure()));
+
+    DeferGC deferGC(vm.heap);
+    Butterfly* newButterfly = m_butterfly->resizeArray(vm, this, structure(), 0, ArrayStorage::sizeFor(length));
+    m_butterfly.set(vm, this, newButterfly);
+    m_butterfly->setVectorLength(length);
+    m_butterfly->setPublicLength(length);
+}
+
 Butterfly* JSObject::growOutOfLineStorage(VM& vm, size_t oldSize, size_t newSize)
 {

trunk/Source/JavaScriptCore/runtime/JSObject.h

@@ -828 +828 @@
     }
         
+    // Call this if you want to shrink the butterfly backing store, and you're
+    // sure that the array is contiguous.
+    void reallocateAndShrinkButterfly(VM&, unsigned length);
+    
     template<IndexingType indexingShape>
     unsigned countElements(Butterfly*);

trunk/Source/JavaScriptCore/tests/mozilla/js1_5/Array/regress-101964.js

@@ -32 +32 @@
 var BIG = 10000000;
 var LITTLE = 10;
-var FAST = 10000; // This used to test that array truncation should be 50 ms or less. We've changed it because we don't care how long it takes. We just try to make sure it doesn't take *ridiculously* long and we want it to run to completion correctly.
+var FAST = 50; // array truncation should be 50 ms or less to pass the test
 var MSG_FAST = 'Truncation took less than ' + FAST + ' ms';
 var MSG_SLOW = 'Truncation took ';