Changeset 236161 in webkit


Ignore:
Timestamp:
Sep 18, 2018 5:57:56 PM (6 years ago)
Author:
mark.lam@apple.com
Message:

Ensure that ForInContexts are invalidated if their loop local is over-written.
​https://bugs.webkit.org/show_bug.cgi?id=189571
<rdar://problem/44402277>

Reviewed by Saam Barati.

JSTests:

  • stress/regress-189571.js: Added.

Source/JavaScriptCore:

Instead of hunting down every place in the BytecodeGenerator that potentially
needs to invalidate an enclosing ForInContext (if one exists), we simply iterate
the bytecode range of the loop body when the ForInContext is popped, and
invalidate the context if we ever find the loop temp variable over-written.

This has 2 benefits:

  1. It ensures that every type of opcode that can write to the loop temp will be handled appropriately, not just the op_mov that we've hunted down.
  2. It avoids us having to check the BytecodeGenerator's m_forInContextStack every time we emit an op_mov (or other opcodes that can write to a local) even when we're not inside a for-in loop.

JSC benchmarks show that that this change is performance neutral.

  • bytecompiler/BytecodeGenerator.cpp:

(JSC::BytecodeGenerator::pushIndexedForInScope):
(JSC::BytecodeGenerator::popIndexedForInScope):
(JSC::BytecodeGenerator::pushStructureForInScope):
(JSC::BytecodeGenerator::popStructureForInScope):
(JSC::ForInContext::finalize):
(JSC::StructureForInContext::finalize):
(JSC::IndexedForInContext::finalize):
(JSC::BytecodeGenerator::invalidateForInContextForLocal): Deleted.

  • bytecompiler/BytecodeGenerator.h:

(JSC::ForInContext::ForInContext):
(JSC::ForInContext::bodyBytecodeStartOffset const):
(JSC::StructureForInContext::StructureForInContext):
(JSC::IndexedForInContext::IndexedForInContext):

  • bytecompiler/NodesCodegen.cpp:

(JSC::PostfixNode::emitResolve):
(JSC::PrefixNode::emitResolve):
(JSC::ReadModifyResolveNode::emitBytecode):
(JSC::AssignResolveNode::emitBytecode):
(JSC::EmptyLetExpression::emitBytecode):
(JSC::ForInNode::emitLoopHeader):
(JSC::ForOfNode::emitBytecode):
(JSC::BindingNode::bindValue const):
(JSC::AssignmentElementNode::bindValue const):

  • runtime/CommonSlowPaths.cpp:

(JSC::SLOW_PATH_DECL):

Location:
trunk
Files:
1 added
6 edited

Legend:

