Attachment 359349 Details for Bug 193513 – Patch

[patch] Patch

bug-193513-20190116211151.patch (text/plain), 7.45 KB, created by Yusuke Suzuki on 2019-01-16 21:11:51 PST

(hide)

Description:

Filename:

MIME Type:

Creator: Yusuke Suzuki

Created: 2019-01-16 21:11:51 PST

Size: 7.45 KB

patch

obsolete

>Subversion Revision: 240102
>diff --git a/Source/JavaScriptCore/ChangeLog b/Source/JavaScriptCore/ChangeLog
>index 347fa49aa02a81b56932835e99c3d950d849432b..0472229762ed09a2a0bac6d22efdc6b17cdbfacf 100644
>--- a/Source/JavaScriptCore/ChangeLog
>+++ b/Source/JavaScriptCore/ChangeLog
>@@ -1,3 +1,25 @@
>+2019-01-16  Yusuke Suzuki  <yusukesuzuki@slowstart.org>
>+
>+        [JSC] ToThis omission in DFGByteCodeParser is wrong
>+        https://bugs.webkit.org/show_bug.cgi?id=193513
>+        <rdar://problem/45842236>
>+
>+        Reviewed by NOBODY (OOPS!).
>+
>+        DFGByteCodeParser omitted ToThis node when we have `ToThis(ToThis(value))`. This semantics is wrong if ToThis has different semantics
>+        in the sloppy mode and the strict mode. If we convert `ToThisInSloppyMode(ToThisInStrictMode(boolean))` to `ToThisInStrictMode(boolean)`,
>+        we get boolean instead of BooleanObject.
>+
>+        This optimization is introduced more than 7 years ago, and from that, we have several optimizations that can remove such ToThis nodes
>+        in BytecodeParser, AI, and Fixup. Furthermore, this optimization is simply wrong since `toThis()` function of JSCell can be defined
>+        as they want. Before ensuring all the toThis function is safe, we should not fold `ToThis(ToThis(value))` => `ToThis(value)`.
>+        This patch just removes the problematic optimization. The performance numbers look neutral.
>+
>+        * dfg/DFGAbstractInterpreterInlines.h:
>+        (JSC::DFG::AbstractInterpreter<AbstractStateType>::executeEffects):
>+        * dfg/DFGByteCodeParser.cpp:
>+        (JSC::DFG::ByteCodeParser::parseBlock):
>+
> 2019-01-16  Mark Lam  <mark.lam@apple.com>
> 
>         Refactor new bytecode structs so that the fields are prefixed with "m_".
>diff --git a/Source/JavaScriptCore/dfg/DFGAbstractInterpreterInlines.h b/Source/JavaScriptCore/dfg/DFGAbstractInterpreterInlines.h
>index 183a2debdc77dccde283b8608960a46e3a32998c..f8b49b9f4637de5d4d228f548e1df7d93751208d 100644
>--- a/Source/JavaScriptCore/dfg/DFGAbstractInterpreterInlines.h
>+++ b/Source/JavaScriptCore/dfg/DFGAbstractInterpreterInlines.h
>@@ -2549,31 +2549,27 @@ bool AbstractInterpreter<AbstractStateType>::executeEffects(unsigned clobberLimi
>         bool strictMode = m_graph.executableFor(node->origin.semantic)->isStrictMode();
> 
>         ToThisResult result = isToThisAnIdentity(m_vm, strictMode, source);
>-        if (result != ToThisResult::Dynamic) {
>-            switch (result) {
>-            case ToThisResult::Identity:
>-                m_state.setFoundConstants(true);
>+        switch (result) {
>+        case ToThisResult::Identity:
>+            m_state.setFoundConstants(true);
>+            destination = source;
>+            break;
>+        case ToThisResult::Undefined:
>+            setConstant(node, jsUndefined());
>+            break;
>+        case ToThisResult::GlobalThis:
>+            m_state.setFoundConstants(true);
>+            destination.setType(m_graph, SpecObject);
>+            break;
>+        case ToThisResult::Dynamic:
>+            if (strictMode)
>+                destination.makeHeapTop();
>+            else {
>                 destination = source;
>-                break;
>-            case ToThisResult::Undefined:
>-                setConstant(node, jsUndefined());
>-                break;
>-            case ToThisResult::GlobalThis:
>-                m_state.setFoundConstants(true);
>-                destination.setType(m_graph, SpecObject);
>-                break;
>-            case ToThisResult::Dynamic:
>-                RELEASE_ASSERT_NOT_REACHED();
>+                destination.merge(SpecObject);
>             }
>             break;
>         }
>-
>-        if (strictMode)
>-            destination.makeHeapTop();
>-        else {
>-            destination = source;
>-            destination.merge(SpecObject);
>-        }
>         break;
>     }
> 
>diff --git a/Source/JavaScriptCore/dfg/DFGByteCodeParser.cpp b/Source/JavaScriptCore/dfg/DFGByteCodeParser.cpp
>index 8e49207954d244206d10ea0fe2ea99505320ea94..d824b3b857adc868ca06b337105459b0213380e6 100644
>--- a/Source/JavaScriptCore/dfg/DFGByteCodeParser.cpp
>+++ b/Source/JavaScriptCore/dfg/DFGByteCodeParser.cpp
>@@ -4731,22 +4731,20 @@ void ByteCodeParser::parseBlock(unsigned limit)
>             
>         case op_to_this: {
>             Node* op1 = getThis();
>-            if (op1->op() != ToThis) {
>-                auto& metadata = currentInstruction->as<OpToThis>().metadata(codeBlock);
>-                Structure* cachedStructure = metadata.m_cachedStructure.get();
>-                if (metadata.m_toThisStatus != ToThisOK
>-                    || !cachedStructure
>-                    || cachedStructure->classInfo()->methodTable.toThis != JSObject::info()->methodTable.toThis
>-                    || m_inlineStackTop->m_profiledBlock->couldTakeSlowCase(m_currentIndex)
>-                    || m_inlineStackTop->m_exitProfile.hasExitSite(m_currentIndex, BadCache)
>-                    || (op1->op() == GetLocal && op1->variableAccessData()->structureCheckHoistingFailed())) {
>-                    setThis(addToGraph(ToThis, OpInfo(), OpInfo(getPrediction()), op1));
>-                } else {
>-                    addToGraph(
>-                        CheckStructure,
>-                        OpInfo(m_graph.addStructureSet(cachedStructure)),
>-                        op1);
>-                }
>+            auto& metadata = currentInstruction->as<OpToThis>().metadata(codeBlock);
>+            Structure* cachedStructure = metadata.m_cachedStructure.get();
>+            if (metadata.m_toThisStatus != ToThisOK
>+                || !cachedStructure
>+                || cachedStructure->classInfo()->methodTable.toThis != JSObject::info()->methodTable.toThis
>+                || m_inlineStackTop->m_profiledBlock->couldTakeSlowCase(m_currentIndex)
>+                || m_inlineStackTop->m_exitProfile.hasExitSite(m_currentIndex, BadCache)
>+                || (op1->op() == GetLocal && op1->variableAccessData()->structureCheckHoistingFailed())) {
>+                setThis(addToGraph(ToThis, OpInfo(), OpInfo(getPrediction()), op1));
>+            } else {
>+                addToGraph(
>+                    CheckStructure,
>+                    OpInfo(m_graph.addStructureSet(cachedStructure)),
>+                    op1);
>             }
>             NEXT_OPCODE(op_to_this);
>         }
>diff --git a/JSTests/ChangeLog b/JSTests/ChangeLog
>index b99a96932849c401b8c94c76797db6f2546417c2..17c518328b47d69b5bca0fb729e95456493c49b9 100644
>--- a/JSTests/ChangeLog
>+++ b/JSTests/ChangeLog
>@@ -1,3 +1,15 @@
>+2019-01-16  Yusuke Suzuki  <yusukesuzuki@slowstart.org>
>+
>+        [JSC] ToThis omission in DFGByteCodeParser is wrong
>+        https://bugs.webkit.org/show_bug.cgi?id=193513
>+        <rdar://problem/45842236>
>+
>+        Reviewed by NOBODY (OOPS!).
>+
>+        * stress/to-this-omission-with-different-strict-modes.js: Added.
>+        (thisA):
>+        (thisAStrictWrapper):
>+
> 2019-01-15  Mark Lam  <mark.lam@apple.com>
> 
>         JSFunction::canUseAllocationProfile() should account for builtin functions with no own prototypes.
>diff --git a/JSTests/stress/to-this-omission-with-different-strict-modes.js b/JSTests/stress/to-this-omission-with-different-strict-modes.js
>new file mode 100644
>index 0000000000000000000000000000000000000000..2024daca194989944906e87d0053d0d718235c86
>--- /dev/null
>+++ b/JSTests/stress/to-this-omission-with-different-strict-modes.js
>@@ -0,0 +1,10 @@
>+function thisA() {
>+    return this.a
>+}
>+function thisAStrictWrapper() {
>+    'use strict';
>+    thisA.apply(this);
>+}
>+let x = false;
>+for (let j=0; j<1e4; j++)
>+    thisAStrictWrapper.call(x);

Actions: View | Formatted Diff | Diff

Attachments on bug 193513: 359344 | 359348 | 359349