Changeset 194021 in webkit


Ignore:
Timestamp:
Dec 13, 2015 6:52:51 PM (8 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):

Location:
trunk/Source/JavaScriptCore
Files:
1 added
3 edited

Legend:

Unmodified
Added
Removed
  • trunk/Source/JavaScriptCore/ChangeLog

    r194017 r194021  
     12015-12-13  Yusuke Suzuki  <utatane.tea@gmail.com>
     2
     3        [JSC] Should not emit get_by_id for indexed property access
     4        https://bugs.webkit.org/show_bug.cgi?id=151354
     5
     6        Reviewed by Darin Adler.
     7
     8        Before this patch, `a["1"]` is converted to `a.1` get_by_id operation in the bytecode compiler.
     9        get_by_id emits IC. IC rely on the fact that Structure transition occur when adding / removing object's properties.
     10        However, it's not true for indexed element properties. They are stored in the element storage and Structure transition does not occur.
     11
     12        For example, in the following case,
     13
     14             function getOne(a) { return a['1']; }
     15
     16             for (var i = 0; i < 36; ++i)
     17                 getOne({2: true});
     18
     19             if (!getOne({1: true}))
     20                 throw new Error("OUT");
     21
     22        In this case, `a['1']` creates get_by_id. `getOne({2: true})` calls makes getOne's get_by_id to create IC says that,
     23        "when comming this structure chain, there is no property in "1", so we should return `undefined`".
     24
     25        After that, we call `getOne({1: true})`. But in this case, `{2: true}` and `{1: true}` have the same structure chain,
     26        because indexed property addition does not occur structure transition.
     27        So previous IC fast path is used and return `undefined`. But the correct answer is returning `true`.
     28
     29        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.
     30        There are bugs in get_by_id, put_by_id, put_by_id (direct). But only get_by_id poses user observable issue.
     31        Because in the put_by_id case, the generic path just says "this put is uncacheable".
     32
     33        * bytecompiler/BytecodeGenerator.cpp:
     34        (JSC::BytecodeGenerator::emitGetById):
     35        (JSC::BytecodeGenerator::emitPutById):
     36        (JSC::BytecodeGenerator::emitDirectPutById):
     37        * bytecompiler/NodesCodegen.cpp:
     38        (JSC::isNonIndexStringElement):
     39        (JSC::BracketAccessorNode::emitBytecode):
     40        (JSC::FunctionCallBracketNode::emitBytecode):
     41        (JSC::AssignBracketNode::emitBytecode):
     42        (JSC::ObjectPatternNode::bindValue):
     43        * tests/stress/element-property-get-should-not-handled-with-get-by-id.js: Added.
     44        (getOne):
     45
    1462015-12-13  Andreas Kling  <akling@apple.com>
    247
  • trunk/Source/JavaScriptCore/bytecompiler/BytecodeGenerator.cpp

    r193974 r194021  
    22852285RegisterID* BytecodeGenerator::emitGetById(RegisterID* dst, RegisterID* base, const Identifier& property)
    22862286{
     2287    ASSERT_WITH_MESSAGE(!parseIndex(property), "Indexed properties should be handled with get_by_val.");
     2288
    22872289    m_codeBlock->addPropertyAccessInstruction(instructions().size());
    22882290
     
    23012303RegisterID* BytecodeGenerator::emitPutById(RegisterID* base, const Identifier& property, RegisterID* value)
    23022304{
     2305    ASSERT_WITH_MESSAGE(!parseIndex(property), "Indexed properties should be handled with put_by_val.");
     2306
    23032307    unsigned propertyIndex = addConstant(property);
    23042308
     
    23222326RegisterID* BytecodeGenerator::emitDirectPutById(RegisterID* base, const Identifier& property, RegisterID* value, PropertyNode::PutType putType)
    23232327{
    2324     ASSERT(!parseIndex(property));
     2328    ASSERT_WITH_MESSAGE(!parseIndex(property), "Indexed properties should be handled with put_by_val(direct).");
     2329
    23252330    unsigned propertyIndex = addConstant(property);
    23262331
  • trunk/Source/JavaScriptCore/bytecompiler/NodesCodegen.cpp

    r193974 r194021  
    603603// ------------------------------ BracketAccessorNode --------------------------------
    604604
     605static bool isNonIndexStringElement(ExpressionNode& element)
     606{
     607    return element.isString() && !parseIndex(static_cast<StringNode&>(element).value());
     608}
     609
    605610RegisterID* BracketAccessorNode::emitBytecode(BytecodeGenerator& generator, RegisterID* dst)
    606611{
    607612    if (m_base->isSuperNode()) {
    608613        // FIXME: Should we generate the profiler info?
    609         if (m_subscript->isString()) {
     614        if (isNonIndexStringElement(*m_subscript)) {
    610615            const Identifier& id = static_cast<StringNode*>(m_subscript)->value();
    611616            return generator.emitGetById(generator.finalDestination(dst), emitSuperBaseForCallee(generator), id);
     
    617622    RegisterID* finalDest = generator.finalDestination(dst);
    618623
    619     if (m_subscript->isString()) {
     624    if (isNonIndexStringElement(*m_subscript)) {
    620625        RefPtr<RegisterID> base = generator.emitNode(m_base);
    621626        ret = generator.emitGetById(finalDest, base.get(), static_cast<StringNode*>(m_subscript)->value());
     
    847852{
    848853    bool baseIsSuper = m_base->isSuperNode();
    849     bool subscriptIsString = m_subscript->isString();
     854    bool subscriptIsNonIndexString = isNonIndexStringElement(*m_subscript);
    850855
    851856    RefPtr<RegisterID> base;
     
    853858        base = emitSuperBaseForCallee(generator);
    854859    else {
    855         if (subscriptIsString)
     860        if (subscriptIsNonIndexString)
    856861            base = generator.emitNode(m_base);
    857862        else
     
    860865
    861866    RefPtr<RegisterID> function;
    862     if (subscriptIsString) {
     867    if (subscriptIsNonIndexString) {
    863868        generator.emitExpressionInfo(subexpressionDivot(), subexpressionStart(), subexpressionEnd());
    864869        function = generator.emitGetById(generator.tempDestination(dst), base.get(), static_cast<StringNode*>(m_subscript)->value());
     
    19781983    RegisterID* forwardResult = (dst == generator.ignoredResult()) ? result.get() : generator.moveToDestinationIfNeeded(generator.tempDestination(result.get()), result.get());
    19791984
    1980     if (m_subscript->isString())
     1985    if (isNonIndexStringElement(*m_subscript))
    19811986        generator.emitPutById(base.get(), static_cast<StringNode*>(m_subscript)->value(), forwardResult);
    19821987    else
     
    34863491        auto& target = m_targetPatterns[i];
    34873492        RefPtr<RegisterID> temp = generator.newTemporary();
    3488         if (!target.propertyExpression)
    3489             generator.emitGetById(temp.get(), rhs, target.propertyName);
    3490         else {
     3493        if (!target.propertyExpression) {
     3494            // Should not emit get_by_id for indexed ones.
     3495            Optional<uint32_t> optionalIndex = parseIndex(target.propertyName);
     3496            if (!optionalIndex)
     3497                generator.emitGetById(temp.get(), rhs, target.propertyName);
     3498            else {
     3499                RefPtr<RegisterID> index = generator.emitLoad(generator.newTemporary(), jsNumber(optionalIndex.value()));
     3500                generator.emitGetByVal(temp.get(), rhs, index.get());
     3501            }
     3502        } else {
    34913503            RefPtr<RegisterID> propertyName = generator.emitNode(target.propertyExpression);
    34923504            generator.emitGetByVal(temp.get(), rhs, propertyName.get());
Note: See TracChangeset for help on using the changeset viewer.