Changeset 54798 in webkit


Ignore:
Timestamp:
Feb 15, 2010, 2:37:43 PM (16 years ago)
Author:
[email protected]
Message:

​https://bugs.webkit.org/show_bug.cgi?id=33731
Many false leaks in release builds due to PtrAndFlags

Reviewed by Darin Adler.

JavaScriptCore:

StructureTransitionTable was effectively a smart pointer type,
one machine word in size and wholly contained as a member of
of Structure. It either pointed to an actual table, or could
be used to describe a single transtion entry without use of a
table.

This, however, worked by using a PtrAndFlags, which is not
compatible with the leaks tool. Since there is no clear way to
obtain another bit for 'free' here, and since there are bits
available up in Structure, merge this functionality back up into
Structure. Having this in a separate class was quite clean
from an enacapsulation perspective, but this solution doesn't
seem to bad - all table access is now intermediated through the
Structure::structureTransitionTableFoo methods, keeping the
optimization fairly well contained.

This was the last use of PtrAndFlags, so removing the file too.

  • JavaScriptCore.xcodeproj/project.pbxproj:
  • bytecode/CodeBlock.h:
  • runtime/Structure.cpp:

(JSC::Structure::Structure):
(JSC::Structure::~Structure):
(JSC::Structure::addPropertyTransitionToExistingStructure):
(JSC::Structure::addPropertyTransition):
(JSC::Structure::hasTransition):

  • runtime/Structure.h:

(JSC::Structure::):
(JSC::Structure::structureTransitionTableContains):
(JSC::Structure::structureTransitionTableGet):
(JSC::Structure::structureTransitionTableHasTransition):
(JSC::Structure::structureTransitionTableRemove):
(JSC::Structure::structureTransitionTableAdd):
(JSC::Structure::structureTransitionTable):
(JSC::Structure::setStructureTransitionTable):
(JSC::Structure::singleTransition):
(JSC::Structure::setSingleTransition):

  • runtime/StructureTransitionTable.h:
  • wtf/PtrAndFlags.h: Removed.

WebCore:

PtrAndFlags has now been removed; remove forwarding header.

  • ForwardingHeaders/wtf/PtrAndFlags.h: Removed.
Location:
trunk
Files:
2 deleted
7 edited

Legend:

Unmodified
Added
Removed

  • trunk/JavaScriptCore/ChangeLog

    r54795 r54798  
     12010-02-14  Gavin Barraclough  <[email protected]>
     2
     3        Reviewed by Darin Adler.
     4
     5        https://bugs.webkit.org/show_bug.cgi?id=33731
     6        Many false leaks in release builds due to PtrAndFlags
     7
     8        StructureTransitionTable was effectively a smart pointer type,
     9        one machine word in size and wholly contained as a member of
     10        of Structure.  It either pointed to an actual table, or could
     11        be used to describe a single transtion entry without use of a
     12        table.
     13
     14        This, however, worked by using a PtrAndFlags, which is not
     15        compatible with the leaks tool.  Since there is no clear way to
     16        obtain another bit for 'free' here, and since there are bits
     17        available up in Structure, merge this functionality back up into
     18        Structure.  Having this in a separate class was quite clean
     19        from an enacapsulation perspective, but this solution doesn't
     20        seem to bad - all table access is now intermediated through the
     21        Structure::structureTransitionTableFoo methods, keeping the
     22        optimization fairly well contained.
     23
     24        This was the last use of PtrAndFlags, so removing the file too.
     25
     26        * JavaScriptCore.xcodeproj/project.pbxproj:
     27        * bytecode/CodeBlock.h:
     28        * runtime/Structure.cpp:
     29        (JSC::Structure::Structure):
     30        (JSC::Structure::~Structure):
     31        (JSC::Structure::addPropertyTransitionToExistingStructure):
     32        (JSC::Structure::addPropertyTransition):
     33        (JSC::Structure::hasTransition):
     34        * runtime/Structure.h:
     35        (JSC::Structure::):
     36        (JSC::Structure::structureTransitionTableContains):
     37        (JSC::Structure::structureTransitionTableGet):
     38        (JSC::Structure::structureTransitionTableHasTransition):
     39        (JSC::Structure::structureTransitionTableRemove):
     40        (JSC::Structure::structureTransitionTableAdd):
     41        (JSC::Structure::structureTransitionTable):
     42        (JSC::Structure::setStructureTransitionTable):
     43        (JSC::Structure::singleTransition):
     44        (JSC::Structure::setSingleTransition):
     45        * runtime/StructureTransitionTable.h:
     46        * wtf/PtrAndFlags.h: Removed.
     47
    1482010-02-15  Gavin Barraclough  <[email protected]>
    249

  • trunk/JavaScriptCore/JavaScriptCore.xcodeproj/project.pbxproj

    r54618 r54798  
    4040                088FA5BC0EF76D4300578E6F /* RandomNumber.h in Headers */ = {isa = PBXBuildFile; fileRef = 088FA5BA0EF76D4300578E6F /* RandomNumber.h */; settings = {ATTRIBUTES = (Private, ); }; };
    4141                08E279E90EF83B10007DB523 /* RandomNumberSeed.h in Headers */ = {isa = PBXBuildFile; fileRef = 08E279E80EF83B10007DB523 /* RandomNumberSeed.h */; };
    42                 0B1F921D0F1753500036468E /* PtrAndFlags.h in Headers */ = {isa = PBXBuildFile; fileRef = 0B1F921B0F17502D0036468E /* PtrAndFlags.h */; settings = {ATTRIBUTES = (Private, ); }; };
    4342                0B330C270F38C62300692DE3 /* TypeTraits.cpp in Sources */ = {isa = PBXBuildFile; fileRef = 0B330C260F38C62300692DE3 /* TypeTraits.cpp */; };
    4443                0B4D7E630F319AC800AD7E58 /* TypeTraits.h in Headers */ = {isa = PBXBuildFile; fileRef = 0B4D7E620F319AC800AD7E58 /* TypeTraits.h */; settings = {ATTRIBUTES = (Private, ); }; };
    … …  
    19821981                                BC18C4560E16F5CD00B34460 /* Protect.h in Headers */,
    19831982                                BC257DF40E1F53740016B6C9 /* PrototypeFunction.h in Headers */,
    1984                                 0B1F921D0F1753500036468E /* PtrAndFlags.h in Headers */,
    19851983                                147B84630E6DE6B1004775A4 /* PutPropertySlot.h in Headers */,
    19861984                                1429DA4A0ED245EC00B89619 /* Quantifier.h in Headers */,

  • trunk/JavaScriptCore/bytecode/CodeBlock.h

    r54747 r54798  
    3737#include "JumpTable.h"
    3838#include "Nodes.h"
    39 #include "PtrAndFlags.h"
    4039#include "RegExp.h"
    4140#include "UString.h"

  • trunk/JavaScriptCore/runtime/Structure.cpp

    r54265 r54798  
    8080static int comparePropertyMapEntryIndices(const void* a, const void* b);
    8181
     82inline void Structure::setTransitionTable(TransitionTable* table)
     83{
     84    ASSERT(m_isUsingSingleSlot);
     85#ifndef NDEBUG
     86    setSingleTransition(0);
     87#endif
     88    m_isUsingSingleSlot = false;
     89    m_transitions.m_table = table;
     90    // This implicitly clears the flag that indicates we're using a single transition
     91    ASSERT(!m_isUsingSingleSlot);
     92}
     93
     94// The contains and get methods accept imprecise matches, so if an unspecialised transition exists
     95// for the given key they will consider that transition to be a match.  If a specialised transition
     96// exists and it matches the provided specificValue, get will return the specific transition.
     97inline bool Structure::transitionTableContains(const StructureTransitionTableHash::Key& key, JSCell* specificValue)
     98{
     99    if (m_isUsingSingleSlot) {
     100        Structure* existingTransition = singleTransition();
     101        return existingTransition && existingTransition->m_nameInPrevious.get() == key.first
     102               && existingTransition->m_attributesInPrevious == key.second
     103               && (existingTransition->m_specificValueInPrevious == specificValue || existingTransition->m_specificValueInPrevious == 0);
     104    }
     105    TransitionTable::iterator find = transitionTable()->find(key);
     106    if (find == transitionTable()->end())
     107        return false;
     108
     109    return find->second.first || find->second.second->transitionedFor(specificValue);
     110}
     111
     112inline Structure* Structure::transitionTableGet(const StructureTransitionTableHash::Key& key, JSCell* specificValue) const
     113{
     114    if (m_isUsingSingleSlot) {
     115        Structure* existingTransition = singleTransition();
     116        if (existingTransition && existingTransition->m_nameInPrevious.get() == key.first
     117            && existingTransition->m_attributesInPrevious == key.second
     118            && (existingTransition->m_specificValueInPrevious == specificValue || existingTransition->m_specificValueInPrevious == 0))
     119            return existingTransition;
     120        return 0;
     121    }
     122
     123    Transition transition = transitionTable()->get(key);
     124    if (transition.second && transition.second->transitionedFor(specificValue))
     125        return transition.second;
     126    return transition.first;
     127}
     128
     129inline bool Structure::transitionTableHasTransition(const StructureTransitionTableHash::Key& key) const
     130{
     131    if (m_isUsingSingleSlot) {
     132        Structure* transition = singleTransition();
     133        return transition && transition->m_nameInPrevious == key.first
     134        && transition->m_attributesInPrevious == key.second;
     135    }
     136    return transitionTable()->contains(key);
     137}
     138
     139inline void Structure::transitionTableRemove(const StructureTransitionTableHash::Key& key, JSCell* specificValue)
     140{
     141    if (m_isUsingSingleSlot) {
     142        ASSERT(transitionTableContains(key, specificValue));
     143        setSingleTransition(0);
     144        return;
     145    }
     146    TransitionTable::iterator find = transitionTable()->find(key);
     147    if (!specificValue)
     148        find->second.first = 0;
     149    else
     150        find->second.second = 0;
     151    if (!find->second.first && !find->second.second)
     152        transitionTable()->remove(find);
     153}
     154
     155inline void Structure::transitionTableAdd(const StructureTransitionTableHash::Key& key, Structure* structure, JSCell* specificValue)
     156{
     157    if (m_isUsingSingleSlot) {
     158        if (!singleTransition()) {
     159            setSingleTransition(structure);
     160            return;
     161        }
     162        Structure* existingTransition = singleTransition();
     163        TransitionTable* transitionTable = new TransitionTable;
     164        setTransitionTable(transitionTable);
     165        if (existingTransition)
     166            transitionTableAdd(std::make_pair(existingTransition->m_nameInPrevious.get(), existingTransition->m_attributesInPrevious), existingTransition, existingTransition->m_specificValueInPrevious);
     167    }
     168    if (!specificValue) {
     169        TransitionTable::iterator find = transitionTable()->find(key);
     170        if (find == transitionTable()->end())
     171            transitionTable()->add(key, Transition(structure, 0));
     172        else
     173            find->second.first = structure;
     174    } else {
     175        // If we're adding a transition to a specific value, then there cannot be
     176        // an existing transition
     177        ASSERT(!transitionTable()->contains(key));
     178        transitionTable()->add(key, Transition(0, structure));
     179    }
     180}
     181
    82182void Structure::dumpStatistics()
    83183{
    … …  
    137237    , m_specificFunctionThrashCount(0)
    138238    , m_anonymousSlotCount(anonymousSlotCount)
    139 {
     239    , m_isUsingSingleSlot(true)
     240{
     241    m_transitions.m_singleTransition = 0;
     242
    140243    ASSERT(m_prototype);
    141244    ASSERT(m_prototype.isObject() || m_prototype.isNull());
    … …  
    160263    if (m_previous) {
    161264        ASSERT(m_nameInPrevious);
    162         m_previous->table.remove(make_pair(m_nameInPrevious.get(), m_attributesInPrevious), m_specificValueInPrevious);
     265        m_previous->transitionTableRemove(make_pair(m_nameInPrevious.get(), m_attributesInPrevious), m_specificValueInPrevious);
    163266
    164267    }
    … …  
    177280        fastFree(m_propertyTable);
    178281    }
     282
     283    if (!m_isUsingSingleSlot)
     284        delete transitionTable();
    179285
    180286#ifndef NDEBUG
    … …  
    341447    ASSERT(structure->typeInfo().type() == ObjectType);
    342448
    343     if (Structure* existingTransition = structure->table.get(make_pair(propertyName.ustring().rep(), attributes), specificValue)) {
     449    if (Structure* existingTransition = structure->transitionTableGet(make_pair(propertyName.ustring().rep(), attributes), specificValue)) {
    344450        ASSERT(existingTransition->m_offset != noOffset);
    345451        offset = existingTransition->m_offset + existingTransition->m_anonymousSlotCount;
    … …  
    406512    transition->m_offset = offset - structure->m_anonymousSlotCount;
    407513    ASSERT(structure->anonymousSlotCount() == transition->anonymousSlotCount());
    408     structure->table.add(make_pair(propertyName.ustring().rep(), attributes), transition.get(), specificValue);
     514    structure->transitionTableAdd(make_pair(propertyName.ustring().rep(), attributes), transition.get(), specificValue);
    409515    return transition.release();
    410516}
    … …  
    853959bool Structure::hasTransition(UString::Rep* rep, unsigned attributes)
    854960{
    855     return table.hasTransition(make_pair(rep, attributes));
     961    return transitionTableHasTransition(make_pair(rep, attributes));
    856962}
    857963

  • trunk/JavaScriptCore/runtime/Structure.h

    r54696 r54798  
    180180            return m_offset == noOffset ? 0 : m_offset + 1;
    181181        }
     182
     183        typedef std::pair<Structure*, Structure*> Transition;
     184        typedef HashMap<StructureTransitionTableHash::Key, Transition, StructureTransitionTableHash, StructureTransitionTableHashTraits> TransitionTable;
     185
     186        inline bool transitionTableContains(const StructureTransitionTableHash::Key& key, JSCell* specificValue);
     187        inline void transitionTableRemove(const StructureTransitionTableHash::Key& key, JSCell* specificValue);
     188        inline void transitionTableAdd(const StructureTransitionTableHash::Key& key, Structure* structure, JSCell* specificValue);
     189        inline bool transitionTableHasTransition(const StructureTransitionTableHash::Key& key) const;
     190        inline Structure* transitionTableGet(const StructureTransitionTableHash::Key& key, JSCell* specificValue) const;
     191
     192        TransitionTable* transitionTable() const { ASSERT(!m_isUsingSingleSlot); return m_transitions.m_table; }
     193        inline void setTransitionTable(TransitionTable* table);
     194        Structure* singleTransition() const { ASSERT(m_isUsingSingleSlot); return m_transitions.m_singleTransition; }
     195        void setSingleTransition(Structure* structure) { ASSERT(m_isUsingSingleSlot); m_transitions.m_singleTransition = structure; }
    182196       
    183197        bool isValid(ExecState*, StructureChain* cachedPrototypeChain) const;
    … …  
    200214        JSCell* m_specificValueInPrevious;
    201215
    202         StructureTransitionTable table;
     216        // 'm_isUsingSingleSlot' indicates whether we are using the single transition optimisation.
     217        union {
     218            TransitionTable* m_table;
     219            Structure* m_singleTransition;
     220        } m_transitions;
    203221
    204222        WeakGCPtr<JSPropertyNameIterator> m_enumerationCache;
    … …  
    225243        unsigned m_specificFunctionThrashCount : 2;
    226244        unsigned m_anonymousSlotCount : 5;
    227         // 5 free bits
     245        unsigned m_isUsingSingleSlot : 1;
     246        // 4 free bits
    228247    };
    229248
    … …  
    272291        }
    273292    }
    274    
    275     bool StructureTransitionTable::contains(const StructureTransitionTableHash::Key& key, JSCell* specificValue)
    276     {
    277         if (usingSingleTransitionSlot()) {
    278             Structure* existingTransition = singleTransition();
    279             return existingTransition && existingTransition->m_nameInPrevious.get() == key.first
    280                    && existingTransition->m_attributesInPrevious == key.second
    281                    && (existingTransition->m_specificValueInPrevious == specificValue || existingTransition->m_specificValueInPrevious == 0);
    282         }
    283         TransitionTable::iterator find = table()->find(key);
    284         if (find == table()->end())
    285             return false;
    286 
    287         return find->second.first || find->second.second->transitionedFor(specificValue);
    288     }
    289 
    290     Structure* StructureTransitionTable::get(const StructureTransitionTableHash::Key& key, JSCell* specificValue) const
    291     {
    292         if (usingSingleTransitionSlot()) {
    293             Structure* existingTransition = singleTransition();
    294             if (existingTransition && existingTransition->m_nameInPrevious.get() == key.first
    295                 && existingTransition->m_attributesInPrevious == key.second
    296                 && (existingTransition->m_specificValueInPrevious == specificValue || existingTransition->m_specificValueInPrevious == 0))
    297                 return existingTransition;
    298             return 0;
    299         }
    300 
    301         Transition transition = table()->get(key);
    302         if (transition.second && transition.second->transitionedFor(specificValue))
    303             return transition.second;
    304         return transition.first;
    305     }
    306 
    307     bool StructureTransitionTable::hasTransition(const StructureTransitionTableHash::Key& key) const
    308     {
    309         if (usingSingleTransitionSlot()) {
    310             Structure* transition = singleTransition();
    311             return transition && transition->m_nameInPrevious == key.first
    312             && transition->m_attributesInPrevious == key.second;
    313         }
    314         return table()->contains(key);
    315     }
    316    
    317     void StructureTransitionTable::reifySingleTransition()
    318     {
    319         ASSERT(usingSingleTransitionSlot());
    320         Structure* existingTransition = singleTransition();
    321         TransitionTable* transitionTable = new TransitionTable;
    322         setTransitionTable(transitionTable);
    323         if (existingTransition)
    324             add(std::make_pair(existingTransition->m_nameInPrevious.get(), existingTransition->m_attributesInPrevious), existingTransition, existingTransition->m_specificValueInPrevious);
    325     }
     293
    326294} // namespace JSC
    327295

  • trunk/JavaScriptCore/runtime/StructureTransitionTable.h

    r54022 r54798  
    3131#include <wtf/HashMap.h>
    3232#include <wtf/HashTraits.h>
    33 #include <wtf/PtrAndFlags.h>
    3433#include <wtf/OwnPtr.h>
    3534#include <wtf/RefPtr.h>
    … …  
    6867    };
    6968
    70     class StructureTransitionTable {
    71         typedef std::pair<Structure*, Structure*> Transition;
    72         typedef HashMap<StructureTransitionTableHash::Key, Transition, StructureTransitionTableHash, StructureTransitionTableHashTraits> TransitionTable;
    73     public:
    74         StructureTransitionTable() {
    75             m_transitions.m_singleTransition.set(0);
    76             m_transitions.m_singleTransition.setFlag(usingSingleSlot);
    77         }
    78 
    79         ~StructureTransitionTable() {
    80             if (!usingSingleTransitionSlot())
    81                 delete table();
    82         }
    83 
    84         // The contains and get methods accept imprecise matches, so if an unspecialised transition exists
    85         // for the given key they will consider that transition to be a match.  If a specialised transition
    86         // exists and it matches the provided specificValue, get will return the specific transition.
    87         inline bool contains(const StructureTransitionTableHash::Key&, JSCell* specificValue);
    88         inline Structure* get(const StructureTransitionTableHash::Key&, JSCell* specificValue) const;
    89         inline bool hasTransition(const StructureTransitionTableHash::Key& key) const;
    90         void remove(const StructureTransitionTableHash::Key& key, JSCell* specificValue)
    91         {
    92             if (usingSingleTransitionSlot()) {
    93                 ASSERT(contains(key, specificValue));
    94                 setSingleTransition(0);
    95                 return;
    96             }
    97             TransitionTable::iterator find = table()->find(key);
    98             if (!specificValue)
    99                 find->second.first = 0;
    100             else
    101                 find->second.second = 0;
    102             if (!find->second.first && !find->second.second)
    103                 table()->remove(find);
    104         }
    105         void add(const StructureTransitionTableHash::Key& key, Structure* structure, JSCell* specificValue)
    106         {
    107             if (usingSingleTransitionSlot()) {
    108                 if (!singleTransition()) {
    109                     setSingleTransition(structure);
    110                     return;
    111                 }
    112                 reifySingleTransition();
    113             }
    114             if (!specificValue) {
    115                 TransitionTable::iterator find = table()->find(key);
    116                 if (find == table()->end())
    117                     table()->add(key, Transition(structure, 0));
    118                 else
    119                     find->second.first = structure;
    120             } else {
    121                 // If we're adding a transition to a specific value, then there cannot be
    122                 // an existing transition
    123                 ASSERT(!table()->contains(key));
    124                 table()->add(key, Transition(0, structure));
    125             }
    126         }
    127 
    128     private:
    129         TransitionTable* table() const { ASSERT(!usingSingleTransitionSlot()); return m_transitions.m_table; }
    130         Structure* singleTransition() const {
    131             ASSERT(usingSingleTransitionSlot());
    132             return m_transitions.m_singleTransition.get();
    133         }
    134         bool usingSingleTransitionSlot() const { return m_transitions.m_singleTransition.isFlagSet(usingSingleSlot); }
    135         void setSingleTransition(Structure* structure)
    136         {
    137             ASSERT(usingSingleTransitionSlot());
    138             m_transitions.m_singleTransition.set(structure);
    139         }
    140 
    141         void setTransitionTable(TransitionTable* table)
    142         {
    143             ASSERT(usingSingleTransitionSlot());
    144 #ifndef NDEBUG
    145             setSingleTransition(0);
    146 #endif
    147             m_transitions.m_table = table;
    148             // This implicitly clears the flag that indicates we're using a single transition
    149             ASSERT(!usingSingleTransitionSlot());
    150         }
    151         inline void reifySingleTransition();
    152 
    153         enum UsingSingleSlot {
    154             usingSingleSlot
    155         };
    156         // Last bit indicates whether we are using the single transition optimisation
    157         union {
    158             TransitionTable* m_table;
    159             PtrAndFlagsBase<Structure, UsingSingleSlot> m_singleTransition;
    160         } m_transitions;
    161     };
    162 
    16369} // namespace JSC
    16470

  • trunk/WebCore/ChangeLog

    r54789 r54798  
     12010-02-14  Gavin Barraclough  <[email protected]>
     2
     3        Reviewed by Darin Adler.
     4
     5        https://bugs.webkit.org/show_bug.cgi?id=33731
     6        Many false leaks in release builds due to PtrAndFlags
     7
     8        PtrAndFlags has now been removed; remove forwarding header.
     9
     10        * ForwardingHeaders/wtf/PtrAndFlags.h: Removed.
     11
    1122010-02-15  Gavin Barraclough  <[email protected]>
    213
Note: See TracChangeset for help on using the changeset viewer.