Attachment 369013 Details for Bug 197584 – Patch

[patch] Patch

bug-197584-20190503161925.patch (text/plain), 8.59 KB, created by Yusuke Suzuki on 2019-05-03 16:19:26 PDT

(hide)

Description:

Filename:

MIME Type:

Creator: Yusuke Suzuki

Created: 2019-05-03 16:19:26 PDT

Size: 8.59 KB

patch

obsolete

>Subversion Revision: 244915
>diff --git a/Source/JavaScriptCore/ChangeLog b/Source/JavaScriptCore/ChangeLog
>index 9daa6541a3e3d51fb32b49e871d89a3527db7978..1919c6c14ab8722f3420fa850d084755801cc452 100644
>--- a/Source/JavaScriptCore/ChangeLog
>+++ b/Source/JavaScriptCore/ChangeLog
>@@ -1,3 +1,43 @@
>+2019-05-03  Yusuke Suzuki  <ysuzuki@apple.com>
>+
>+        [JSC] Need to emit SetLocal if we emit MovHint in DFGByteCodeParser
>+        https://bugs.webkit.org/show_bug.cgi?id=197584
>+
>+        Reviewed by NOBODY (OOPS!).
>+
>+        In r244864, we emit MovHint for adhocly created GetterCall/SetterCall frame locals in the callee side to make OSR availability analysis's pruning correct.
>+        However, we just emit MovHint, and we do not emit SetLocal since we ensured that these locals are already flushed in the same place before. However, MovHint
>+        and SetLocal are needed to be a pair in DFGByteCodeParser because we rely on this assumption in SSA conversion phase. SSA conversion phase always emit KillStack
>+        just before MovHint's target location even if the MovHint's target is the same to the previously emitted MovHint and SetLocal.
>+        This patch emits SetLocal too when emitting MovHint for GetterCall/SetterCall frame locals.
>+
>+        The example is like this.
>+
>+            a:  SomeValueNode
>+             :  MovHint(@a, loc10)
>+            b:  SetLocal(@a, loc10)
>+                ...
>+            c:  MovHint(@a, loc10)
>+
>+        Then, this will be converted to the style in SSA conversion.
>+
>+            a:  SomeValueNode
>+             :  KillStack(loc10)
>+            b:  PutStack(@a, loc10)
>+                ...
>+            c:  KillStack(loc10)
>+
>+        Then, @b will be removed later since @c kills it.
>+
>+        * dfg/DFGByteCodeParser.cpp:
>+        (JSC::DFG::ByteCodeParser::inlineCall):
>+        * heap/MarkedBlock.cpp:
>+        (JSC::MarkedBlock::MarkedBlock):
>+        (JSC::MarkedBlock::Handle::stopAllocating):
>+        (JSC::MarkedBlock::Handle::resumeAllocating):
>+        (JSC::MarkedBlock::aboutToMarkSlow):
>+        (JSC::MarkedBlock::Handle::didConsumeFreeList):
>+
> 2019-05-03  Yusuke Suzuki  <ysuzuki@apple.com>
> 
>         [JSC] Generator CodeBlock generation should be idempotent
>diff --git a/Source/JavaScriptCore/dfg/DFGByteCodeParser.cpp b/Source/JavaScriptCore/dfg/DFGByteCodeParser.cpp
>index 06fb181d3d71755422deb55d0e67384ec4307359..9f04275d88f7e38953a0a314ced6607d05530a6b 100644
>--- a/Source/JavaScriptCore/dfg/DFGByteCodeParser.cpp
>+++ b/Source/JavaScriptCore/dfg/DFGByteCodeParser.cpp
>@@ -1646,7 +1646,9 @@ void ByteCodeParser::inlineCall(Node* callTargetNode, VirtualRegister result, Ca
>         // callee make it so that if we exit at <HERE>, we can recover loc9 and loc10.
>         for (int index = 0; index < argumentCountIncludingThis; ++index) {
>             VirtualRegister argumentToGet = callerStackTop->remapOperand(virtualRegisterForArgument(index, registerOffset));
>-            addToGraph(MovHint, OpInfo(argumentToGet.offset()), getDirect(argumentToGet));
>+            Node* value = getDirect(argumentToGet);
>+            addToGraph(MovHint, OpInfo(argumentToGet.offset()), value);
>+            m_setLocalQueue.append(DelayedSetLocal { currentCodeOrigin(), argumentToGet, value, ImmediateNakedSet });
>         }
>         break;
>     }
>diff --git a/Source/JavaScriptCore/heap/MarkedBlock.cpp b/Source/JavaScriptCore/heap/MarkedBlock.cpp
>index b1a10a1e793e1813d40af1a10c5e057243fe12fc..4954348738a789d237be7182934078c175189053 100644
>--- a/Source/JavaScriptCore/heap/MarkedBlock.cpp
>+++ b/Source/JavaScriptCore/heap/MarkedBlock.cpp
>@@ -38,6 +38,9 @@
> #include <wtf/CommaPrinter.h>
> 
> namespace JSC {
>+namespace MarkedBlockInternal {
>+static constexpr bool verbose = false;
>+}
> 
> const size_t MarkedBlock::blockSize;
> 
>@@ -87,7 +90,7 @@ MarkedBlock::Handle::~Handle()
> MarkedBlock::MarkedBlock(VM& vm, Handle& handle)
> {
>     new (&footer()) Footer(vm, handle);
>-    if (false)
>+    if (MarkedBlockInternal::verbose)
>         dataLog(RawPointer(this), ": Allocated.\n");
> }
> 
>@@ -124,12 +127,12 @@ void MarkedBlock::Handle::stopAllocating(const FreeList& freeList)
> {
>     auto locker = holdLock(blockFooter().m_lock);
>     
>-    if (false)
>+    if (MarkedBlockInternal::verbose)
>         dataLog(RawPointer(this), ": MarkedBlock::Handle::stopAllocating!\n");
>     ASSERT(!directory()->isAllocated(NoLockingNecessary, this));
> 
>     if (!isFreeListed()) {
>-        if (false)
>+        if (MarkedBlockInternal::verbose)
>             dataLog("There ain't no newly allocated.\n");
>         // This means that we either didn't use this block at all for allocation since last GC,
>         // or someone had already done stopAllocating() before.
>@@ -137,7 +140,7 @@ void MarkedBlock::Handle::stopAllocating(const FreeList& freeList)
>         return;
>     }
>     
>-    if (false)
>+    if (MarkedBlockInternal::verbose)
>         dataLog("Free list: ", freeList, "\n");
>     
>     // Roll back to a coherent state for Heap introspection. Cells newly
>@@ -155,7 +158,7 @@ void MarkedBlock::Handle::stopAllocating(const FreeList& freeList)
> 
>     freeList.forEach(
>         [&] (HeapCell* cell) {
>-            if (false)
>+            if (MarkedBlockInternal::verbose)
>                 dataLog("Free cell: ", RawPointer(cell), "\n");
>             if (m_attributes.destruction == NeedsDestruction)
>                 cell->zap();
>@@ -183,13 +186,13 @@ void MarkedBlock::Handle::resumeAllocating(FreeList& freeList)
>     {
>         auto locker = holdLock(blockFooter().m_lock);
>         
>-        if (false)
>+        if (MarkedBlockInternal::verbose)
>             dataLog(RawPointer(this), ": MarkedBlock::Handle::resumeAllocating!\n");
>         ASSERT(!directory()->isAllocated(NoLockingNecessary, this));
>         ASSERT(!isFreeListed());
>         
>         if (!block().hasAnyNewlyAllocated()) {
>-            if (false)
>+            if (MarkedBlockInternal::verbose)
>                 dataLog("There ain't no newly allocated.\n");
>             // This means we had already exhausted the block when we stopped allocation.
>             freeList.clear();
>@@ -223,7 +226,7 @@ void MarkedBlock::aboutToMarkSlow(HeapVersion markingVersion)
> 
>     if (handle().directory()->isAllocated(holdLock(directory->bitvectorLock()), &handle())
>         || !marksConveyLivenessDuringMarking(markingVersion)) {
>-        if (false)
>+        if (MarkedBlockInternal::verbose)
>             dataLog(RawPointer(this), ": Clearing marks without doing anything else.\n");
>         // We already know that the block is full and is already recognized as such, or that the
>         // block did not survive the previous GC. So, we can clear mark bits the old fashioned
>@@ -233,7 +236,7 @@ void MarkedBlock::aboutToMarkSlow(HeapVersion markingVersion)
>         // we created a newlyAllocated.
>         footer().m_marks.clearAll();
>     } else {
>-        if (false)
>+        if (MarkedBlockInternal::verbose)
>             dataLog(RawPointer(this), ": Doing things.\n");
>         HeapVersion newlyAllocatedVersion = space()->newlyAllocatedVersion();
>         if (footer().m_newlyAllocatedVersion == newlyAllocatedVersion) {
>@@ -303,7 +306,7 @@ bool MarkedBlock::isMarked(const void* p)
> void MarkedBlock::Handle::didConsumeFreeList()
> {
>     auto locker = holdLock(blockFooter().m_lock);
>-    if (false)
>+    if (MarkedBlockInternal::verbose)
>         dataLog(RawPointer(this), ": MarkedBlock::Handle::didConsumeFreeList!\n");
>     ASSERT(isFreeListed());
>     m_isFreeListed = false;
>diff --git a/JSTests/ChangeLog b/JSTests/ChangeLog
>index 24c9dc42c6ca7ebfcc49e6b41d89458f76229aaa..72b2dca97301167d7e250ec8818c3c8df1eb0758 100644
>--- a/JSTests/ChangeLog
>+++ b/JSTests/ChangeLog
>@@ -1,3 +1,14 @@
>+2019-05-03  Yusuke Suzuki  <ysuzuki@apple.com>
>+
>+        [JSC] Need to emit SetLocal if we emit MovHint in DFGByteCodeParser
>+        https://bugs.webkit.org/show_bug.cgi?id=197584
>+
>+        Reviewed by NOBODY (OOPS!).
>+
>+        * stress/adhoc-setter-frame-should-emit-setlocal-again.js: Added.
>+        (X):
>+        (foo):
>+
> 2019-05-03  Yusuke Suzuki  <ysuzuki@apple.com>
> 
>         [JSC] Generator CodeBlock generation should be idempotent
>diff --git a/JSTests/stress/adhoc-setter-frame-should-emit-setlocal-again.js b/JSTests/stress/adhoc-setter-frame-should-emit-setlocal-again.js
>new file mode 100644
>index 0000000000000000000000000000000000000000..9898e780a97108f81f8f37346cc6d501f9b16168
>--- /dev/null
>+++ b/JSTests/stress/adhoc-setter-frame-should-emit-setlocal-again.js
>@@ -0,0 +1,17 @@
>+function X(){}
>+
>+X.prototype.__defineSetter__('f', function* () {
>+    arguments;
>+});
>+
>+function foo(o) {
>+    i+'';
>+    for (var i = 0; i < 1000; ++i) {
>+        o.f = 0;
>+    }
>+}
>+noInline(foo);
>+
>+for (let i = 0; i < 1000; ++i) {
>+    foo(new X());
>+}

Actions: View | Formatted Diff | Diff

Attachments on bug 197584: 369013 | 369014