Unmodified
Added
Removed

  • trunk/JSTests/ChangeLog

    r236089 r236161  
     12018-09-17  Mark Lam  <mark.lam@apple.com>
     2
     3        Ensure that ForInContexts are invalidated if their loop local is over-written.
     4        https://bugs.webkit.org/show_bug.cgi?id=189571
     5        <rdar://problem/44402277>
     6
     7        Reviewed by Saam Barati.
     8
     9        * stress/regress-189571.js: Added.
     10
    1112018-09-17  Saam barati  <sbarati@apple.com>
    212

  • trunk/Source/JavaScriptCore/ChangeLog

    r236091 r236161  
     12018-09-18  Mark Lam  <mark.lam@apple.com>
     2
     3        Ensure that ForInContexts are invalidated if their loop local is over-written.
     4        https://bugs.webkit.org/show_bug.cgi?id=189571
     5        <rdar://problem/44402277>
     6
     7        Reviewed by Saam Barati.
     8
     9        Instead of hunting down every place in the BytecodeGenerator that potentially
     10        needs to invalidate an enclosing ForInContext (if one exists), we simply iterate
     11        the bytecode range of the loop body when the ForInContext is popped, and
     12        invalidate the context if we ever find the loop temp variable over-written.
     13
     14        This has 2 benefits:
     15        1. It ensures that every type of opcode that can write to the loop temp will be
     16           handled appropriately, not just the op_mov that we've hunted down.
     17        2. It avoids us having to check the BytecodeGenerator's m_forInContextStack
     18           every time we emit an op_mov (or other opcodes that can write to a local)
     19           even when we're not inside a for-in loop.
     20
     21        JSC benchmarks show that that this change is performance neutral.
     22
     23        * bytecompiler/BytecodeGenerator.cpp:
     24        (JSC::BytecodeGenerator::pushIndexedForInScope):
     25        (JSC::BytecodeGenerator::popIndexedForInScope):
     26        (JSC::BytecodeGenerator::pushStructureForInScope):
     27        (JSC::BytecodeGenerator::popStructureForInScope):
     28        (JSC::ForInContext::finalize):
     29        (JSC::StructureForInContext::finalize):
     30        (JSC::IndexedForInContext::finalize):
     31        (JSC::BytecodeGenerator::invalidateForInContextForLocal): Deleted.
     32        * bytecompiler/BytecodeGenerator.h:
     33        (JSC::ForInContext::ForInContext):
     34        (JSC::ForInContext::bodyBytecodeStartOffset const):
     35        (JSC::StructureForInContext::StructureForInContext):
     36        (JSC::IndexedForInContext::IndexedForInContext):
     37        * bytecompiler/NodesCodegen.cpp:
     38        (JSC::PostfixNode::emitResolve):
     39        (JSC::PrefixNode::emitResolve):
     40        (JSC::ReadModifyResolveNode::emitBytecode):
     41        (JSC::AssignResolveNode::emitBytecode):
     42        (JSC::EmptyLetExpression::emitBytecode):
     43        (JSC::ForInNode::emitLoopHeader):
     44        (JSC::ForOfNode::emitBytecode):
     45        (JSC::BindingNode::bindValue const):
     46        (JSC::AssignmentElementNode::bindValue const):
     47        * runtime/CommonSlowPaths.cpp:
     48        (JSC::SLOW_PATH_DECL):
     49
    1502018-09-17  Devin Rousso  <drousso@apple.com>
    251

  • trunk/Source/JavaScriptCore/bytecompiler/BytecodeGenerator.cpp

    r236018 r236161  
    3737#include "BytecodeGeneratorification.h"
    3838#include "BytecodeLivenessAnalysis.h"
     39#include "BytecodeUseDef.h"
    3940#include "CatchScope.h"
    4041#include "DefinePropertyAttributes.h"
    … …  
    46254626    if (!localRegister)
    46264627        return;
    4627     m_forInContextStack.append(adoptRef(*new IndexedForInContext(localRegister, indexRegister)));
     4628    unsigned bodyBytecodeStartOffset = instructions().size();
     4629    m_forInContextStack.append(adoptRef(*new IndexedForInContext(localRegister, indexRegister, bodyBytecodeStartOffset)));
    46284630}
    46294631
    … …  
    46324634    if (!localRegister)
    46334635        return;
    4634     m_forInContextStack.last()->asIndexedForInContext().finalize(*this);
     4636    unsigned bodyBytecodeEndOffset = instructions().size();
     4637    m_forInContextStack.last()->asIndexedForInContext().finalize(*this, m_codeBlock.get(), bodyBytecodeEndOffset);
    46354638    m_forInContextStack.removeLast();
    46364639}
    … …  
    47354738    if (!localRegister)
    47364739        return;
    4737     m_forInContextStack.append(adoptRef(*new StructureForInContext(localRegister, indexRegister, propertyRegister, enumeratorRegister)));
     4740    unsigned bodyBytecodeStartOffset = instructions().size();
     4741    m_forInContextStack.append(adoptRef(*new StructureForInContext(localRegister, indexRegister, propertyRegister, enumeratorRegister, bodyBytecodeStartOffset)));
    47384742}
    47394743
    … …  
    47424746    if (!localRegister)
    47434747        return;
    4744     m_forInContextStack.last()->asStructureForInContext().finalize(*this);
     4748    unsigned bodyBytecodeEndOffset = instructions().size();
     4749    m_forInContextStack.last()->asStructureForInContext().finalize(*this, m_codeBlock.get(), bodyBytecodeEndOffset);
    47454750    m_forInContextStack.removeLast();
    4746 }
    4747 
    4748 void BytecodeGenerator::invalidateForInContextForLocal(RegisterID* localRegister)
    4749 {
    4750     // Lexically invalidating ForInContexts is kind of weak sauce, but it only occurs if
    4751     // either of the following conditions is true:
    4752     //
    4753     // (1) The loop iteration variable is re-assigned within the body of the loop.
    4754     // (2) The loop iteration variable is captured in the lexical scope of the function.
    4755     //
    4756     // These two situations occur sufficiently rarely that it's okay to use this style of
    4757     // "analysis" to make iteration faster. If we didn't want to do this, we would either have
    4758     // to perform some flow-sensitive analysis to see if/when the loop iteration variable was
    4759     // reassigned, or we'd have to resort to runtime checks to see if the variable had been
    4760     // reassigned from its original value.
    4761     for (size_t i = m_forInContextStack.size(); i--; ) {
    4762         ForInContext& context = m_forInContextStack[i].get();
    4763         if (context.local() == localRegister)
    4764             context.invalidate();
    4765     }
    47664751}
    47674752
    … …  
    51915176}
    51925177
    5193 void StructureForInContext::finalize(BytecodeGenerator& generator)
    5194 {
     5178void ForInContext::finalize(BytecodeGenerator& generator, UnlinkedCodeBlock* codeBlock, unsigned bodyBytecodeEndOffset)
     5179{
     5180    // Lexically invalidating ForInContexts is kind of weak sauce, but it only occurs if
     5181    // either of the following conditions is true:
     5182    //
     5183    // (1) The loop iteration variable is re-assigned within the body of the loop.
     5184    // (2) The loop iteration variable is captured in the lexical scope of the function.
     5185    //
     5186    // These two situations occur sufficiently rarely that it's okay to use this style of
     5187    // "analysis" to make iteration faster. If we didn't want to do this, we would either have
     5188    // to perform some flow-sensitive analysis to see if/when the loop iteration variable was
     5189    // reassigned, or we'd have to resort to runtime checks to see if the variable had been
     5190    // reassigned from its original value.
     5191
     5192    for (unsigned offset = bodyBytecodeStartOffset(); isValid() && offset < bodyBytecodeEndOffset;) {
     5193        UnlinkedInstruction* instruction = &generator.instructions()[offset];
     5194        OpcodeID opcodeID = instruction->u.opcode;
     5195        unsigned opcodeLength = opcodeLengths[opcodeID];
     5196
     5197        ASSERT(opcodeID != op_enter);
     5198        computeDefsForBytecodeOffset(codeBlock, opcodeID, instruction, [&] (UnlinkedCodeBlock*, UnlinkedInstruction*, OpcodeID, int operand) {
     5199            if (local()->index() == operand)
     5200                invalidate();
     5201        });
     5202        offset += opcodeLength;
     5203    }
     5204}
     5205
     5206void StructureForInContext::finalize(BytecodeGenerator& generator, UnlinkedCodeBlock* codeBlock, unsigned bodyBytecodeEndOffset)
     5207{
     5208    Base::finalize(generator, codeBlock, bodyBytecodeEndOffset);
    51955209    if (isValid())
    51965210        return;
    … …  
    52205234}
    52215235
    5222 void IndexedForInContext::finalize(BytecodeGenerator& generator)
    5223 {
     5236void IndexedForInContext::finalize(BytecodeGenerator& generator, UnlinkedCodeBlock* codeBlock, unsigned bodyBytecodeEndOffset)
     5237{
     5238    Base::finalize(generator, codeBlock, bodyBytecodeEndOffset);
    52245239    if (isValid())
    52255240        return;

  • trunk/Source/JavaScriptCore/bytecompiler/BytecodeGenerator.h

    r236018 r236161  
    212212
    213213    protected:
    214         ForInContext(RegisterID* localRegister, Type type)
     214        ForInContext(RegisterID* localRegister, Type type, unsigned bodyBytecodeStartOffset)
    215215            : m_localRegister(localRegister)
    216216            , m_type(type)
     217            , m_bodyBytecodeStartOffset(bodyBytecodeStartOffset)
    217218        { }
     219
     220        unsigned bodyBytecodeStartOffset() const { return m_bodyBytecodeStartOffset; }
     221
     222        void finalize(BytecodeGenerator&, UnlinkedCodeBlock*, unsigned bodyBytecodeEndOffset);
    218223
    219224    private:
    … …  
    221226        bool m_isValid { true };
    222227        Type m_type;
     228        unsigned m_bodyBytecodeStartOffset;
    223229    };
    224230
    225231    class StructureForInContext : public ForInContext {
     232        using Base = ForInContext;
    226233    public:
    227234        using GetInst = std::tuple<unsigned, int, UnlinkedValueProfile>;
    228235
    229         StructureForInContext(RegisterID* localRegister, RegisterID* indexRegister, RegisterID* propertyRegister, RegisterID* enumeratorRegister)
    230             : ForInContext(localRegister, Type::StructureForIn)
     236        StructureForInContext(RegisterID* localRegister, RegisterID* indexRegister, RegisterID* propertyRegister, RegisterID* enumeratorRegister, unsigned bodyBytecodeStartOffset)
     237            : ForInContext(localRegister, Type::StructureForIn, bodyBytecodeStartOffset)
    231238            , m_indexRegister(indexRegister)
    232239            , m_propertyRegister(propertyRegister)
    … …  
    244251        }
    245252
    246         void finalize(BytecodeGenerator&);
     253        void finalize(BytecodeGenerator&, UnlinkedCodeBlock*, unsigned bodyBytecodeEndOffset);
    247254
    248255    private:
    … …  
    254261
    255262    class IndexedForInContext : public ForInContext {
     263        using Base = ForInContext;
    256264    public:
    257         IndexedForInContext(RegisterID* localRegister, RegisterID* indexRegister)
    258             : ForInContext(localRegister, Type::IndexedForIn)
     265        IndexedForInContext(RegisterID* localRegister, RegisterID* indexRegister, unsigned bodyBytecodeStartOffset)
     266            : ForInContext(localRegister, Type::IndexedForIn, bodyBytecodeStartOffset)
    259267            , m_indexRegister(indexRegister)
    260268        {
    … …  
    263271        RegisterID* index() const { return m_indexRegister.get(); }
    264272
    265         void finalize(BytecodeGenerator&);
     273        void finalize(BytecodeGenerator&, UnlinkedCodeBlock*, unsigned bodyBytecodeEndOffset);
    266274        void addGetInst(unsigned instIndex, int propertyIndex) { m_getInsts.append({ instIndex, propertyIndex }); }
    267275
    … …  
    939947        void pushStructureForInScope(RegisterID* local, RegisterID* index, RegisterID* property, RegisterID* enumerator);
    940948        void popStructureForInScope(RegisterID* local);
    941         void invalidateForInContextForLocal(RegisterID* local);
    942949
    943950        LabelScope* breakTarget(const Identifier&);

  • trunk/Source/JavaScriptCore/bytecompiler/NodesCodegen.cpp

    r234082 r236161  
    15571557            localReg = generator.move(generator.tempDestination(dst), local);
    15581558        }
    1559         generator.invalidateForInContextForLocal(local);
    15601559        RefPtr<RegisterID> oldValue = emitPostIncOrDec(generator, generator.finalDestination(dst), localReg.get(), m_operator);
    15611560        generator.emitProfileType(localReg.get(), var, divotStart(), divotEnd());
    … …  
    17701769            localReg = generator.move(generator.tempDestination(dst), localReg.get());
    17711770        } else if (generator.vm()->typeProfiler()) {
    1772             generator.invalidateForInContextForLocal(local);
    17731771            RefPtr<RegisterID> tempDst = generator.tempDestination(dst);
    17741772            generator.move(tempDst.get(), localReg.get());
    … …  
    17781776            return generator.move(dst, tempDst.get());
    17791777        }
    1780         generator.invalidateForInContextForLocal(local);
    17811778        emitIncOrDec(generator, localReg.get(), m_operator);
    17821779        return generator.move(dst, localReg.get());
    … …  
    24422439            emitReadModifyAssignment(generator, result.get(), result.get(), m_right, m_operator, OperandTypes(ResultType::unknownType(), m_right->resultDescriptor()));
    24432440            generator.move(local, result.get());
    2444             generator.invalidateForInContextForLocal(local);
    24452441            generator.emitProfileType(local, divotStart(), divotEnd());
    24462442            return generator.move(dst, result.get());
    … …  
    24482444       
    24492445        RegisterID* result = emitReadModifyAssignment(generator, local, local, m_right, m_operator, OperandTypes(ResultType::unknownType(), m_right->resultDescriptor()));
    2450         generator.invalidateForInContextForLocal(local);
    24512446        generator.emitProfileType(result, divotStart(), divotEnd());
    24522447        return generator.move(dst, result);
    … …  
    25062501            generator.move(local, tempDst.get());
    25072502            generator.emitProfileType(local, var, divotStart(), divotEnd());
    2508             generator.invalidateForInContextForLocal(local);
    25092503            result = generator.move(dst, tempDst.get());
    25102504        } else {
    25112505            RegisterID* right = generator.emitNode(local, m_right);
    25122506            generator.emitProfileType(right, var, divotStart(), divotEnd());
    2513             generator.invalidateForInContextForLocal(local);
    25142507            result = generator.move(dst, right);
    25152508        }
    … …  
    27532746    if (RegisterID* local = var.local()) {
    27542747        generator.emitLoad(local, jsUndefined());
    2755         generator.invalidateForInContextForLocal(local);
    27562748        generator.emitProfileType(local, var, position(), JSTextPosition(-1, position().offset + m_ident.length(), -1));
    27572749    } else {
    … …  
    29672959                generator.emitReadOnlyExceptionIfNeeded(var);
    29682960            generator.move(local, propertyName);
    2969             generator.invalidateForInContextForLocal(local);
    29702961        } else {
    29712962            if (generator.isStrictMode())
    … …  
    30353026        }
    30363027        generator.move(var.local(), propertyName);
    3037         generator.invalidateForInContextForLocal(var.local());
    30383028        generator.emitProfileType(propertyName, var, simpleBinding->divotStart(), simpleBinding->divotEnd());
    30393029        return;
    … …  
    32263216                    generator.emitReadOnlyExceptionIfNeeded(var);
    32273217                generator.move(local, value);
    3228                 generator.invalidateForInContextForLocal(local);
    32293218            } else {
    32303219                if (generator.isStrictMode())
    … …  
    44004389        }
    44014390        generator.move(local, value);
    4402         generator.invalidateForInContextForLocal(local);
    44034391        generator.emitProfileType(local, var, divotStart(), divotEnd());
    44044392        if (m_bindingContext == AssignmentContext::DeclarationStatement || m_bindingContext == AssignmentContext::ConstDeclarationStatement)
    … …  
    44494437                generator.emitReadOnlyExceptionIfNeeded(var);
    44504438            else {
    4451                 generator.invalidateForInContextForLocal(local);
    44524439                generator.move(local, value);
    44534440                generator.emitProfileType(local, divotStart(), divotEnd());

  • trunk/Source/JavaScriptCore/runtime/CommonSlowPaths.cpp

    r235827 r236161  
    811811    CHECK_EXCEPTION();
    812812    JSValue property = OP(3).jsValue();
     813    ASSERT(property.isString());
    813814    JSString* string = asString(property);
    814815    auto propertyName = string->toIdentifier(exec);
    … …  
    822823    JSValue baseValue = OP_C(2).jsValue();
    823824    JSValue property = OP(3).jsValue();
     825    ASSERT(property.isString());
    824826    JSString* string = asString(property);
    825827    auto propertyName = string->toIdentifier(exec);
Note: See TracChangeset for help on using the changeset viewer.