Attachment 342880 Details for Bug 186721 – Patch

[patch] Patch

bug-186721-20180617004314.patch (text/plain), 8.84 KB, created by Yusuke Suzuki on 2018-06-16 08:43:15 PDT

(hide)

Description:

Filename:

MIME Type:

Creator: Yusuke Suzuki

Created: 2018-06-16 08:43:15 PDT

Size: 8.84 KB

patch

obsolete

>Subversion Revision: 232902
>diff --git a/Source/JavaScriptCore/ChangeLog b/Source/JavaScriptCore/ChangeLog
>index da5b38eee1bb5c58520859cf11524c61e114b2f3..df09f24790de338326f9dc31ca21a615c886b87c 100644
>--- a/Source/JavaScriptCore/ChangeLog
>+++ b/Source/JavaScriptCore/ChangeLog
>@@ -1,3 +1,32 @@
>+2018-06-16  Yusuke Suzuki  <utatane.tea@gmail.com>
>+
>+        [DFG] Reduce OSRExit for Kraken/crypto-aes due to CoW array
>+        https://bugs.webkit.org/show_bug.cgi?id=186721
>+
>+        Reviewed by NOBODY (OOPS!).
>+
>+        We still have several other OSRExits, but this patch reduces that.
>+
>+        1. While ArraySlice code accepts CoW arrays, it always emits CheckStructure without CoW Array structures.
>+        So DFG emits ArraySlice onto CoW arrays, and always performs OSRExits.
>+
>+        2. The CoW patch removed ArrayAllocationProfile updates. This makes allocated JSImmutableButterfly
>+        non-appropriate.
>+
>+        These changes a bit fix Kraken/crypto-aes regression.
>+
>+                                      baseline                  patched
>+
>+        stanford-crypto-aes        63.718+-2.312      ^      56.140+-0.966         ^ definitely 1.1350x faster
>+
>+
>+        * dfg/DFGByteCodeParser.cpp:
>+        (JSC::DFG::ByteCodeParser::handleIntrinsicCall):
>+        * ftl/FTLOperations.cpp:
>+        (JSC::FTL::operationMaterializeObjectInOSR):
>+        * runtime/CommonSlowPaths.cpp:
>+        (JSC::SLOW_PATH_DECL):
>+
> 2018-06-15  Yusuke Suzuki  <utatane.tea@gmail.com>
> 
>         [DFG][FTL] Spread onto PhantomNewArrayBuffer assumes JSFixedArray, but JSImmutableButterfly is returned
>diff --git a/Source/JavaScriptCore/dfg/DFGByteCodeParser.cpp b/Source/JavaScriptCore/dfg/DFGByteCodeParser.cpp
>index 2541c17474f8236ff0dd6373348e204256e7660c..a2f9237124939e33995894ce213579b9c04c8f38 100644
>--- a/Source/JavaScriptCore/dfg/DFGByteCodeParser.cpp
>+++ b/Source/JavaScriptCore/dfg/DFGByteCodeParser.cpp
>@@ -2303,6 +2303,9 @@ bool ByteCodeParser::handleIntrinsicCall(Node* callee, int resultOperand, Intrin
>                 structureSet.add(globalObject->originalArrayStructureForIndexingType(ArrayWithInt32));
>                 structureSet.add(globalObject->originalArrayStructureForIndexingType(ArrayWithContiguous));
>                 structureSet.add(globalObject->originalArrayStructureForIndexingType(ArrayWithDouble));
>+                structureSet.add(globalObject->originalArrayStructureForIndexingType(CopyOnWriteArrayWithInt32));
>+                structureSet.add(globalObject->originalArrayStructureForIndexingType(CopyOnWriteArrayWithContiguous));
>+                structureSet.add(globalObject->originalArrayStructureForIndexingType(CopyOnWriteArrayWithDouble));
>                 addToGraph(CheckStructure, OpInfo(m_graph.addStructureSet(structureSet)), array);
> 
>                 addVarArgChild(array);
>diff --git a/Source/JavaScriptCore/ftl/FTLOperations.cpp b/Source/JavaScriptCore/ftl/FTLOperations.cpp
>index b016e188aded9a9d5282b5f50c9f2eede3ba39c7..147bb2c12fb53c4e0b79e5bee5bcae6e1669e14b 100644
>--- a/Source/JavaScriptCore/ftl/FTLOperations.cpp
>+++ b/Source/JavaScriptCore/ftl/FTLOperations.cpp
>@@ -28,7 +28,9 @@
> 
> #if ENABLE(FTL_JIT)
> 
>+#include "BytecodeStructs.h"
> #include "ClonedArguments.h"
>+#include "CommonSlowPaths.h"
> #include "DirectArguments.h"
> #include "FTLJITCode.h"
> #include "FTLLazySlowPath.h"
>@@ -459,24 +461,47 @@ extern "C" JSCell* JIT_OPERATION operationMaterializeObjectInOSR(
>     }
> 
>     case PhantomNewArrayBuffer: {
>-        JSImmutableButterfly* array = nullptr;
>+        JSImmutableButterfly* immutableButterfly = nullptr;
>         for (unsigned i = materialization->properties().size(); i--;) {
>             const ExitPropertyValue& property = materialization->properties()[i];
>             if (property.location().kind() == NewArrayBufferPLoc) {
>-                array = jsCast<JSImmutableButterfly*>(JSValue::decode(values[i]));
>+                immutableButterfly = jsCast<JSImmutableButterfly*>(JSValue::decode(values[i]));
>                 break;
>             }
>         }
>-        RELEASE_ASSERT(array);
>+        RELEASE_ASSERT(immutableButterfly);
> 
>         // For now, we use array allocation profile in the actual CodeBlock. It is OK since current NewArrayBuffer
>         // and PhantomNewArrayBuffer are always bound to a specific op_new_array_buffer.
>         CodeBlock* codeBlock = baselineCodeBlockForOriginAndBaselineCodeBlock(materialization->origin(), exec->codeBlock());
>         Instruction* currentInstruction = &codeBlock->instructions()[materialization->origin().bytecodeIndex];
>         RELEASE_ASSERT(Interpreter::getOpcodeID(currentInstruction[0].u.opcode) == op_new_array_buffer);
>-        Structure* structure = exec->lexicalGlobalObject()->arrayStructureForIndexingTypeDuringAllocation(currentInstruction[3].u.arrayAllocationProfile->selectIndexingType());
>+        auto* newArrayBuffer = bitwise_cast<OpNewArrayBuffer*>(currentInstruction);
>+        ArrayAllocationProfile* profile = currentInstruction[3].u.arrayAllocationProfile;
>+
>+        // FIXME: Share the code with CommonSlowPaths. Currently, codeBlock etc. are slightly different.
>+        IndexingType indexingMode = profile->selectIndexingType();
>+        Structure* structure = exec->lexicalGlobalObject()->arrayStructureForIndexingTypeDuringAllocation(indexingMode);
>+        ASSERT(isCopyOnWrite(indexingMode));
>         ASSERT(!structure->outOfLineCapacity());
>-        return JSArray::createWithButterfly(vm, nullptr, structure, array->toButterfly());
>+
>+        if (UNLIKELY(immutableButterfly->indexingMode() != indexingMode)) {
>+            auto* newButterfly = JSImmutableButterfly::create(vm, indexingMode, immutableButterfly->length());
>+            for (unsigned i = 0; i < immutableButterfly->length(); ++i)
>+                newButterfly->setIndex(vm, i, immutableButterfly->get(i));
>+            immutableButterfly = newButterfly;
>+
>+            // FIXME: This is kinda gross and only works because we can't inline new_array_bufffer in the baseline.
>+            // We also cannot allocate a new butterfly from compilation threads since it's invalid to allocate cells from
>+            // a compilation thread.
>+            WTF::storeStoreFence();
>+            codeBlock->constantRegister(newArrayBuffer->immutableButterfly()).set(vm, codeBlock, immutableButterfly);
>+            WTF::storeStoreFence();
>+        }
>+
>+        JSArray* result = CommonSlowPaths::allocateNewArrayBuffer(vm, structure, immutableButterfly);
>+        ArrayAllocationProfile::updateLastAllocationFor(profile, result);
>+        return result;
>     }
> 
>     case PhantomNewArrayWithSpread: {
>diff --git a/Source/JavaScriptCore/runtime/CommonSlowPaths.cpp b/Source/JavaScriptCore/runtime/CommonSlowPaths.cpp
>index 1dafc63caff78ebafa37f06a2b5d6748dde3fee3..dabd9c037af1d395d4405b6f83b1b2c0ead4798f 100644
>--- a/Source/JavaScriptCore/runtime/CommonSlowPaths.cpp
>+++ b/Source/JavaScriptCore/runtime/CommonSlowPaths.cpp
>@@ -1163,6 +1163,7 @@ SLOW_PATH_DECL(slow_path_new_array_buffer)
> 
>     JSArray* result = CommonSlowPaths::allocateNewArrayBuffer(vm, structure, immutableButterfly);
>     ASSERT(isCopyOnWrite(result->indexingMode()) || exec->lexicalGlobalObject()->isHavingABadTime());
>+    ArrayAllocationProfile::updateLastAllocationFor(profile, result);
>     RETURN(result);
> }
> 
>diff --git a/JSTests/ChangeLog b/JSTests/ChangeLog
>index dd389dbdd2e994b42514f0a923ddb1154241a5cb..2c408bae41d0c30690904b33a314c48cb625592e 100644
>--- a/JSTests/ChangeLog
>+++ b/JSTests/ChangeLog
>@@ -1,3 +1,16 @@
>+2018-06-16  Yusuke Suzuki  <utatane.tea@gmail.com>
>+
>+        [DFG] Reduce OSRExit for Kraken/crypto-aes due to CoW array
>+        https://bugs.webkit.org/show_bug.cgi?id=186721
>+
>+        Reviewed by NOBODY (OOPS!).
>+
>+        * stress/array-slice-cow.js: Added.
>+        (shouldBe):
>+        (testInt32):
>+        (testDouble):
>+        (testContiguous):
>+
> 2018-06-15  Yusuke Suzuki  <utatane.tea@gmail.com>
> 
>         [DFG][FTL] Spread onto PhantomNewArrayBuffer assumes JSFixedArray, but JSImmutableButterfly is returned
>diff --git a/JSTests/stress/array-slice-cow.js b/JSTests/stress/array-slice-cow.js
>new file mode 100644
>index 0000000000000000000000000000000000000000..57f1ccf78e9047821f5b65ed8735dc8aedde31b7
>--- /dev/null
>+++ b/JSTests/stress/array-slice-cow.js
>@@ -0,0 +1,31 @@
>+function shouldBe(actual, expected) {
>+    if (actual !== expected)
>+        throw new Error('bad value: ' + actual);
>+}
>+
>+function testInt32()
>+{
>+    var array = [0, 1, 2, 3];
>+    return array.slice(1);
>+}
>+noInline(testInt32);
>+
>+function testDouble()
>+{
>+    var array = [0.1, 1.1, 2.1, 3.1];
>+    return array.slice(1);
>+}
>+noInline(testDouble);
>+
>+function testContiguous()
>+{
>+    var array = [true, false, true, false];
>+    return array.slice(1);
>+}
>+noInline(testContiguous);
>+
>+for (var i = 0; i < 1e4; ++i) {
>+    shouldBe(JSON.stringify(testInt32()), `[1,2,3]`);
>+    shouldBe(JSON.stringify(testDouble()), `[1.1,2.1,3.1]`);
>+    shouldBe(JSON.stringify(testContiguous()), `[false,true,false]`);
>+}

Flags:

keith_miller: review+

Actions: View | Formatted Diff | Diff

Attachments on bug 186721: 342879 | 342880