Changeset 194021 in webkit for trunk/Source/JavaScriptCore/bytecompiler/NodesCodegen.cpp

Timestamp:

Dec 13, 2015, 6:52:51 PM (10 years ago)

Author:

Yusuke Suzuki

Message:

[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 a1; }

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):

File:

• trunk/Source/JavaScriptCore/bytecompiler/NodesCodegen.cpp (modified) (7 diffs)

trunk/Source/JavaScriptCore/bytecompiler/NodesCodegen.cpp

@@ -603 +603 @@
 // ------------------------------ 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);
@@ -617 +622 @@
     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());
@@ -847 +852 @@
 {
     bool baseIsSuper = m_base->isSuperNode();
-    bool subscriptIsString = m_subscript->isString();
+    bool subscriptIsNonIndexString = isNonIndexStringElement(*m_subscript);
 
     RefPtr<RegisterID> base;
@@ -853 +858 @@
         base = emitSuperBaseForCallee(generator);
     else {
-        if (subscriptIsString)
+        if (subscriptIsNonIndexString)
             base = generator.emitNode(m_base);
         else
@@ -860 +865 @@
 
     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());
@@ -1978 +1983 @@
     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
@@ -3486 +3491 @@
         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());