Changeset 41232 in webkit for trunk/JavaScriptCore/runtime


Ignore:
Timestamp:
Feb 25, 2009, 3:44:07 PM (16 years ago)
Author:
[email protected]
Message:

JavaScriptCore:

2009-02-25 Geoffrey Garen <​[email protected]>

Reviewed by Maciej Stachowiak.


Fixed <rdar://problem/6611174> REGRESSION (r36701): Unable to select
messages on hotmail (24052)


The bug was that for-in enumeration used a cached prototype chain without
validating that it was up-to-date.


This led me to refactor prototype chain caching so it was easier to work
with and harder to get wrong.


After a bit of inlining, this patch is performance-neutral on SunSpider
and the v8 benchmarks.

  • interpreter/Interpreter.cpp: (JSC::Interpreter::tryCachePutByID): (JSC::Interpreter::tryCacheGetByID):
  • jit/JITStubs.cpp: (JSC::JITStubs::tryCachePutByID): (JSC::JITStubs::tryCacheGetByID): (JSC::JITStubs::cti_op_get_by_id_proto_list): Use the new refactored goodness. See lines beginning with "-" and smile.
  • runtime/JSGlobalObject.h: (JSC::Structure::prototypeForLookup): A shout out to const.
  • runtime/JSPropertyNameIterator.h: (JSC::JSPropertyNameIterator::next): We can use a pointer comparison to see if our cached structure chain is equal to the object's structure chain, since in the case of a cache hit, we share references to the same structure chain.
  • runtime/Operations.h: (JSC::countPrototypeChainEntriesAndCheckForProxies): Use the new refactored goodness.
  • runtime/PropertyNameArray.h: (JSC::PropertyNameArray::PropertyNameArray): (JSC::PropertyNameArray::setShouldCache): (JSC::PropertyNameArray::shouldCache): Renamed "cacheable" to "shouldCache" to communicate that the client is specifying a recommendation, not a capability.


  • runtime/Structure.cpp: (JSC::Structure::Structure): No need to initialize a RefPtr. (JSC::Structure::getEnumerablePropertyNames): Moved some code into helper functions.

(JSC::Structure::prototypeChain): New centralized accessor for a prototype
chain. Revalidates on every access, since the objects in the prototype
chain may have mutated.

(JSC::Structure::isValid): Helper function for revalidating a cached
prototype chain.

(JSC::Structure::getEnumerableNamesFromPropertyTable):
(JSC::Structure::getEnumerableNamesFromClassInfoTable): Factored out of
getEnumerablePropertyNames.

  • runtime/Structure.h:
  • runtime/StructureChain.cpp: (JSC::StructureChain::StructureChain):
  • runtime/StructureChain.h: (JSC::StructureChain::create): No need for structureChainsAreEqual, since we use pointer equality now. Refactored StructureChain to make a little more sense and eliminate special cases for null prototypes.

LayoutTests:

2009-02-24 Geoffrey Garen <​[email protected]>

Reviewed by Maciej Stachowiak.


Added a test for <rdar://problem/6611174> REGRESSION (r36701): Unable to
select messages on hotmail (24052)

  • fast/js/for-in-cached-expected.txt: Added.
  • fast/js/for-in-cached.html: Added.
  • fast/js/resources/for-in-cached.js: Added. (forIn):
Location:
trunk/JavaScriptCore/runtime
Files:
8 edited

Legend:

Unmodified
Added
Removed

  • trunk/JavaScriptCore/runtime/JSGlobalObject.h

    r41126 r41232  
    330330    }
    331331
    332     inline JSValuePtr Structure::prototypeForLookup(ExecState* exec)
     332    inline JSValuePtr Structure::prototypeForLookup(ExecState* exec) const
    333333    {
    334334        if (typeInfo().type() == ObjectType)
    … …  
    340340        ASSERT(typeInfo().type() == NumberType);
    341341        return exec->lexicalGlobalObject()->numberPrototype();
     342    }
     343
     344    inline StructureChain* Structure::prototypeChain(ExecState* exec) const
     345    {
     346        // We cache our prototype chain so our clients can share it.
     347        if (!isValid(exec, m_cachedPrototypeChain.get())) {
     348            JSValuePtr prototype = prototypeForLookup(exec);
     349            m_cachedPrototypeChain = StructureChain::create(prototype.isNull() ? 0 : asObject(prototype)->structure());
     350        }
     351        return m_cachedPrototypeChain.get();
     352    }
     353
     354    inline bool Structure::isValid(ExecState* exec, StructureChain* cachedPrototypeChain) const
     355    {
     356        if (!cachedPrototypeChain)
     357            return false;
     358
     359        JSValuePtr prototype = prototypeForLookup(exec);
     360        RefPtr<Structure>* cachedStructure = cachedPrototypeChain->head();
     361        while(*cachedStructure && !prototype.isNull()) {
     362            if (asObject(prototype)->structure() != *cachedStructure)
     363                return false;
     364            ++cachedStructure;
     365            prototype = asObject(prototype)->prototype();
     366        }
     367        return prototype.isNull() && !*cachedStructure;
    342368    }
    343369

  • trunk/JavaScriptCore/runtime/JSPropertyNameIterator.h

    r40046 r41232  
    100100        return noValue();
    101101
    102     if (m_data->cachedStructure() == m_object->structure() && structureChainsAreEqual(m_data->cachedPrototypeChain(), m_object->structure()->cachedPrototypeChain()))
     102    if (m_data->cachedStructure() == m_object->structure() && m_data->cachedPrototypeChain() == m_object->structure()->prototypeChain(exec))
    103103        return jsOwnedString(exec, (*m_position++).ustring());
    104104

  • trunk/JavaScriptCore/runtime/Operations.h

    r41168 r41232  
    227227    }
    228228
    229     inline StructureChain* cachePrototypeChain(CallFrame* callFrame, Structure* structure)
    230     {
    231         JSValuePtr prototype = structure->prototypeForLookup(callFrame);
    232         if (!prototype.isCell())
    233             return 0;
    234         RefPtr<StructureChain> chain = StructureChain::create(asObject(prototype)->structure());
    235         structure->setCachedPrototypeChain(chain.release());
    236         return structure->cachedPrototypeChain();
    237     }
    238 
    239229    inline size_t countPrototypeChainEntriesAndCheckForProxies(CallFrame* callFrame, JSValuePtr baseValue, const PropertySlot& slot)
    240230    {
    … …  
    255245            // Since we're accessing a prototype in a loop, it's a good bet that it
    256246            // should not be treated as a dictionary.
    257             if (cell->structure()->isDictionary()) {
    258                 RefPtr<Structure> transition = Structure::fromDictionaryTransition(cell->structure());
    259                 asObject(cell)->setStructure(transition.release());
    260                 cell->structure()->setCachedPrototypeChain(0);
    261             }
     247            if (cell->structure()->isDictionary())
     248                asObject(cell)->setStructure(Structure::fromDictionaryTransition(cell->structure()));
    262249
    263250            ++count;

  • trunk/JavaScriptCore/runtime/PropertyNameArray.h

    r38528 r41232  
    6666            : m_data(PropertyNameArrayData::create())
    6767            , m_globalData(globalData)
    68             , m_cacheable(true)
     68            , m_shouldCache(true)
    6969        {
    7070        }
    … …  
    7373            : m_data(PropertyNameArrayData::create())
    7474            , m_globalData(&exec->globalData())
    75             , m_cacheable(true)
     75            , m_shouldCache(true)
    7676        {
    7777        }
    … …  
    9696        PassRefPtr<PropertyNameArrayData> releaseData() { return m_data.release(); }
    9797
    98         void setCacheable(bool cacheable) { m_cacheable = cacheable; }
    99         bool cacheable() const { return m_cacheable; }
     98        void setShouldCache(bool shouldCache) { m_shouldCache = shouldCache; }
     99        bool shouldCache() const { return m_shouldCache; }
    100100
    101101    private:
    … …  
    105105        IdentifierSet m_set;
    106106        JSGlobalData* m_globalData;
    107         bool m_cacheable;
     107        bool m_shouldCache;
    108108    };
    109109

  • trunk/JavaScriptCore/runtime/Structure.cpp

    r40608 r41232  
    124124    : m_typeInfo(typeInfo)
    125125    , m_prototype(prototype)
    126     , m_cachedPrototypeChain(0)
    127     , m_previous(0)
    128     , m_nameInPrevious(0)
    129126    , m_propertyTable(0)
    130127    , m_propertyStorageCapacity(JSObject::inlineStorageCapacity)
    … …  
    290287void Structure::getEnumerablePropertyNames(ExecState* exec, PropertyNameArray& propertyNames, JSObject* baseObject)
    291288{
    292     bool shouldCache = propertyNames.cacheable() && !(propertyNames.size() || m_isDictionary);
     289    bool shouldCache = propertyNames.shouldCache() && !(propertyNames.size() || m_isDictionary);
     290
     291    if (shouldCache && m_cachedPropertyNameArrayData) {
     292        if (m_cachedPropertyNameArrayData->cachedPrototypeChain() == prototypeChain(exec)) {
     293            propertyNames.setData(m_cachedPropertyNameArrayData);
     294            return;
     295        }
     296        clearEnumerationCache();
     297    }
     298
     299    getEnumerableNamesFromPropertyTable(propertyNames);
     300    getEnumerableNamesFromClassInfoTable(exec, baseObject->classInfo(), propertyNames);
     301
     302    if (m_prototype.isObject()) {
     303        propertyNames.setShouldCache(false); // No need for our prototypes to waste memory on caching, since they're not being enumerated directly.
     304        asObject(m_prototype)->getPropertyNames(exec, propertyNames);
     305    }
    293306
    294307    if (shouldCache) {
    295         if (m_cachedPropertyNameArrayData) {
    296             if (structureChainsAreEqual(m_cachedPropertyNameArrayData->cachedPrototypeChain(), cachedPrototypeChain())) {
    297                 propertyNames.setData(m_cachedPropertyNameArrayData);
    298                 return;
    299             }
    300         }
    301         propertyNames.setCacheable(false);
    302     }
    303 
    304     getEnumerablePropertyNamesInternal(propertyNames);
    305 
    306     // Add properties from the static hashtables of properties
    307     for (const ClassInfo* info = baseObject->classInfo(); info; info = info->parentClass) {
    308         const HashTable* table = info->propHashTable(exec);
    309         if (!table)
    310             continue;
    311         table->initializeIfNeeded(exec);
    312         ASSERT(table->table);
    313 #if ENABLE(PERFECT_HASH_SIZE)
    314         int hashSizeMask = table->hashSizeMask;
    315 #else
    316         int hashSizeMask = table->compactSize - 1;
    317 #endif
    318         const HashEntry* entry = table->table;
    319         for (int i = 0; i <= hashSizeMask; ++i, ++entry) {
    320             if (entry->key() && !(entry->attributes() & DontEnum))
    321                 propertyNames.add(entry->key());
    322         }
    323     }
    324 
    325     if (m_prototype.isObject())
    326         asObject(m_prototype)->getPropertyNames(exec, propertyNames);
    327 
    328     if (shouldCache) {
    329         if (m_cachedPropertyNameArrayData)
    330             m_cachedPropertyNameArrayData->setCachedStructure(0);
    331 
    332308        m_cachedPropertyNameArrayData = propertyNames.data();
    333 
    334         StructureChain* chain = cachedPrototypeChain();
    335         if (!chain)
    336             chain = createCachedPrototypeChain();
    337         m_cachedPropertyNameArrayData->setCachedPrototypeChain(chain);
     309        m_cachedPropertyNameArrayData->setCachedPrototypeChain(prototypeChain(exec));
    338310        m_cachedPropertyNameArrayData->setCachedStructure(this);
    339311    }
    … …  
    535507    clearEnumerationCache();
    536508    return offset;
    537 }
    538 
    539 StructureChain* Structure::createCachedPrototypeChain()
    540 {
    541     ASSERT(typeInfo().type() == ObjectType);
    542     ASSERT(!m_cachedPrototypeChain);
    543 
    544     JSValuePtr prototype = storedPrototype();
    545     if (!prototype.isCell())
    546         return 0;
    547 
    548     RefPtr<StructureChain> chain = StructureChain::create(asObject(prototype)->structure());
    549     setCachedPrototypeChain(chain.release());
    550     return cachedPrototypeChain();
    551509}
    552510
    … …  
    925883}
    926884
    927 void Structure::getEnumerablePropertyNamesInternal(PropertyNameArray& propertyNames)
     885void Structure::getEnumerableNamesFromPropertyTable(PropertyNameArray& propertyNames)
    928886{
    929887    materializePropertyMapIfNecessary();
    … …  
    979937        for (size_t i = 0; i < sortedEnumerables.size(); ++i)
    980938            propertyNames.add(sortedEnumerables[i]->key);
     939    }
     940}
     941
     942void Structure::getEnumerableNamesFromClassInfoTable(ExecState* exec, const ClassInfo* classInfo, PropertyNameArray& propertyNames)
     943{
     944    // Add properties from the static hashtables of properties
     945    for (; classInfo; classInfo = classInfo->parentClass) {
     946        const HashTable* table = classInfo->propHashTable(exec);
     947        if (!table)
     948            continue;
     949        table->initializeIfNeeded(exec);
     950        ASSERT(table->table);
     951#if ENABLE(PERFECT_HASH_SIZE)
     952        int hashSizeMask = table->hashSizeMask;
     953#else
     954        int hashSizeMask = table->compactSize - 1;
     955#endif
     956        const HashEntry* entry = table->table;
     957        for (int i = 0; i <= hashSizeMask; ++i, ++entry) {
     958            if (entry->key() && !(entry->attributes() & DontEnum))
     959                propertyNames.add(entry->key());
     960        }
    981961    }
    982962}

  • trunk/JavaScriptCore/runtime/Structure.h

    r40046 r41232  
    8888
    8989        JSValuePtr storedPrototype() const { return m_prototype; }
    90         JSValuePtr prototypeForLookup(ExecState*);
     90        JSValuePtr prototypeForLookup(ExecState*) const;
     91        StructureChain* prototypeChain(ExecState*) const;
    9192
    9293        Structure* previousID() const { return m_previous.get(); }
    93 
    94         StructureChain* createCachedPrototypeChain();
    95         void setCachedPrototypeChain(PassRefPtr<StructureChain> cachedPrototypeChain) { m_cachedPrototypeChain = cachedPrototypeChain; }
    96         StructureChain* cachedPrototypeChain() const { return m_cachedPrototypeChain.get(); }
    9794
    9895        void growPropertyStorageCapacity();
    … …  
    114111        size_t put(const Identifier& propertyName, unsigned attributes);
    115112        size_t remove(const Identifier& propertyName);
    116         void getEnumerablePropertyNamesInternal(PropertyNameArray&);
     113        void getEnumerableNamesFromPropertyTable(PropertyNameArray&);
     114        void getEnumerableNamesFromClassInfoTable(ExecState*, const ClassInfo*, PropertyNameArray&);
    117115
    118116        void expandPropertyMapHashTable();
    … …  
    145143            return m_offset == noOffset ? 0 : m_offset + 1;
    146144        }
     145       
     146        bool isValid(ExecState*, StructureChain* cachedPrototypeChain) const;
    147147
    148148        static const unsigned emptyEntryIndex = 0;
    … …  
    155155
    156156        JSValuePtr m_prototype;
    157         RefPtr<StructureChain> m_cachedPrototypeChain;
     157        mutable RefPtr<StructureChain> m_cachedPrototypeChain;
    158158
    159159        RefPtr<Structure> m_previous;

  • trunk/JavaScriptCore/runtime/StructureChain.cpp

    r40046 r41232  
    3333namespace JSC {
    3434
    35 StructureChain::StructureChain(Structure* structure)
     35StructureChain::StructureChain(Structure* head)
    3636{
    37     size_t size = 1;
    38 
    39     Structure* tmp = structure;
    40     while (!tmp->storedPrototype().isNull()) {
     37    size_t size = 0;
     38    for (Structure* current = head; current; current = current->storedPrototype().isNull() ? 0 : asObject(current->storedPrototype())->structure())
    4139        ++size;
    42         tmp = asCell(tmp->storedPrototype())->structure();
    43     }
    4440   
    4541    m_vector.set(new RefPtr<Structure>[size + 1]);
    4642
    47     size_t i;
    48     for (i = 0; i < size - 1; ++i) {
    49         m_vector[i] = structure;
    50         structure = asObject(structure->storedPrototype())->structure();
    51     }
    52     m_vector[i] = structure;
    53     m_vector[i + 1] = 0;
    54 }
    55 
    56 bool structureChainsAreEqual(StructureChain* chainA, StructureChain* chainB)
    57 {
    58     if (!chainA || !chainB)
    59         return false;
    60 
    61     RefPtr<Structure>* a = chainA->head();
    62     RefPtr<Structure>* b = chainB->head();
    63     while (1) {
    64         if (*a != *b)
    65             return false;
    66         if (!*a)
    67             return true;
    68         a++;
    69         b++;
    70     }
     43    size_t i = 0;
     44    for (Structure* current = head; current; current = current->storedPrototype().isNull() ? 0 : asObject(current->storedPrototype())->structure())
     45        m_vector[i++] = current;
     46    m_vector[i] = 0;
    7147}
    7248

  • trunk/JavaScriptCore/runtime/StructureChain.h

    r38440 r41232  
    3838    class StructureChain : public RefCounted<StructureChain> {
    3939    public:
    40         static PassRefPtr<StructureChain> create(Structure* structure) { return adoptRef(new StructureChain(structure)); }
    41 
     40        static PassRefPtr<StructureChain> create(Structure* head) { return adoptRef(new StructureChain(head)); }
    4241        RefPtr<Structure>* head() { return m_vector.get(); }
    4342
    4443    private:
    45         StructureChain(Structure* structure);
     44        StructureChain(Structure* head);
    4645
    4746        OwnArrayPtr<RefPtr<Structure> > m_vector;
    4847    };
    4948
    50     bool structureChainsAreEqual(StructureChain*, StructureChain*);
    51 
    5249} // namespace JSC
    5350
Note: See TracChangeset for help on using the changeset viewer.