[JSC] Should not emit get_by_id for indexed property access
authorutatane.tea@gmail.com <utatane.tea@gmail.com@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Mon, 14 Dec 2015 02:52:51 +0000 (02:52 +0000)
committerutatane.tea@gmail.com <utatane.tea@gmail.com@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Mon, 14 Dec 2015 02:52:51 +0000 (02:52 +0000)
https://bugs.webkit.org/show_bug.cgi?id=151354

Reviewed by Darin Adler.

Before this patch, `a["1"]` is converted to `a.1` get_by_id operation in the bytecode compiler.
get_by_id emits IC. IC rely on the fact that Structure transition occur when adding / removing object's properties.
However, it's not true for indexed element properties. They are stored in the element storage and Structure transition does not occur.

For example, in the following case,

     function getOne(a) { return a['1']; }

     for (var i = 0; i < 36; ++i)
         getOne({2: true});

     if (!getOne({1: true}))
         throw new Error("OUT");

In this case, `a['1']` creates get_by_id. `getOne({2: true})` calls makes getOne's get_by_id to create IC says that,
"when comming this structure chain, there is no property in "1", so we should return `undefined`".

After that, we call `getOne({1: true})`. But in this case, `{2: true}` and `{1: true}` have the same structure chain,
because indexed property addition does not occur structure transition.
So previous IC fast path is used and return `undefined`. But the correct answer is returning `true`.

This patch fixes the above issue. When there is string bracket access, we only emits get_by_id if the given string is not an index.
There are bugs in get_by_id, put_by_id, put_by_id (direct). But only get_by_id poses user observable issue.
Because in the put_by_id case, the generic path just says "this put is uncacheable".

* bytecompiler/BytecodeGenerator.cpp:
(JSC::BytecodeGenerator::emitGetById):
(JSC::BytecodeGenerator::emitPutById):
(JSC::BytecodeGenerator::emitDirectPutById):
* bytecompiler/NodesCodegen.cpp:
(JSC::isNonIndexStringElement):
(JSC::BracketAccessorNode::emitBytecode):
(JSC::FunctionCallBracketNode::emitBytecode):
(JSC::AssignBracketNode::emitBytecode):
(JSC::ObjectPatternNode::bindValue):
* tests/stress/element-property-get-should-not-handled-with-get-by-id.js: Added.
(getOne):

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

Source/JavaScriptCore/ChangeLog
Source/JavaScriptCore/bytecompiler/BytecodeGenerator.cpp
Source/JavaScriptCore/bytecompiler/NodesCodegen.cpp
Source/JavaScriptCore/tests/stress/element-property-get-should-not-handled-with-get-by-id.js [new file with mode: 0644]

index 1e95d62..a0c1a56 100644 (file)
@@ -1,3 +1,48 @@
+2015-12-13  Yusuke Suzuki  <utatane.tea@gmail.com>
+
+        [JSC] Should not emit get_by_id for indexed property access
+        https://bugs.webkit.org/show_bug.cgi?id=151354
+
+        Reviewed by Darin Adler.
+
+        Before this patch, `a["1"]` is converted to `a.1` get_by_id operation in the bytecode compiler.
+        get_by_id emits IC. IC rely on the fact that Structure transition occur when adding / removing object's properties.
+        However, it's not true for indexed element properties. They are stored in the element storage and Structure transition does not occur.
+
+        For example, in the following case,
+
+             function getOne(a) { return a['1']; }
+
+             for (var i = 0; i < 36; ++i)
+                 getOne({2: true});
+
+             if (!getOne({1: true}))
+                 throw new Error("OUT");
+
+        In this case, `a['1']` creates get_by_id. `getOne({2: true})` calls makes getOne's get_by_id to create IC says that,
+        "when comming this structure chain, there is no property in "1", so we should return `undefined`".
+
+        After that, we call `getOne({1: true})`. But in this case, `{2: true}` and `{1: true}` have the same structure chain,
+        because indexed property addition does not occur structure transition.
+        So previous IC fast path is used and return `undefined`. But the correct answer is returning `true`.
+
+        This patch fixes the above issue. When there is string bracket access, we only emits get_by_id if the given string is not an index.
+        There are bugs in get_by_id, put_by_id, put_by_id (direct). But only get_by_id poses user observable issue.
+        Because in the put_by_id case, the generic path just says "this put is uncacheable".
+
+        * bytecompiler/BytecodeGenerator.cpp:
+        (JSC::BytecodeGenerator::emitGetById):
+        (JSC::BytecodeGenerator::emitPutById):
+        (JSC::BytecodeGenerator::emitDirectPutById):
+        * bytecompiler/NodesCodegen.cpp:
+        (JSC::isNonIndexStringElement):
+        (JSC::BracketAccessorNode::emitBytecode):
+        (JSC::FunctionCallBracketNode::emitBytecode):
+        (JSC::AssignBracketNode::emitBytecode):
+        (JSC::ObjectPatternNode::bindValue):
+        * tests/stress/element-property-get-should-not-handled-with-get-by-id.js: Added.
+        (getOne):
+
 2015-12-13  Andreas Kling  <akling@apple.com>
 
         CachedScript could have a copy-free path for all-ASCII scripts.
