Ignore:
Timestamp:
Feb 19, 2021, 1:42:20 AM (4 years ago)
Author:
[email protected]
Message:

[JSC] Simplify excludedSet handling in object rest expression
​https://bugs.webkit.org/show_bug.cgi?id=221668

Reviewed by Alexey Shvayka.

JSTests:

  • microbenchmarks/object-rest-computed-destructuring.js: Added.
  • stress/object-rest-computed-inlined-numbers.js: Added.

(inner):
(outer):

  • stress/object-rest-computed-inlined.js: Added.

(inner):
(outer):

Source/JavaScriptCore:

This patch simplifies Object rest/spread by avoiding use of JSSet.
Instead, we store IdentifierSet into UnlinkedCodeBlock's vector and
access this directly through JS constant number.
And for additional properties, we collect it during destructuring,
and pass it to @copyDataProperties as an argument so that we do not
need to call Set#add in JS side.

We also check (isNumber()
isString) conditions for properties to

avoid calling ToPropertyKey for constant case. This is OK since we
will call ToPropertyKey inside @copyDataProperties, and this is
side-effect free if the input is number or string.

This patch shows 2x improvement in microbenchmarking.

ToT Patched

object-rest-computed-destructuring 62.9960+-6.5072 30.2157+-1.1420 definitely 2.0849x faster

  • bytecode/CodeBlock.cpp:

(JSC::CodeBlock::finishCreation):
(JSC::CodeBlock::setConstantIdentifierSetRegisters): Deleted.

  • bytecode/CodeBlock.h:
  • bytecode/UnlinkedCodeBlock.h:

(JSC::UnlinkedCodeBlock::constantIdentifierSets):

  • bytecode/UnlinkedCodeBlockGenerator.h:

(JSC::UnlinkedCodeBlockGenerator::constantIdentifierSets):
(JSC::UnlinkedCodeBlockGenerator::addSetConstant):

  • bytecompiler/BytecodeGenerator.cpp:

(JSC::BytecodeGenerator::emitLoad):

  • bytecompiler/BytecodeGenerator.h:
  • bytecompiler/NodesCodegen.cpp:

(JSC::ObjectPatternNode::bindValue const):
(JSC::ObjectSpreadExpressionNode::emitBytecode):

  • runtime/CachedTypes.cpp:

(JSC::CachedConstantIdentifierSetEntry::encode): Deleted.
(JSC::CachedConstantIdentifierSetEntry::decode const): Deleted.

  • runtime/JSGlobalObjectFunctions.cpp:

(JSC::getCallerCodeBlock):
(JSC::JSC_DEFINE_HOST_FUNCTION):

Location:
trunk/Source/JavaScriptCore/bytecompiler
Files:
3 edited

Legend:

