Bug 534136 Part 5: Clean up atomtable hash entries. r=jst
authorJonas Sicking <jonas@sicking.cc>
Mon, 08 Mar 2010 07:45:00 -0800
changeset 39103 62bc77046ac9bbf386f2258766f18bf382fcde36
parent 39102 5901c6b98f836784818fc0495a1775f02726ee5c
child 39104 9cb12f3ff15679669d5f45600a533dec6afa1cd3
push id12012
push usersicking@mozilla.com
push dateMon, 08 Mar 2010 15:47:04 +0000
treeherdermozilla-central@f8dd7b5b02ef [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewersjst
bugs534136
milestone1.9.3a3pre
Bug 534136 Part 5: Clean up atomtable hash entries. r=jst
xpcom/ds/nsAtomTable.cpp
--- a/xpcom/ds/nsAtomTable.cpp
+++ b/xpcom/ds/nsAtomTable.cpp
@@ -69,213 +69,101 @@ static PLDHashTable gAtomTable;
  */
 static nsDataHashtable<nsStringHashKey, nsIAtom*>* gStaticAtomTable = 0;
 
 /**
  * Whether it is still OK to add atoms to gStaticAtomTable.
  */
 static PRBool gStaticAtomTableSealed = PR_FALSE;
 
-// The |key| pointer in the various PLDHashTable callbacks we use is an
-// AtomTableClearEntry*.  These pointers can come from two places: either a
-// (probably stack-allocated) string key being passed to PL_DHashTableOperate,
-// or an actual entry in the atom table. PLDHashTable reseves the keyHash
-// values 0 and 1 for internal use, which means that the *PLDHashTable code*
-// will never pass an entry whose keyhash is 0 or 1 to our hooks. That means we
-// can use those values to tell whether an AtomTableEntry is a string key
-// created by a PLDHashTable code caller or an actual live AtomTableEntry used
-// by our PLDHashTable.
-//
-// Evil? Yes, but kinda neat too :-)
-//
-// An AtomTableEntry is a UTF-8 string key if keyHash is 0, in that
-// case mBits points to a UTF-8 encoded char *. If keyHash is 1 the
-// AtomTableEntry is a UTF-16 encoded string key and mBits points to a
-// UTF-16 encoded PRUnichar *.
-//
-// If keyHash is any other value (> 1), the AtomTableEntry is an
-// actual live entry in the table, and then mBits & ~0x1 in the
-// AtomTableEntry points to an AtomImpl or a nsStaticAtomWrapper,
-// indicated by the first bit of PtrBits.
-// XXX This whole mess could be vastly simplified now that pldhash
-// no longer has a getKey callback.
-typedef PRUword PtrBits;
+//----------------------------------------------------------------------
 
 struct AtomTableEntry : public PLDHashEntryHdr {
-  // If keyHash > 1, mBits & 0x1 means (mBits & ~0x1) points to an
-  // nsStaticAtomWrapper else it points to an nsAtomImpl
-  PtrBits mBits;
-  PRUint32 mLength;
+  AtomImpl* mAtom;
+};
 
-  inline AtomTableEntry(const char *aString, PRUint32 aLength)
-    : mBits(PtrBits(aString)), mLength(aLength)
-  {
-    keyHash = 0;
-  }
-
-  inline AtomTableEntry(const PRUnichar *aString, PRUint32 aLength)
-    : mBits(PtrBits(aString)), mLength(aLength)
+struct AtomTableKey
+{
+  AtomTableKey(const PRUnichar* aUTF16String, PRUint32 aLength)
+    : mUTF16String(aUTF16String),
+      mUTF8String(nsnull),
+      mLength(aLength)
   {
-    keyHash = 1;
-  }
-
-  inline PRBool IsUTF8String() const {
-    return keyHash == 0;
-  }
-
-  inline PRBool IsUTF16String() const {
-    return keyHash == 1;
-  }
-
-  inline void SetAtomImpl(AtomImpl* aAtom) {
-    NS_ASSERTION(keyHash > 1,
-                 "SetAtomImpl() called on non-atom AtomTableEntry!");
-    NS_ASSERTION(aAtom, "Setting null atom");
-    mBits = PtrBits(aAtom);
-    mLength = aAtom->GetLength();
   }
 
-  inline void ClearAtom() {
-    mBits = nsnull;
-  }
-
-  inline PRBool HasValue() const {
-    NS_ASSERTION(keyHash > 1,
-                 "HasValue() called on non-atom AtomTableEntry!");
-    return (mBits & ~0x1) != 0;
-  }
-
-  // these accessors assume that you already know the type
-  inline AtomImpl *GetAtomImpl() const {
-    NS_ASSERTION(keyHash > 1,
-                 "GetAtomImpl() called on non-atom AtomTableEntry!");
-    return (AtomImpl*) (mBits & ~0x1);
-  }
-
-  // type-agnostic accessors
-
-  // get the string buffer
-  inline const PRUnichar* getAtomString() const {
-    NS_ASSERTION(keyHash > 1,
-                 "getAtomString() called on non-atom AtomTableEntry!");
-
-    return GetAtomImpl()->GetUTF16String();
+  AtomTableKey(const char* aUTF8String, PRUint32 aLength)
+    : mUTF16String(nsnull),
+      mUTF8String(aUTF8String),
+      mLength(aLength)
+  {
   }
 
-  // get the string buffer
-  inline const char* getUTF8String() const {
-    NS_ASSERTION(keyHash == 0,
-                 "getUTF8String() called on non-UTF8 AtomTableEntry!");
-
-    return (char *)mBits;
-  }
-
-  // get the string buffer
-  inline const PRUnichar* getUTF16String() const {
-    NS_ASSERTION(keyHash == 1,
-                 "getUTF16String() called on non-UTF16 AtomTableEntry!");
-
-    return (PRUnichar *)mBits;
-  }
-
-  // get the string length
-  inline PRUint32 getLength() const {
-    return mLength;
-  }
-
-  // get an addreffed nsIAtom - not using already_AddRef'ed atom
-  // because the callers are not (and should not be) using nsCOMPtr
-  inline nsIAtom* GetAtom() const {
-    NS_ASSERTION(keyHash > 1,
-                 "GetAtom() called on non-atom AtomTableEntry!");
-
-    nsIAtom* result;
-    
-    NS_ADDREF(result = GetAtomImpl());
-    
-    return result;
-  }
+  const PRUnichar* mUTF16String;
+  const char* mUTF8String;
+  PRUint32 mLength;
 };
 
 static PLDHashNumber
 AtomTableGetHash(PLDHashTable *table, const void *key)
 {
-  const AtomTableEntry *e = static_cast<const AtomTableEntry*>(key);
+  const AtomTableKey *k = static_cast<const AtomTableKey*>(key);
 
-  if (e->IsUTF8String()) {
-    return nsCRT::HashCodeAsUTF16(e->getUTF8String(), e->getLength());;
+  if (k->mUTF8String) {
+    return nsCRT::HashCodeAsUTF16(k->mUTF8String, k->mLength);;
   }
 
-  NS_ASSERTION(e->IsUTF16String(),
-               "AtomTableGetHash() called on non-string-key AtomTableEntry!");
-
-  return nsCRT::HashCode(e->getUTF16String(), e->getLength());
+  return nsCRT::HashCode(k->mUTF16String, k->mLength);
 }
 
 static PRBool
 AtomTableMatchKey(PLDHashTable *table, const PLDHashEntryHdr *entry,
                   const void *key)
 {
   const AtomTableEntry *he = static_cast<const AtomTableEntry*>(entry);
-  const AtomTableEntry *strKey = static_cast<const AtomTableEntry*>(key);
-
-  const PRUnichar *atomString = he->getAtomString();
+  const AtomTableKey *k = static_cast<const AtomTableKey*>(key);
 
-  if (strKey->IsUTF8String()) {
+  if (k->mUTF8String) {
     return
-      CompareUTF8toUTF16(nsDependentCSubstring(strKey->getUTF8String(),
-                                               strKey->getUTF8String() + strKey->getLength()),
-                         nsDependentSubstring(atomString, atomString + he->getLength())) == 0;
+      CompareUTF8toUTF16(nsDependentCSubstring(k->mUTF8String,
+                                               k->mUTF8String + k->mLength),
+                         nsDependentAtomString(he->mAtom)) == 0;
   }
 
-  PRUint32 length = he->getLength();
-  if (length != strKey->getLength()) {
+  PRUint32 length = he->mAtom->GetLength();
+  if (length != k->mLength) {
     return PR_FALSE;
   }
 
-  const PRUnichar *str;
-
-  if (strKey->IsUTF16String()) {
-    str = strKey->getUTF16String();
-  } else {
-    str = strKey->getAtomString();
-  }
-
-  return memcmp(atomString, str, length * sizeof(PRUnichar)) == 0;
+  return memcmp(he->mAtom->GetUTF16String(),
+                k->mUTF16String, length * sizeof(PRUnichar)) == 0;
 }
 
 static void
 AtomTableClearEntry(PLDHashTable *table, PLDHashEntryHdr *entry)
 {
-  AtomTableEntry *he = static_cast<AtomTableEntry*>(entry);
-  
-  AtomImpl *atom = he->GetAtomImpl();
   // Normal |AtomImpl| atoms are deleted when their refcount hits 0, and
   // they then remove themselves from the table.  In other words, they
   // are owned by the callers who own references to them.
   // |PermanentAtomImpl| permanent atoms ignore their refcount and are
   // deleted when they are removed from the table at table destruction.
   // In other words, they are owned by the atom table.
+
+  AtomImpl *atom = static_cast<AtomTableEntry*>(entry)->mAtom;
   if (atom->IsPermanent()) {
-    he->keyHash = 0;
-
+    // Note that the cast here is important since AtomImpls doesn't have a
+    // virtual dtor.
     delete static_cast<PermanentAtomImpl*>(atom);
   }
-  
-  he->ClearAtom();
 }
 
 static PRBool
 AtomTableInitEntry(PLDHashTable *table, PLDHashEntryHdr *entry,
                    const void *key)
 {
-  AtomTableEntry *he = static_cast<AtomTableEntry*>(entry);
-  const AtomTableEntry *strKey = static_cast<const AtomTableEntry*>(key);
-
-  he->mLength = strKey->getLength();
+  static_cast<AtomTableEntry*>(entry)->mAtom = nsnull;
 
   return PR_TRUE;
 }
 
 
 static const PLDHashTableOps AtomTableOps = {
   PL_DHashAllocTable,
   PL_DHashFreeTable,
@@ -290,17 +178,17 @@ static const PLDHashTableOps AtomTableOp
 
 #ifdef DEBUG
 static PLDHashOperator
 DumpAtomLeaks(PLDHashTable *table, PLDHashEntryHdr *he,
               PRUint32 index, void *arg)
 {
   AtomTableEntry *entry = static_cast<AtomTableEntry*>(he);
   
-  AtomImpl* atom = entry->GetAtomImpl();
+  AtomImpl* atom = entry->mAtom;
   if (!atom->IsPermanent()) {
     ++*static_cast<PRUint32*>(arg);
     nsCAutoString str;
     atom->ToUTF8String(str);
     fputs(str.get(), stdout);
     fputs("\n", stdout);
   }
   return PL_DHASH_NEXT;
@@ -378,17 +266,17 @@ AtomImpl::AtomImpl(nsStringBuffer* aStri
 
 AtomImpl::~AtomImpl()
 {
   NS_PRECONDITION(gAtomTable.ops, "uninitialized atom hashtable");
   // Permanent atoms are removed from the hashtable at shutdown, and we
   // don't want to remove them twice.  See comment above in
   // |AtomTableClearEntry|.
   if (!IsPermanentInDestructor()) {
-    AtomTableEntry key(mString, mLength);
+    AtomTableKey key(mString, mLength);
     PL_DHashTableOperate(&gAtomTable, &key, PL_DHASH_REMOVE);
     if (gAtomTable.entryCount == 0) {
       PL_DHashTableFinish(&gAtomTable);
       NS_ASSERTION(gAtomTable.entryCount == 0,
                    "PL_DHashTableFinish changed the entry count");
     }
   }
 
@@ -479,33 +367,33 @@ GetAtomHashEntry(const char* aString, PR
   NS_ASSERTION(NS_IsMainThread(), "wrong thread");
   if (!gAtomTable.ops &&
       !PL_DHashTableInit(&gAtomTable, &AtomTableOps, 0,
                          sizeof(AtomTableEntry), ATOM_HASHTABLE_INITIAL_SIZE)) {
     gAtomTable.ops = nsnull;
     return nsnull;
   }
 
-  AtomTableEntry key(aString, aLength);
+  AtomTableKey key(aString, aLength);
   return static_cast<AtomTableEntry*>
                     (PL_DHashTableOperate(&gAtomTable, &key, PL_DHASH_ADD));
 }
 
 static inline AtomTableEntry*
 GetAtomHashEntry(const PRUnichar* aString, PRUint32 aLength)
 {
   NS_ASSERTION(NS_IsMainThread(), "wrong thread");
   if (!gAtomTable.ops &&
       !PL_DHashTableInit(&gAtomTable, &AtomTableOps, 0,
                          sizeof(AtomTableEntry), ATOM_HASHTABLE_INITIAL_SIZE)) {
     gAtomTable.ops = nsnull;
     return nsnull;
   }
 
-  AtomTableEntry key(aString, aLength);
+  AtomTableKey key(aString, aLength);
   return static_cast<AtomTableEntry*>
                     (PL_DHashTableOperate(&gAtomTable, &key, PL_DHASH_ADD));
 }
 
 class CheckStaticAtomSizes
 {
   CheckStaticAtomSizes() {
     PR_STATIC_ASSERT((sizeof(nsFakeStringBuffer<1>().mRefCnt) ==
@@ -545,32 +433,32 @@ NS_RegisterStaticAtoms(const nsStaticAto
 
     PRUint32 stringLen =
       aAtoms[i].mStringBuffer->StorageSize() / sizeof(PRUnichar) - 1;
 
     AtomTableEntry *he =
       GetAtomHashEntry((PRUnichar*)aAtoms[i].mStringBuffer->Data(),
                        stringLen);
 
-    if (he->HasValue()) {
+    if (he->mAtom) {
       // there already is an atom with this name in the table.. but we
       // still have to update mBits
-      if (!he->GetAtomImpl()->IsPermanent()) {
+      if (!he->mAtom->IsPermanent()) {
         // since we wanted to create a static atom but there is
         // already one there, we convert it to a non-refcounting
         // permanent atom
-        PromoteToPermanent(he->GetAtomImpl());
+        PromoteToPermanent(he->mAtom);
       }
       
-      *aAtoms[i].mAtom = he->GetAtom();
+      *aAtoms[i].mAtom = he->mAtom;
     }
     else {
       AtomImpl* atom = new PermanentAtomImpl(aAtoms[i].mStringBuffer,
                                              stringLen);
-      he->SetAtomImpl(atom);
+      he->mAtom = atom;
       *aAtoms[i].mAtom = atom;
 
       if (!gStaticAtomTableSealed) {
         gStaticAtomTable->Put(nsAtomString(atom), atom);
       }
     }
 #else // NS_STATIC_ATOM_USE_WIDE_STRINGS
     NS_ASSERTION(nsCRT::IsAscii((char*)aAtoms[i].mStringBuffer->Data()),
@@ -599,30 +487,30 @@ NS_NewAtom(const char* aUTF8String)
 }
 
 NS_COM nsIAtom*
 NS_NewAtom(const nsACString& aUTF8String)
 {
   AtomTableEntry *he = GetAtomHashEntry(aUTF8String.Data(),
                                         aUTF8String.Length());
 
-  if (!he) {
-    return nsnull;
+  if (he->mAtom) {
+    nsIAtom* atom;
+    NS_ADDREF(atom = he->mAtom);
+
+    return atom;
   }
 
-  if (he->HasValue())
-    return he->GetAtom();
-
   // This results in an extra addref/release of the nsStringBuffer.
   // Unfortunately there doesn't seem to be any APIs to avoid that.
   nsString str;
   CopyUTF8toUTF16(aUTF8String, str);
   AtomImpl* atom = new AtomImpl(str);
 
-  he->SetAtomImpl(atom);
+  he->mAtom = atom;
   NS_ADDREF(atom);
 
   return atom;
 }
 
 NS_COM nsIAtom*
 NS_NewAtom(const PRUnichar* aUTF16String)
 {
@@ -630,45 +518,48 @@ NS_NewAtom(const PRUnichar* aUTF16String
 }
 
 NS_COM nsIAtom*
 NS_NewAtom(const nsAString& aUTF16String)
 {
   AtomTableEntry *he = GetAtomHashEntry(aUTF16String.Data(),
                                         aUTF16String.Length());
 
-  if (he->HasValue())
-    return he->GetAtom();
+  if (he->mAtom) {
+    nsIAtom* atom;
+    NS_ADDREF(atom = he->mAtom);
+
+    return atom;
+  }
 
   AtomImpl* atom = new AtomImpl(aUTF16String);
-  he->SetAtomImpl(atom);
+  he->mAtom = atom;
   NS_ADDREF(atom);
 
   return atom;
 }
 
 NS_COM nsIAtom*
 NS_NewPermanentAtom(const nsAString& aUTF16String)
 {
   AtomTableEntry *he = GetAtomHashEntry(aUTF16String.Data(),
                                         aUTF16String.Length());
 
-  if (he->HasValue()) {
-    AtomImpl* atom = he->GetAtomImpl();
+  AtomImpl* atom = he->mAtom;
+  if (atom) {
     if (!atom->IsPermanent()) {
       PromoteToPermanent(atom);
     }
-    return atom;
   }
-  
-  AtomImpl* atom = new PermanentAtomImpl(aUTF16String);
-  he->SetAtomImpl(atom);
+  else {
+    atom = new PermanentAtomImpl(aUTF16String);
+    he->mAtom = atom;
+  }
 
   // No need to addref since permanent atoms aren't refcounted anyway
-
   return atom;
 }
 
 NS_COM nsrefcnt
 NS_GetNumberOfAtoms(void)
 {
   return gAtomTable.entryCount;
 }