index dc9b370..7856e30 100644 (file)
@@ -2284,6 +2284,8 @@ RegisterID* BytecodeGenerator::emitInstanceOfCustom(RegisterID* dst, RegisterID*
 
 RegisterID* BytecodeGenerator::emitGetById(RegisterID* dst, RegisterID* base, const Identifier& property)
 {
+    ASSERT_WITH_MESSAGE(!parseIndex(property), "Indexed properties should be handled with get_by_val.");
+
     m_codeBlock->addPropertyAccessInstruction(instructions().size());
 
     UnlinkedValueProfile profile = emitProfiledOpcode(op_get_by_id);
@@ -2300,6 +2302,8 @@ RegisterID* BytecodeGenerator::emitGetById(RegisterID* dst, RegisterID* base, co
 
 RegisterID* BytecodeGenerator::emitPutById(RegisterID* base, const Identifier& property, RegisterID* value)
 {
+    ASSERT_WITH_MESSAGE(!parseIndex(property), "Indexed properties should be handled with put_by_val.");
+
     unsigned propertyIndex = addConstant(property);
 
     m_staticPropertyAnalyzer.putById(base->index(), propertyIndex);
@@ -2321,7 +2325,8 @@ RegisterID* BytecodeGenerator::emitPutById(RegisterID* base, const Identifier& p
 
 RegisterID* BytecodeGenerator::emitDirectPutById(RegisterID* base, const Identifier& property, RegisterID* value, PropertyNode::PutType putType)
 {
-    ASSERT(!parseIndex(property));
+    ASSERT_WITH_MESSAGE(!parseIndex(property), "Indexed properties should be handled with put_by_val(direct).");
+
     unsigned propertyIndex = addConstant(property);
 
     m_staticPropertyAnalyzer.putById(base->index(), propertyIndex);
index 4794b3c..8790509 100644 (file)
@@ -602,11 +602,16 @@ void PropertyListNode::emitPutConstantProperty(BytecodeGenerator& generator, Reg
 
 // ------------------------------ BracketAccessorNode --------------------------------
 
+static bool isNonIndexStringElement(ExpressionNode& element)
+{
+    return element.isString() && !parseIndex(static_cast<StringNode&>(element).value());
+}
+
 RegisterID* BracketAccessorNode::emitBytecode(BytecodeGenerator& generator, RegisterID* dst)
 {
     if (m_base->isSuperNode()) {
         // FIXME: Should we generate the profiler info?
-        if (m_subscript->isString()) {
+        if (isNonIndexStringElement(*m_subscript)) {
             const Identifier& id = static_cast<StringNode*>(m_subscript)->value();
             return generator.emitGetById(generator.finalDestination(dst), emitSuperBaseForCallee(generator), id);
         }
@@ -616,7 +621,7 @@ RegisterID* BracketAccessorNode::emitBytecode(BytecodeGenerator& generator, Regi
     RegisterID* ret;
     RegisterID* finalDest = generator.finalDestination(dst);
 
-    if (m_subscript->isString()) {
+    if (isNonIndexStringElement(*m_subscript)) {
         RefPtr<RegisterID> base = generator.emitNode(m_base);
         ret = generator.emitGetById(finalDest, base.get(), static_cast<StringNode*>(m_subscript)->value());
     } else {
@@ -846,20 +851,20 @@ RegisterID* BytecodeIntrinsicNode::emit_intrinsic_toString(BytecodeGenerator& ge
 RegisterID* FunctionCallBracketNode::emitBytecode(BytecodeGenerator& generator, RegisterID* dst)
 {
     bool baseIsSuper = m_base->isSuperNode();
-    bool subscriptIsString = m_subscript->isString();
+    bool subscriptIsNonIndexString = isNonIndexStringElement(*m_subscript);
 
     RefPtr<RegisterID> base;
     if (baseIsSuper)
         base = emitSuperBaseForCallee(generator);
     else {
-        if (subscriptIsString)
+        if (subscriptIsNonIndexString)
             base = generator.emitNode(m_base);
         else
             base = generator.emitNodeForLeftHandSide(m_base, m_subscriptHasAssignments, m_subscript->isPure(generator));
     }
 
     RefPtr<RegisterID> function;
-    if (subscriptIsString) {
+    if (subscriptIsNonIndexString) {
         generator.emitExpressionInfo(subexpressionDivot(), subexpressionStart(), subexpressionEnd());
         function = generator.emitGetById(generator.tempDestination(dst), base.get(), static_cast<StringNode*>(m_subscript)->value());
     } else {
@@ -1977,7 +1982,7 @@ RegisterID* AssignBracketNode::emitBytecode(BytecodeGenerator& generator, Regist
     generator.emitExpressionInfo(divot(), divotStart(), divotEnd());
     RegisterID* forwardResult = (dst == generator.ignoredResult()) ? result.get() : generator.moveToDestinationIfNeeded(generator.tempDestination(result.get()), result.get());
 
-    if (m_subscript->isString())
+    if (isNonIndexStringElement(*m_subscript))
         generator.emitPutById(base.get(), static_cast<StringNode*>(m_subscript)->value(), forwardResult);
     else
         generator.emitPutByVal(base.get(), property.get(), forwardResult);
@@ -3485,9 +3490,16 @@ void ObjectPatternNode::bindValue(BytecodeGenerator& generator, RegisterID* rhs)
     for (size_t i = 0; i < m_targetPatterns.size(); i++) {
         auto& target = m_targetPatterns[i];
         RefPtr<RegisterID> temp = generator.newTemporary();
-        if (!target.propertyExpression)
-            generator.emitGetById(temp.get(), rhs, target.propertyName);
-        else {
+        if (!target.propertyExpression) {
+            // Should not emit get_by_id for indexed ones.
+            Optional<uint32_t> optionalIndex = parseIndex(target.propertyName);
+            if (!optionalIndex)
+                generator.emitGetById(temp.get(), rhs, target.propertyName);
+            else {
+                RefPtr<RegisterID> index = generator.emitLoad(generator.newTemporary(), jsNumber(optionalIndex.value()));
+                generator.emitGetByVal(temp.get(), rhs, index.get());
+            }
+        } else {
             RefPtr<RegisterID> propertyName = generator.emitNode(target.propertyExpression);
             generator.emitGetByVal(temp.get(), rhs, propertyName.get());
         }
diff --git a/Source/JavaScriptCore/tests/stress/element-property-get-should-not-handled-with-get-by-id.js b/Source/JavaScriptCore/tests/stress/element-property-get-should-not-handled-with-get-by-id.js
new file mode 100644 (file)
index 0000000..da2ad72
--- /dev/null
@@ -0,0 +1,18 @@
+(function () {
+    function getOne(a)
+    {
+        return a['1'];
+    }
+
+    for (var i = 0; i < 36; ++i)
+        getOne({2: true});
+
+    if (!getOne({1: true}))
+        throw new Error("OUT");
+
+    for (var i = 0; i < 1e4; ++i)
+        getOne({2: true});
+
+    if (!getOne({1: true}))
+        throw new Error("OUT");
+}());