Unreviewed, rolling out r222380.
authorutatane.tea@gmail.com <utatane.tea@gmail.com@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Fri, 22 Sep 2017 10:25:58 +0000 (10:25 +0000)
committerutatane.tea@gmail.com <utatane.tea@gmail.com@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Fri, 22 Sep 2017 10:25:58 +0000 (10:25 +0000)
https://bugs.webkit.org/show_bug.cgi?id=177352

Octane/box2d shows 8% regression (Requested by yusukesuzuki on
#webkit).

Reverted changeset:

"[DFG][FTL] Profile array vector length for array allocation"
https://bugs.webkit.org/show_bug.cgi?id=177051
http://trac.webkit.org/changeset/222380

Patch by Commit Queue <commit-queue@webkit.org> on 2017-09-22

git-svn-id: https://svn.webkit.org/repository/webkit/trunk@222382 268f45cc-cd09-0410-ab3c-d52691b4dbfc

15 files changed:
JSTests/ChangeLog
JSTests/microbenchmarks/new-array-buffer-vector-profile.js [deleted file]
Source/JavaScriptCore/ChangeLog
Source/JavaScriptCore/bytecode/ArrayAllocationProfile.cpp
Source/JavaScriptCore/bytecode/ArrayAllocationProfile.h
Source/JavaScriptCore/bytecode/CodeBlock.cpp
Source/JavaScriptCore/dfg/DFGByteCodeParser.cpp
Source/JavaScriptCore/dfg/DFGGraph.cpp
Source/JavaScriptCore/dfg/DFGNode.h
Source/JavaScriptCore/dfg/DFGOperations.cpp
Source/JavaScriptCore/dfg/DFGOperations.h
Source/JavaScriptCore/dfg/DFGSpeculativeJIT64.cpp
Source/JavaScriptCore/ftl/FTLLowerDFGToB3.cpp
Source/JavaScriptCore/runtime/ArrayConventions.h
Source/JavaScriptCore/runtime/JSArray.h

index ab66f77..ac411c2 100644 (file)
@@ -1,3 +1,17 @@
+2017-09-22  Commit Queue  <commit-queue@webkit.org>
+
+        Unreviewed, rolling out r222380.
+        https://bugs.webkit.org/show_bug.cgi?id=177352
+
+        Octane/box2d shows 8% regression (Requested by yusukesuzuki on
+        #webkit).
+
+        Reverted changeset:
+
+        "[DFG][FTL] Profile array vector length for array allocation"
+        https://bugs.webkit.org/show_bug.cgi?id=177051
+        http://trac.webkit.org/changeset/222380
+
 2017-09-21  Yusuke Suzuki  <utatane.tea@gmail.com>
 
         [DFG][FTL] Profile array vector length for array allocation
diff --git a/JSTests/microbenchmarks/new-array-buffer-vector-profile.js b/JSTests/microbenchmarks/new-array-buffer-vector-profile.js
deleted file mode 100644 (file)
index 1808b49..0000000
+++ /dev/null
@@ -1,12 +0,0 @@
-function target()
-{
-    var array = [0];
-    array.push(1);
-    array.push(2);
-    array.push(3);
-    array.push(4);
-    return array;
-}
-noInline(target);
-for (var i = 0; i < 1e6; ++i)
-    target();
index de234ec..321ffdd 100644 (file)
@@ -1,3 +1,17 @@
+2017-09-22  Commit Queue  <commit-queue@webkit.org>
+
+        Unreviewed, rolling out r222380.
+        https://bugs.webkit.org/show_bug.cgi?id=177352
+
+        Octane/box2d shows 8% regression (Requested by yusukesuzuki on
+        #webkit).
+
+        Reverted changeset:
+
+        "[DFG][FTL] Profile array vector length for array allocation"
+        https://bugs.webkit.org/show_bug.cgi?id=177051
+        http://trac.webkit.org/changeset/222380
+
 2017-09-21  Yusuke Suzuki  <utatane.tea@gmail.com>
 
         [DFG][FTL] Profile array vector length for array allocation
index 41ce23c..a931521 100644 (file)
@@ -30,7 +30,7 @@
 
 namespace JSC {
 
-void ArrayAllocationProfile::updateProfile()
+void ArrayAllocationProfile::updateIndexingType()
 {
     // This is awkwardly racy but totally sound even when executed concurrently. The
     // worst cases go something like this:
@@ -49,11 +49,9 @@ void ArrayAllocationProfile::updateProfile()
     JSArray* lastArray = m_lastArray;
     if (!lastArray)
         return;
-    if (LIKELY(Options::useArrayAllocationProfiling())) {
+    if (LIKELY(Options::useArrayAllocationProfiling()))
         m_currentIndexingType = leastUpperBoundOfIndexingTypes(m_currentIndexingType, lastArray->indexingType());
-        m_largestSeenVectorLength = std::min(std::max(m_largestSeenVectorLength, lastArray->getVectorLength()), BASE_CONTIGUOUS_VECTOR_LEN_MAX);
-    }
-    m_lastArray = nullptr;
+    m_lastArray = 0;
 }
 
 } // namespace JSC
index 1bfb470..cf30de6 100644 (file)
@@ -32,22 +32,19 @@ namespace JSC {
 
 class ArrayAllocationProfile {
 public:
+    ArrayAllocationProfile()
+        : m_currentIndexingType(ArrayWithUndecided)
+        , m_lastArray(0)
+    {
+    }
+    
     IndexingType selectIndexingType()
     {
         JSArray* lastArray = m_lastArray;
         if (lastArray && UNLIKELY(lastArray->indexingType() != m_currentIndexingType))
-            updateProfile();
+            updateIndexingType();
         return m_currentIndexingType;
     }
-
-    // vector length hint becomes [0, BASE_CONTIGUOUS_VECTOR_LEN_MAX].
-    unsigned vectorLengthHint()
-    {
-        JSArray* lastArray = m_lastArray;
-        if (lastArray && (m_largestSeenVectorLength != BASE_CONTIGUOUS_VECTOR_LEN_MAX) && UNLIKELY(lastArray->getVectorLength() > m_largestSeenVectorLength))
-            updateProfile();
-        return m_largestSeenVectorLength;
-    }
     
     JSArray* updateLastAllocation(JSArray* lastArray)
     {
@@ -55,7 +52,7 @@ public:
         return lastArray;
     }
     
-    JS_EXPORT_PRIVATE void updateProfile();
+    JS_EXPORT_PRIVATE void updateIndexingType();
     
     static IndexingType selectIndexingTypeFor(ArrayAllocationProfile* profile)
     {
@@ -73,9 +70,8 @@ public:
 
 private:
     
-    IndexingType m_currentIndexingType { ArrayWithUndecided };
-    unsigned m_largestSeenVectorLength { 0 };
-    JSArray* m_lastArray { nullptr };
+    IndexingType m_currentIndexingType;
+    JSArray* m_lastArray;
 };
 
 } // namespace JSC
index 8e42e1b..68defde 100644 (file)
@@ -2568,7 +2568,7 @@ void CodeBlock::updateAllArrayPredictions()
     
     // Don't count these either, for similar reasons.
     for (unsigned i = m_arrayAllocationProfiles.size(); i--;)
-        m_arrayAllocationProfiles[i].updateProfile();
+        m_arrayAllocationProfiles[i].updateIndexingType();
 }
 
 void CodeBlock::updateAllPredictions()
index d481173..331c406 100644 (file)
@@ -4399,7 +4399,6 @@ bool ByteCodeParser::parseBlock(unsigned limit)
             data.startConstant = m_inlineStackTop->m_constantBufferRemap[startConstant];
             data.numConstants = numConstants;
             data.indexingType = profile->selectIndexingType();
-            data.vectorLengthHint = std::max<unsigned>(profile->vectorLengthHint(), numConstants);
 
             // If this statement has never executed, we'll have the wrong indexing type in the profile.
             for (int i = 0; i < numConstants; ++i) {
@@ -4409,7 +4408,7 @@ bool ByteCodeParser::parseBlock(unsigned limit)
                         m_codeBlock->constantBuffer(data.startConstant)[i]);
             }
             
-            m_graph.m_newArrayBufferData.append(WTFMove(data));
+            m_graph.m_newArrayBufferData.append(data);
             set(VirtualRegister(currentInstruction[1].u.operand), addToGraph(NewArrayBuffer, OpInfo(&m_graph.m_newArrayBufferData.last())));
             NEXT_OPCODE(op_new_array_buffer);
         }
index 14f0c00..543261a 100644 (file)
@@ -321,7 +321,6 @@ void Graph::dump(PrintStream& out, const char* prefix, Node* node, DumpContext*
         for (unsigned i = 0; i < node->numConstants(); ++i)
             out.print(anotherComma, pointerDumpInContext(freeze(m_codeBlock->constantBuffer(node->startConstant())[i]), context));
         out.print("]");
-        out.print(comma, "vectorLengthHint = ", node->vectorLengthHint());
     }
     if (node->hasLazyJSValue())
         out.print(comma, node->lazyJSValue());
index 628bfc7..2bfdd1b 100644 (file)
@@ -99,7 +99,6 @@ struct MultiPutByOffsetData {
 struct NewArrayBufferData {
     unsigned startConstant;
     unsigned numConstants;
-    unsigned vectorLengthHint;
     IndexingType indexingType;
 };
 
@@ -1116,11 +1115,6 @@ public:
     {
         return newArrayBufferData()->numConstants;
     }
-
-    unsigned vectorLengthHint()
-    {
-        return newArrayBufferData()->vectorLengthHint;
-    }
     
     bool hasIndexingType()
     {
index 558ab65..c74bb35 100644 (file)
@@ -1311,25 +1311,6 @@ char* JIT_OPERATION operationNewArrayWithSize(ExecState* exec, Structure* arrayS
     return bitwise_cast<char*>(result);
 }
 
-char* JIT_OPERATION operationNewArrayWithSizeAndHint(ExecState* exec, Structure* arrayStructure, int32_t size, int32_t vectorLengthHint, Butterfly* butterfly)
-{
-    VM& vm = exec->vm();
-    NativeCallFrameTracer tracer(&vm, exec);
-    auto scope = DECLARE_THROW_SCOPE(vm);
-
-    if (UNLIKELY(size < 0))
-        return bitwise_cast<char*>(throwException(exec, scope, createRangeError(exec, ASCIILiteral("Array size is not a small enough positive integer."))));
-
-    JSArray* result;
-    if (butterfly)
-        result = JSArray::createWithButterfly(vm, nullptr, arrayStructure, butterfly);
-    else {
-        result = JSArray::tryCreate(vm, arrayStructure, size, vectorLengthHint);
-        ASSERT(result);
-    }
-    return bitwise_cast<char*>(result);
-}
-
 char* JIT_OPERATION operationNewArrayBuffer(ExecState* exec, Structure* arrayStructure, size_t start, size_t size)
 {
     VM& vm = exec->vm();
index 9978999..155322c 100644 (file)
@@ -81,7 +81,6 @@ char* JIT_OPERATION operationNewArray(ExecState*, Structure*, void*, size_t) WTF
 char* JIT_OPERATION operationNewArrayBuffer(ExecState*, Structure*, size_t, size_t) WTF_INTERNAL;
 char* JIT_OPERATION operationNewEmptyArray(ExecState*, Structure*) WTF_INTERNAL;
 char* JIT_OPERATION operationNewArrayWithSize(ExecState*, Structure*, int32_t, Butterfly*) WTF_INTERNAL;
-char* JIT_OPERATION operationNewArrayWithSizeAndHint(ExecState*, Structure*, int32_t, int32_t, Butterfly*) WTF_INTERNAL;
 char* JIT_OPERATION operationNewInt8ArrayWithSize(ExecState*, Structure*, int32_t, char*) WTF_INTERNAL;
 char* JIT_OPERATION operationNewInt8ArrayWithOneArgument(ExecState*, Structure*, EncodedJSValue) WTF_INTERNAL;
 char* JIT_OPERATION operationNewInt16ArrayWithSize(ExecState*, Structure*, int32_t, char*) WTF_INTERNAL;
index 06da4dc..e353733 100644 (file)
@@ -4238,8 +4238,6 @@ void SpeculativeJIT::compile(Node* node)
         IndexingType indexingType = node->indexingType();
         if (!globalObject->isHavingABadTime() && !hasAnyArrayStorage(indexingType)) {
             unsigned numElements = node->numConstants();
-            unsigned vectorLengthHint = node->vectorLengthHint();
-            ASSERT(vectorLengthHint >= numElements);
             
             GPRTemporary result(this);
             GPRTemporary storage(this);
@@ -4247,7 +4245,7 @@ void SpeculativeJIT::compile(Node* node)
             GPRReg resultGPR = result.gpr();
             GPRReg storageGPR = storage.gpr();
 
-            emitAllocateRawObject(resultGPR, m_jit.graph().registerStructure(globalObject->arrayStructureForIndexingTypeDuringAllocation(indexingType)), storageGPR, numElements, vectorLengthHint);
+            emitAllocateRawObject(resultGPR, m_jit.graph().registerStructure(globalObject->arrayStructureForIndexingTypeDuringAllocation(indexingType)), storageGPR, numElements, numElements);
             
             DFG_ASSERT(m_jit.graph(), node, indexingType & IsArray);
             JSValue* data = m_jit.codeBlock()->constantBuffer(node->startConstant());
index cce0fc9..00a2054 100644 (file)
@@ -4189,7 +4189,7 @@ private:
                 m_out.select(m_out.equal(indexingType, m_out.constInt32(ArrayWithContiguous)),
                     weakStructure(m_graph.registerStructure(globalObject->arrayStructureForIndexingTypeDuringAllocation(ArrayWithContiguous))),
                     weakStructure(m_graph.registerStructure(globalObject->arrayStructureForIndexingTypeDuringAllocation(ArrayWithDouble)))));
-            arrayResult = allocateJSArray(resultLength, resultLength, structure, indexingType, false, false);
+            arrayResult = allocateJSArray(resultLength, structure, indexingType, false, false);
         }
 
         LBasicBlock loop = m_out.newBlock();
@@ -5114,11 +5114,9 @@ private:
         
         if (!globalObject->isHavingABadTime() && !hasAnyArrayStorage(m_node->indexingType())) {
             unsigned numElements = m_node->numConstants();
-            unsigned vectorLengthHint = m_node->vectorLengthHint();
-           
-            ASSERT(vectorLengthHint >= numElements);
+            
             ArrayValues arrayValues =
-                allocateUninitializedContiguousJSArray(numElements, vectorLengthHint, structure);
+                allocateUninitializedContiguousJSArray(m_out.constInt32(numElements), structure);
             
             JSValue* data = codeBlock()->constantBuffer(m_node->startConstant());
             for (unsigned index = 0; index < m_node->numConstants(); ++index) {
@@ -5157,7 +5155,7 @@ private:
             IndexingType indexingType = m_node->indexingType();
             setJSValue(
                 allocateJSArray(
-                    publicLength, publicLength, weakPointer(globalObject->arrayStructureForIndexingTypeDuringAllocation(indexingType)), m_out.constInt32(indexingType)).array);
+                    publicLength, weakPointer(globalObject->arrayStructureForIndexingTypeDuringAllocation(indexingType)), m_out.constInt32(indexingType)).array);
             mutatorFence();
             return;
         }
@@ -11443,7 +11441,7 @@ private:
         LValue butterfly;
     };
 
-    ArrayValues allocateJSArray(LValue publicLength, LValue vectorLength, LValue structure, LValue indexingType, bool shouldInitializeElements = true, bool shouldLargeArraySizeCreateArrayStorage = true)
+    ArrayValues allocateJSArray(LValue publicLength, LValue structure, LValue indexingType, bool shouldInitializeElements = true, bool shouldLargeArraySizeCreateArrayStorage = true)
     {
         JSGlobalObject* globalObject = m_graph.globalObjectFor(m_node->origin.semantic);
         if (indexingType->hasInt32()) {
@@ -11462,18 +11460,6 @@ private:
         LBasicBlock slowCase = m_out.newBlock();
         
         LBasicBlock lastNext = m_out.insertNewBlocksBefore(fastCase);
-
-        if (vectorLength->hasInt32() && structure->hasIntPtr()) {
-            unsigned vectorLengthConst = static_cast<unsigned>(vectorLength->asInt32());
-            if (vectorLengthConst <= MAX_STORAGE_VECTOR_LENGTH) {
-                vectorLength = m_out.constInt32(
-                    Butterfly::optimalContiguousVectorLength(
-                        bitwise_cast<Structure*>(structure->asIntPtr())->outOfLineCapacity(), vectorLengthConst));
-            }
-        } else {
-            // We don't compute the optimal vector length for new Array(blah) where blah is not
-            // statically known, since the compute effort of doing it here is probably not worth it.
-        }
         
         ValueFromBlock noButterfly = m_out.anchor(m_out.intPtrZero);
         
@@ -11486,6 +11472,22 @@ private:
         m_out.branch(predicate, rarely(largeCase), usually(fastCase));
         
         m_out.appendTo(fastCase, largeCase);
+
+        LValue vectorLength = nullptr;
+        if (publicLength->hasInt32() && structure->hasIntPtr()) {
+            unsigned publicLengthConst = static_cast<unsigned>(publicLength->asInt32());
+            if (publicLengthConst <= MAX_STORAGE_VECTOR_LENGTH) {
+                vectorLength = m_out.constInt32(
+                    Butterfly::optimalContiguousVectorLength(
+                        bitwise_cast<Structure*>(structure->asIntPtr())->outOfLineCapacity(), publicLengthConst));
+            }
+        }
+        
+        if (!vectorLength) {
+            // We don't compute the optimal vector length for new Array(blah) where blah is not
+            // statically known, since the compute effort of doing it here is probably not worth it.
+            vectorLength = publicLength;
+        }
             
         LValue payloadSize =
             m_out.shl(m_out.zeroExt(vectorLength, pointerType()), m_out.constIntPtr(3));
@@ -11531,10 +11533,10 @@ private:
         LValue slowResultValue = lazySlowPath(
             [=, &vm] (const Vector<Location>& locations) -> RefPtr<LazySlowPath::Generator> {
                 return createLazyCallGenerator(vm,
-                    operationNewArrayWithSizeAndHint, locations[0].directGPR(),
-                    locations[1].directGPR(), locations[2].directGPR(), locations[3].directGPR(), locations[4].directGPR());
+                    operationNewArrayWithSize, locations[0].directGPR(),
+                    locations[1].directGPR(), locations[2].directGPR(), locations[3].directGPR());
             },
-            structureValue, publicLength, vectorLength, butterflyValue);
+            structureValue, publicLength, butterflyValue);
         ValueFromBlock slowResult = m_out.anchor(slowResultValue);
         ValueFromBlock slowButterfly = m_out.anchor(
             m_out.loadPtr(slowResultValue, m_heaps.JSObject_butterfly));
@@ -11546,25 +11548,14 @@ private:
             m_out.phi(pointerType(), fastButterfly, slowButterfly));
     }
     