Unmodified
Added
Removed

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

    r273107 r273135  
    18411841}
    18421842
    1843 RegisterID* BytecodeGenerator::emitLoad(RegisterID* dst, IdentifierSet& set)
    1844 {
    1845     if (m_codeBlock->numberOfConstantIdentifierSets()) {
    1846         for (const auto& entry : m_codeBlock->constantIdentifierSets()) {
    1847             if (entry.first != set)
    1848                 continue;
    1849            
    1850             return &m_constantPoolRegisters[entry.second];
    1851         }
    1852     }
    1853    
    1854     unsigned index = addConstantIndex();
    1855     m_codeBlock->addSetConstant(set);
    1856     RegisterID* m_setRegister = &m_constantPoolRegisters[index];
    1857    
    1858     if (dst)
    1859         return move(dst, m_setRegister);
    1860    
    1861     return m_setRegister;
     1843RegisterID* BytecodeGenerator::emitLoad(RegisterID* dst, IdentifierSet&& set)
     1844{
     1845    unsigned setIndex = m_codeBlock->addSetConstant(WTFMove(set));
     1846    return emitLoad(dst, jsNumber(setIndex));
    18621847}
    18631848

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

    r273107 r273135  
    706706        RegisterID* emitLoad(RegisterID* dst, const Identifier&);
    707707        RegisterID* emitLoad(RegisterID* dst, JSValue, SourceCodeRepresentation = SourceCodeRepresentation::Other);
    708         RegisterID* emitLoad(RegisterID* dst, IdentifierSet& excludedList);
     708        RegisterID* emitLoad(RegisterID* dst, IdentifierSet&& excludedList);
    709709
    710710        template<typename UnaryOp, typename = std::enable_if_t<UnaryOp::opcodeID != op_negate>>

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

    r273107 r273135  
    55095509    generator.emitRequireObjectCoercible(rhs, "Right side of assignment cannot be destructured"_s);
    55105510
    5511     RefPtr<RegisterID> excludedList;
    5512     IdentifierSet excludedSet;
    5513     RefPtr<RegisterID> addMethod;
    5514     if (m_containsRestElement && m_containsComputedProperty) {
    5515         RefPtr<RegisterID> setConstructor = generator.moveLinkTimeConstant(nullptr, LinkTimeConstant::Set);
    5516 
    5517         CallArguments args(generator, nullptr, 0);
    5518         excludedList = generator.emitConstruct(generator.newTemporary(), setConstructor.get(), setConstructor.get(), NoExpectedFunction, args, divot(), divotStart(), divotEnd());
    5519 
    5520         addMethod = generator.emitGetById(generator.newTemporary(), excludedList.get(), generator.propertyNames().builtinNames().addPrivateName());
    5521     }
    5522 
    55235511    BytecodeGenerator::PreservedTDZStack preservedTDZStack;
    55245512    generator.preserveTDZStack(preservedTDZStack);
    55255513
    5526     for (size_t i = 0; i < m_targetPatterns.size(); i++) {
    5527         const auto& target = m_targetPatterns[i];
    5528         if (target.bindingType == BindingType::Element) {
    5529             // If the destructuring becomes get_by_id and mov, then we should store results directly to the local's binding.
    5530             // From
    5531             //     get_by_id          dst:loc10, base:loc9, property:0
    5532             //     mov                dst:loc6, src:loc10
    5533             // To
    5534             //     get_by_id          dst:loc6, base:loc9, property:0
    5535             auto writableDirectBindingIfPossible = [&]() -> RegisterID* {
    5536                 // The following pattern is possible. In that case, after setting |data| local variable, we need to store property name into the set.
    5537                 // So, old property name |data| result must be kept before setting it into |data|.
    5538                 //     ({ [data]: data, ...obj } = object);
    5539                 if (m_containsRestElement && m_containsComputedProperty && target.propertyExpression)
    5540                     return nullptr;
    5541                 // default value can include a reference to local variable. So filling value to a local variable can differ result.
    5542                 // We give up fast path if default value includes non constant.
    5543                 // For example,
    5544                 //     ({ data = data } = object);
    5545                 if (target.defaultValue && !target.defaultValue->isConstant())
    5546                     return nullptr;
    5547                 return target.pattern->writableDirectBindingIfPossible(generator);
    5548             };
    5549 
    5550             auto finishDirectBindingAssignment = [&]() {
    5551                 ASSERT(writableDirectBindingIfPossible());
    5552                 target.pattern->finishDirectBindingAssignment(generator);
    5553             };
    5554 
    5555             RefPtr<RegisterID> temp;
    5556             RegisterID* directBinding = writableDirectBindingIfPossible();
    5557             if (directBinding)
    5558                 temp = directBinding;
    5559             else
    5560                 temp = generator.newTemporary();
    5561 
    5562             RefPtr<RegisterID> propertyName;
    5563             if (!target.propertyExpression) {
    5564                 Optional<uint32_t> optionalIndex = parseIndex(target.propertyName);
    5565                 if (!optionalIndex)
    5566                     generator.emitGetById(temp.get(), rhs, target.propertyName);
    5567                 else {
    5568                     RefPtr<RegisterID> propertyIndex = generator.emitLoad(nullptr, jsNumber(optionalIndex.value()));
    5569                     generator.emitGetByVal(temp.get(), rhs, propertyIndex.get());
     5514    {
     5515        RefPtr<RegisterID> newObject;
     5516        IdentifierSet excludedSet;
     5517        Optional<CallArguments> args;
     5518        unsigned numberOfComputedProperties = 0;
     5519        unsigned indexInArguments = 2;
     5520        if (m_containsRestElement) {
     5521            if (m_containsComputedProperty) {
     5522                for (const auto& target : m_targetPatterns) {
     5523                    if (target.bindingType == BindingType::Element) {
     5524                        if (target.propertyExpression)
     5525                            ++numberOfComputedProperties;
     5526                    }
    55705527                }
     5528            }
     5529            newObject = generator.newTemporary();
     5530            args.emplace(generator, nullptr, indexInArguments + numberOfComputedProperties);
     5531        }
     5532
     5533        for (size_t i = 0; i < m_targetPatterns.size(); i++) {
     5534            const auto& target = m_targetPatterns[i];
     5535            if (target.bindingType == BindingType::Element) {
     5536                // If the destructuring becomes get_by_id and mov, then we should store results directly to the local's binding.
     5537                // From
     5538                //     get_by_id          dst:loc10, base:loc9, property:0
     5539                //     mov                dst:loc6, src:loc10
     5540                // To
     5541                //     get_by_id          dst:loc6, base:loc9, property:0
     5542                auto writableDirectBindingIfPossible = [&]() -> RegisterID* {
     5543                    // The following pattern is possible. In that case, after setting |data| local variable, we need to store property name into the set.
     5544                    // So, old property name |data| result must be kept before setting it into |data|.
     5545                    //     ({ [data]: data, ...obj } = object);
     5546                    if (m_containsRestElement && m_containsComputedProperty && target.propertyExpression)
     5547                        return nullptr;
     5548                    // default value can include a reference to local variable. So filling value to a local variable can differ result.
     5549                    // We give up fast path if default value includes non constant.
     5550                    // For example,
     5551                    //     ({ data = data } = object);
     5552                    if (target.defaultValue && !target.defaultValue->isConstant())
     5553                        return nullptr;
     5554                    return target.pattern->writableDirectBindingIfPossible(generator);
     5555                };
     5556
     5557                auto finishDirectBindingAssignment = [&]() {
     5558                    ASSERT(writableDirectBindingIfPossible());
     5559                    target.pattern->finishDirectBindingAssignment(generator);
     5560                };
     5561
     5562                RefPtr<RegisterID> temp;
     5563                RegisterID* directBinding = writableDirectBindingIfPossible();
     5564                if (directBinding)
     5565                    temp = directBinding;
     5566                else
     5567                    temp = generator.newTemporary();
     5568
     5569                if (!target.propertyExpression) {
     5570                    Optional<uint32_t> optionalIndex = parseIndex(target.propertyName);
     5571                    if (!optionalIndex)
     5572                        generator.emitGetById(temp.get(), rhs, target.propertyName);
     5573                    else {
     5574                        RefPtr<RegisterID> propertyIndex = generator.emitLoad(nullptr, jsNumber(optionalIndex.value()));
     5575                        generator.emitGetByVal(temp.get(), rhs, propertyIndex.get());
     5576                    }
     5577                    if (m_containsRestElement)
     5578                        excludedSet.add(target.propertyName.impl());
     5579                } else {
     5580                    RefPtr<RegisterID> propertyName;
     5581                    if (m_containsRestElement) {
     5582                        propertyName = generator.emitNodeForProperty(args->argumentRegister(indexInArguments++), target.propertyExpression);
     5583                        // ToPropertyKey(Number | String) does not have side-effect.
     5584                        // And @copyDataProperties performs ToPropertyKey internally.
     5585                        // And for Number case, passing it to GetByVal is better for performance.
     5586                        if (!target.propertyExpression->isNumber() || !target.propertyExpression->isString())
     5587                            propertyName = generator.emitToPropertyKey(propertyName.get(), propertyName.get());
     5588                    } else
     5589                        propertyName = generator.emitNodeForProperty(target.propertyExpression);
     5590                    generator.emitGetByVal(temp.get(), rhs, propertyName.get());
     5591                }
     5592
     5593                if (target.defaultValue)
     5594                    assignDefaultValueIfUndefined(generator, temp.get(), target.defaultValue);
     5595                if (directBinding)
     5596                    finishDirectBindingAssignment();
     5597                else
     5598                    target.pattern->bindValue(generator, temp.get());
    55715599            } else {
    5572                 propertyName = generator.emitNodeForProperty(target.propertyExpression);
    5573                 generator.emitGetByVal(temp.get(), rhs, propertyName.get());
     5600                ASSERT(target.bindingType == BindingType::RestElement);
     5601                ASSERT(i == m_targetPatterns.size() - 1);
     5602
     5603                generator.emitNewObject(newObject.get());
     5604
     5605                // load and call @copyDataProperties
     5606                RefPtr<RegisterID> copyDataProperties = generator.moveLinkTimeConstant(nullptr, LinkTimeConstant::copyDataProperties);
     5607
     5608                // This must be non-tail-call because @copyDataProperties accesses caller-frame.
     5609                generator.move(args->thisRegister(), newObject.get());
     5610                generator.move(args->argumentRegister(0), rhs);
     5611                generator.emitLoad(args->argumentRegister(1), WTFMove(excludedSet));
     5612                generator.emitCall(generator.newTemporary(), copyDataProperties.get(), NoExpectedFunction, args.value(), divot(), divotStart(), divotEnd(), DebuggableCall::No);
     5613                target.pattern->bindValue(generator, newObject.get());
    55745614            }
    5575 
    5576             if (m_containsRestElement) {
    5577                 if (m_containsComputedProperty) {
    5578                     if (target.propertyExpression)
    5579                         propertyName = generator.emitToPropertyKey(generator.tempDestination(propertyName.get()), propertyName.get());
    5580                     else
    5581                         propertyName = generator.emitLoad(nullptr, target.propertyName);
    5582 
    5583                     CallArguments args(generator, nullptr, 1);
    5584                     generator.move(args.thisRegister(), excludedList.get());
    5585                     generator.move(args.argumentRegister(0), propertyName.get());
    5586                     generator.emitCall(generator.newTemporary(), addMethod.get(), NoExpectedFunction, args, divot(), divotStart(), divotEnd(), DebuggableCall::No);
    5587                 } else
    5588                     excludedSet.add(target.propertyName.impl());
    5589             }
    5590 
    5591             if (target.defaultValue)
    5592                 assignDefaultValueIfUndefined(generator, temp.get(), target.defaultValue);
    5593             if (directBinding)
    5594                 finishDirectBindingAssignment();
    5595             else
    5596                 target.pattern->bindValue(generator, temp.get());
    5597         } else {
    5598             ASSERT(target.bindingType == BindingType::RestElement);
    5599             ASSERT(i == m_targetPatterns.size() - 1);
    5600             RefPtr<RegisterID> newObject = generator.emitNewObject(generator.newTemporary());
    5601            
    5602             // load and call @copyDataProperties
    5603             RefPtr<RegisterID> copyDataProperties = generator.moveLinkTimeConstant(nullptr, LinkTimeConstant::copyDataProperties);
    5604            
    5605             CallArguments args(generator, nullptr, 3);
    5606             generator.emitLoad(args.thisRegister(), jsUndefined());
    5607             generator.move(args.argumentRegister(0), newObject.get());
    5608             generator.move(args.argumentRegister(1), rhs);
    5609             if (m_containsComputedProperty)
    5610                 generator.move(args.argumentRegister(2), excludedList.get());
    5611             else {
    5612                 RefPtr<RegisterID> excludedSetReg = generator.emitLoad(generator.newTemporary(), excludedSet);
    5613                 generator.move(args.argumentRegister(2), excludedSetReg.get());
    5614             }
    5615 
    5616             generator.emitCall(generator.newTemporary(), copyDataProperties.get(), NoExpectedFunction, args, divot(), divotStart(), divotEnd(), DebuggableCall::No);
    5617             target.pattern->bindValue(generator, newObject.get());
    56185615        }
    56195616    }
    … …  
    58265823    RefPtr<RegisterID> copyDataProperties = generator.moveLinkTimeConstant(nullptr, LinkTimeConstant::copyDataProperties);
    58275824   
    5828     CallArguments args(generator, nullptr, 2);
    5829     generator.emitLoad(args.thisRegister(), jsUndefined());
    5830     generator.move(args.argumentRegister(0), dst);
    5831     generator.move(args.argumentRegister(1), src.get());
     5825    CallArguments args(generator, nullptr, 1);
     5826    generator.move(args.thisRegister(), dst);
     5827    generator.move(args.argumentRegister(0), src.get());
    58325828   
     5829    // This must be non-tail-call because @copyDataProperties accesses caller-frame.
    58335830    generator.emitCall(generator.newTemporary(), copyDataProperties.get(), NoExpectedFunction, args, divot(), divotStart(), divotEnd(), DebuggableCall::No);
    58345831   
Note: See TracChangeset for help on using the changeset viewer.