Changeset 32222 in webkit for trunk/JavaScriptCore/kjs/identifier.cpp

Timestamp:

Apr 18, 2008, 12:46:13 PM (17 years ago)

Author:

[email protected]

Message:

Reviewed by Darin.

Fix leaks during plugin tests (which actually excercise background JS), and potential
PAC brokenness that was not reported, but very likely.

The leaks shadowed a bigger problem with Identifier destruction. Identifier::remove involves
an IdentifierTable lookup, which is now a per-thread instance. Since garbage collection can
currently happen on a different thread than allocation, a wrong table was used.

No measurable change on SunSpider total, ~1% variation on individual tests.

• kjs/ustring.cpp: (KJS::): (KJS::UString::Rep::create): (KJS::UString::Rep::destroy):
• kjs/ustring.h: Replaced isIdentifier with a pointer to IdentifierTable, so that destruction can be done correctly. Took one bit from reportedCost, to avoid making UString::Rep larger (performance effect was measurable on SunSpider).

• kjs/identifier.cpp: (KJS::IdentifierTable::IdentifierTable): (KJS::IdentifierTable::~IdentifierTable): (KJS::IdentifierTable::add): (KJS::IdentifierTable::remove): Make IdentifierTable a real class. Its destructor needs to zero out outstanding references, because some identifiers may briefly outlive it during thread destruction, and we don't want them to use their stale pointers.

(KJS::LiteralIdentifierTable):
(KJS::Identifier::add):
Now that LiteralIdentifierTable is per-thread and can be destroyed not just during application
shutdown, it is not appropriate to simply bump refcount for strings that get there; changed
the table to hold RefPtrs.

(KJS::CStringTranslator::translate):
(KJS::UCharBufferTranslator::translate):
(KJS::Identifier::addSlowCase):
(KJS::Identifier::remove):

• kjs/identifier.h: (KJS::Identifier::add): Use and update UString::Rep::identifierTable as appropriate. Updating it is now done in IdentifierTable::add, not in translators.

File:

• trunk/JavaScriptCore/kjs/identifier.cpp (modified) (6 diffs)

trunk/JavaScriptCore/kjs/identifier.cpp

@@ -53 +53 @@
 namespace KJS {
 
-typedef HashSet<UString::Rep*> IdentifierTable;
-typedef HashMap<const char*, UString::Rep*, PtrHash<const char*> > LiteralIdentifierTable;
+class IdentifierTable {
+public:
+    ~IdentifierTable()
+    {
+        HashSet<UString::Rep*>::iterator end = m_table.end();
+        for (HashSet<UString::Rep*>::iterator iter = m_table.begin(); iter != end; ++iter)
+            (*iter)->identifierTable = 0;
+    }
+    
+    std::pair<HashSet<UString::Rep*>::iterator, bool> add(UString::Rep* value)
+    {
+        std::pair<HashSet<UString::Rep*>::iterator, bool> result = m_table.add(value);
+        (*result.first)->identifierTable = this;
+        return result;
+    }
+
+    template<typename U, typename V>
+    std::pair<HashSet<UString::Rep*>::iterator, bool> add(U value)
+    {
+        std::pair<HashSet<UString::Rep*>::iterator, bool> result = m_table.add<U, V>(value);
+        (*result.first)->identifierTable = this;
+        return result;
+    }
+
+    void remove(UString::Rep* r) { m_table.remove(r); }
+
+private:
+    HashSet<UString::Rep*> m_table;
+};
+
+typedef HashMap<const char*, RefPtr<UString::Rep>, PtrHash<const char*> > LiteralIdentifierTable;
 
 static inline IdentifierTable& identifierTable()
@@ -138 +167 @@
         
         UString::Rep *r = UString::Rep::create(d, static_cast<int>(length)).releaseRef();
-        r->isIdentifier = true;
         r->rc = 0;
         r->_hash = hash;
@@ -166 +194 @@
     UString::Rep* addedString = *identifierTable().add<const char*, CStringTranslator>(c).first;
     literalTableLocalRef.add(c, addedString);
-    addedString->ref();
 
     return addedString;
@@ -195 +222 @@
         
         UString::Rep *r = UString::Rep::create(d, buf.length).releaseRef();
-        r->isIdentifier = true;
         r->rc = 0;
         r->_hash = hash;
@@ -216 +242 @@
 PassRefPtr<UString::Rep> Identifier::addSlowCase(UString::Rep *r)
 {
-    ASSERT(!r->isIdentifier);
+    ASSERT(!r->identifierTable);
 
     if (r->len == 0) {
@@ -223 +249 @@
     }
 
-    UString::Rep *result = *identifierTable().add(r).first;
-    if (result == r)
-        r->isIdentifier = true;
-    return result;
+    return *identifierTable().add(r).first;
 }
 
 void Identifier::remove(UString::Rep *r)
 {
-    identifierTable().remove(r);
+    r->identifierTable->remove(r);
 }