Title: [240106] trunk
- Revision
- 240106
- Author
- yusukesuz...@slowstart.org
- Date
- 2019-01-17 00:10:53 -0800 (Thu, 17 Jan 2019)
Log Message
[JSC] ToThis omission in DFGByteCodeParser is wrong
https://bugs.webkit.org/show_bug.cgi?id=193513
<rdar://problem/45842236>
Reviewed by Saam Barati.
JSTests:
* stress/to-this-omission-with-different-strict-modes.js: Added.
(thisA):
(thisAStrictWrapper):
Source/_javascript_Core:
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):
Modified Paths
Added Paths
Diff
Modified: trunk/JSTests/ChangeLog (240105 => 240106)
--- trunk/JSTests/ChangeLog 2019-01-17 07:46:10 UTC (rev 240105)
+++ trunk/JSTests/ChangeLog 2019-01-17 08:10:53 UTC (rev 240106)
@@ -1,3 +1,15 @@
+2019-01-17 Yusuke Suzuki <yusukesuz...@slowstart.org>
+
+ [JSC] ToThis omission in DFGByteCodeParser is wrong
+ https://bugs.webkit.org/show_bug.cgi?id=193513
+ <rdar://problem/45842236>
+
+ Reviewed by Saam Barati.
+
+ * stress/to-this-omission-with-different-strict-modes.js: Added.
+ (thisA):
+ (thisAStrictWrapper):
+
2019-01-15 Mark Lam <mark....@apple.com>
JSFunction::canUseAllocationProfile() should account for builtin functions with no own prototypes.
Added: trunk/JSTests/stress/to-this-omission-with-different-strict-modes.js (0 => 240106)
--- trunk/JSTests/stress/to-this-omission-with-different-strict-modes.js (rev 0)
+++ trunk/JSTests/stress/to-this-omission-with-different-strict-modes.js 2019-01-17 08:10:53 UTC (rev 240106)
@@ -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);
Modified: trunk/Source/_javascript_Core/ChangeLog (240105 => 240106)
--- trunk/Source/_javascript_Core/ChangeLog 2019-01-17 07:46:10 UTC (rev 240105)
+++ trunk/Source/_javascript_Core/ChangeLog 2019-01-17 08:10:53 UTC (rev 240106)
@@ -1,3 +1,25 @@
+2019-01-17 Yusuke Suzuki <yusukesuz...@slowstart.org>
+
+ [JSC] ToThis omission in DFGByteCodeParser is wrong
+ https://bugs.webkit.org/show_bug.cgi?id=193513
+ <rdar://problem/45842236>
+
+ Reviewed by Saam Barati.
+
+ 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....@apple.com>
Refactor new bytecode structs so that the fields are prefixed with "m_".
Modified: trunk/Source/_javascript_Core/dfg/DFGAbstractInterpreterInlines.h (240105 => 240106)
--- trunk/Source/_javascript_Core/dfg/DFGAbstractInterpreterInlines.h 2019-01-17 07:46:10 UTC (rev 240105)
+++ trunk/Source/_javascript_Core/dfg/DFGAbstractInterpreterInlines.h 2019-01-17 08:10:53 UTC (rev 240106)
@@ -2549,31 +2549,27 @@
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;
}
Modified: trunk/Source/_javascript_Core/dfg/DFGByteCodeParser.cpp (240105 => 240106)
--- trunk/Source/_javascript_Core/dfg/DFGByteCodeParser.cpp 2019-01-17 07:46:10 UTC (rev 240105)
+++ trunk/Source/_javascript_Core/dfg/DFGByteCodeParser.cpp 2019-01-17 08:10:53 UTC (rev 240106)
@@ -4731,22 +4731,20 @@
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);
}
_______________________________________________
webkit-changes mailing list
webkit-changes@lists.webkit.org
https://lists.webkit.org/mailman/listinfo/webkit-changes