-    ArrayValues allocateUninitializedContiguousJSArrayInternal(LValue publicLength, LValue vectorLength, RegisteredStructure structure)
+    ArrayValues allocateUninitializedContiguousJSArray(LValue publicLength, RegisteredStructure structure)
     {
         bool shouldInitializeElements = false;
         bool shouldLargeArraySizeCreateArrayStorage = false;
         return allocateJSArray(
-            publicLength, vectorLength, weakStructure(structure), m_out.constInt32(structure->indexingType()), shouldInitializeElements,
+            publicLength, weakStructure(structure), m_out.constInt32(structure->indexingType()), shouldInitializeElements,
             shouldLargeArraySizeCreateArrayStorage);
     }
-
-    ArrayValues allocateUninitializedContiguousJSArray(LValue publicLength, RegisteredStructure structure)
-    {
-        return allocateUninitializedContiguousJSArrayInternal(publicLength, publicLength, structure);
-    }
-
-    ArrayValues allocateUninitializedContiguousJSArray(unsigned publicLength, unsigned vectorLength, RegisteredStructure structure)
-    {
-        ASSERT(vectorLength >= publicLength);
-        return allocateUninitializedContiguousJSArrayInternal(m_out.constInt32(publicLength), m_out.constInt32(vectorLength), structure);
-    }
     
     LValue ensureShadowChickenPacket()
     {
index aab19d0..5fc84c4 100644 (file)
@@ -77,8 +77,6 @@ static_assert(MAX_STORAGE_VECTOR_INDEX <= MAX_ARRAY_INDEX, "MAX_STORAGE_VECTOR_I
 // for an array that was created with a sepcified length (e.g. a = new Array(123))
 #define BASE_CONTIGUOUS_VECTOR_LEN 3U
 #define BASE_CONTIGUOUS_VECTOR_LEN_EMPTY 5U
-#define BASE_CONTIGUOUS_VECTOR_LEN_MIN 3U
-#define BASE_CONTIGUOUS_VECTOR_LEN_MAX 25U
 #define BASE_ARRAY_STORAGE_VECTOR_LEN 4U
 
 // The upper bound to the size we'll grow a zero length array when the first element
index 1a1a40e..5eaab13 100644 (file)
@@ -54,7 +54,6 @@ protected:
 
 public:
     static JSArray* tryCreate(VM&, Structure*, unsigned initialLength = 0);
-    static JSArray* tryCreate(VM&, Structure*, unsigned initialLength, unsigned vectorLengthHint);
     static JSArray* create(VM&, Structure*, unsigned initialLength = 0);
     static JSArray* createWithButterfly(VM&, GCDeferralContext*, Structure*, Butterfly*);
 
@@ -216,9 +215,8 @@ inline Butterfly* tryCreateArrayButterfly(VM& vm, JSCell* intendedOwner, unsigne
 Butterfly* createArrayButterflyInDictionaryIndexingMode(
     VM&, JSCell* intendedOwner, unsigned initialLength);
 
-inline JSArray* JSArray::tryCreate(VM& vm, Structure* structure, unsigned initialLength, unsigned vectorLengthHint)
+inline JSArray* JSArray::tryCreate(VM& vm, Structure* structure, unsigned initialLength)
 {
-    ASSERT(vectorLengthHint >= initialLength);
     unsigned outOfLineStorage = structure->outOfLineCapacity();
 
     Butterfly* butterfly;
@@ -230,10 +228,10 @@ inline JSArray* JSArray::tryCreate(VM& vm, Structure* structure, unsigned initia
             || hasDouble(indexingType)
             || hasContiguous(indexingType));
 
-        if (UNLIKELY(vectorLengthHint > MAX_STORAGE_VECTOR_LENGTH))
+        if (UNLIKELY(initialLength > MAX_STORAGE_VECTOR_LENGTH))
             return nullptr;
 
-        unsigned vectorLength = Butterfly::optimalContiguousVectorLength(structure, vectorLengthHint);
+        unsigned vectorLength = Butterfly::optimalContiguousVectorLength(structure, initialLength);
         void* temp = vm.jsValueGigacageAuxiliarySpace.tryAllocate(nullptr, Butterfly::totalSize(0, outOfLineStorage, true, vectorLength * sizeof(EncodedJSValue)));
         if (!temp)
             return nullptr;
@@ -258,11 +256,6 @@ inline JSArray* JSArray::tryCreate(VM& vm, Structure* structure, unsigned initia
     return createWithButterfly(vm, nullptr, structure, butterfly);
 }
 
-inline JSArray* JSArray::tryCreate(VM& vm, Structure* structure, unsigned initialLength)
-{
-    return tryCreate(vm, structure, initialLength, initialLength);
-}
-
 inline JSArray* JSArray::create(VM& vm, Structure* structure, unsigned initialLength)
 {
     JSArray* result = JSArray::tryCreate(vm, structure, initialLength);