Bug 815467 - Store pldhash's recursionLevel in a better place, so that debug and non-debug pldhashes take up the same amount of memory. r=dbaron.
authorNicholas Nethercote <nnethercote@mozilla.com>
Wed, 30 Oct 2013 15:10:06 -0700
changeset 153296 de06b0e593e6e98241cc7141569e349aa8328e91
parent 153295 8a236c59b0f0bb2b4c487653a0e95a20fad1b826
child 153297 a9ad2b09c37fa64ec862fe27bd5f13103b8bc19e
push id270
push userpvanderbeken@mozilla.com
push dateThu, 06 Mar 2014 09:24:21 +0000
reviewersdbaron
bugs815467
milestone28.0a1
Bug 815467 - Store pldhash's recursionLevel in a better place, so that debug and non-debug pldhashes take up the same amount of memory. r=dbaron.
xpcom/glue/pldhash.cpp
xpcom/glue/pldhash.h
--- a/xpcom/glue/pldhash.cpp
+++ b/xpcom/glue/pldhash.cpp
@@ -24,57 +24,49 @@
 # define METER(x)       x
 #else
 # define METER(x)       /* nothing */
 #endif
 
 /*
  * The following DEBUG-only code is used to assert that calls to one of
  * table->ops or to an enumerator do not cause re-entry into a call that
- * can mutate the table.  The recursion level is stored in additional
- * space allocated at the end of the entry store to avoid changing
- * PLDHashTable, which could cause issues when mixing DEBUG and
- * non-DEBUG components.
+ * can mutate the table.
  */
 #ifdef DEBUG
 
-#define RECURSION_LEVEL(table_) (*(uint32_t*)(table_->entryStore + \
-                                            PL_DHASH_TABLE_SIZE(table_) * \
-                                            table_->entrySize))
 /*
  * Most callers that assert about the recursion level don't care about
  * this magical value because they are asserting that mutation is
  * allowed (and therefore the level is 0 or 1, depending on whether they
  * incremented it).
  *
  * Only PL_DHashTableFinish needs to allow this special value.
  */
-#define IMMUTABLE_RECURSION_LEVEL ((uint32_t)-1)
+#define IMMUTABLE_RECURSION_LEVEL ((uint16_t)-1)
 
 #define RECURSION_LEVEL_SAFE_TO_FINISH(table_)                                \
-    (RECURSION_LEVEL(table_) == 0 ||                                          \
-     RECURSION_LEVEL(table_) == IMMUTABLE_RECURSION_LEVEL)
+    (table_->recursionLevel == 0 ||                                           \
+     table_->recursionLevel == IMMUTABLE_RECURSION_LEVEL)
 
-#define ENTRY_STORE_EXTRA                   sizeof(uint32_t)
 #define INCREMENT_RECURSION_LEVEL(table_)                                     \
     PR_BEGIN_MACRO                                                            \
-        if (RECURSION_LEVEL(table_) != IMMUTABLE_RECURSION_LEVEL)             \
-            ++RECURSION_LEVEL(table_);                                        \
+        if (table_->recursionLevel != IMMUTABLE_RECURSION_LEVEL)              \
+            ++table_->recursionLevel;                                         \
     PR_END_MACRO
 #define DECREMENT_RECURSION_LEVEL(table_)                                     \
     PR_BEGIN_MACRO                                                            \
-        if (RECURSION_LEVEL(table_) != IMMUTABLE_RECURSION_LEVEL) {           \
-            MOZ_ASSERT(RECURSION_LEVEL(table_) > 0);                          \
-            --RECURSION_LEVEL(table_);                                        \
+        if (table->recursionLevel != IMMUTABLE_RECURSION_LEVEL) {             \
+            MOZ_ASSERT(table->recursionLevel > 0);                            \
+            --table->recursionLevel;                                          \
         }                                                                     \
     PR_END_MACRO
 
 #else
 
-#define ENTRY_STORE_EXTRA 0
 #define INCREMENT_RECURSION_LEVEL(table_)   PR_BEGIN_MACRO PR_END_MACRO
 #define DECREMENT_RECURSION_LEVEL(table_)   PR_BEGIN_MACRO PR_END_MACRO
 
 #endif /* defined(DEBUG) */
 
 using namespace mozilla;
 
 void *
@@ -229,25 +221,24 @@ PL_DHashTableInit(PLDHashTable *table, c
         return false;
     table->hashShift = PL_DHASH_BITS - log2;
     table->entrySize = entrySize;
     table->entryCount = table->removedCount = 0;
     table->generation = 0;
     if (!SizeOfEntryStore(capacity, entrySize, &nbytes))
         return false;   // overflowed
 
-    table->entryStore = (char *) ops->allocTable(table,
-                                                 nbytes + ENTRY_STORE_EXTRA);
+    table->entryStore = (char *) ops->allocTable(table, nbytes);
     if (!table->entryStore)
         return false;
     memset(table->entryStore, 0, nbytes);
     METER(memset(&table->stats, 0, sizeof table->stats));
 
 #ifdef DEBUG
-    RECURSION_LEVEL(table) = 0;
+    table->recursionLevel = 0;
 #endif
 
     return true;
 }
 
 /*
  * Compute max and min load numbers (entry counts).
  */
@@ -483,36 +474,35 @@ ChangeTable(PLDHashTable *table, int del
     oldCapacity = 1u << oldLog2;
     newCapacity = 1u << newLog2;
     if (newCapacity > PL_DHASH_MAX_SIZE)
         return false;
     entrySize = table->entrySize;
     if (!SizeOfEntryStore(newCapacity, entrySize, &nbytes))
         return false;   // overflowed
 
-    newEntryStore = (char *) table->ops->allocTable(table,
-                                                    nbytes + ENTRY_STORE_EXTRA);
+    newEntryStore = (char *) table->ops->allocTable(table, nbytes);
     if (!newEntryStore)
         return false;
 
     /* We can't fail from here on, so update table parameters. */
 #ifdef DEBUG
-    recursionLevel = RECURSION_LEVEL(table);
+    recursionLevel = table->recursionLevel;
 #endif
     table->hashShift = PL_DHASH_BITS - newLog2;
     table->removedCount = 0;
     table->generation++;
 
     /* Assign the new entry store to table. */
     memset(newEntryStore, 0, nbytes);
     oldEntryAddr = oldEntryStore = table->entryStore;
     table->entryStore = newEntryStore;
     moveEntry = table->ops->moveEntry;
 #ifdef DEBUG
-    RECURSION_LEVEL(table) = recursionLevel;
+    table->recursionLevel = recursionLevel;
 #endif
 
     /* Copy only live entries, leaving removed ones behind. */
     for (i = 0; i < oldCapacity; i++) {
         oldEntry = (PLDHashEntryHdr *)oldEntryAddr;
         if (ENTRY_IS_LIVE(oldEntry)) {
             oldEntry->keyHash &= ~COLLISION_FLAG;
             newEntry = FindFreeEntry(table, oldEntry->keyHash);
@@ -531,17 +521,17 @@ ChangeTable(PLDHashTable *table, int del
 PLDHashEntryHdr * PL_DHASH_FASTCALL
 PL_DHashTableOperate(PLDHashTable *table, const void *key, PLDHashOperator op)
 {
     PLDHashNumber keyHash;
     PLDHashEntryHdr *entry;
     uint32_t size;
     int deltaLog2;
 
-    MOZ_ASSERT(op == PL_DHASH_LOOKUP || RECURSION_LEVEL(table) == 0);
+    MOZ_ASSERT(op == PL_DHASH_LOOKUP || table->recursionLevel == 0);
     INCREMENT_RECURSION_LEVEL(table);
 
     keyHash = table->ops->hashKey(table, key);
     keyHash *= PL_DHASH_GOLDEN_RATIO;
 
     /* Avoid 0 and 1 hash codes, they indicate free and removed entries. */
     ENSURE_LIVE_KEYHASH(keyHash);
     keyHash &= ~COLLISION_FLAG;
@@ -632,17 +622,17 @@ PL_DHashTableOperate(PLDHashTable *table
     return entry;
 }
 
 void
 PL_DHashTableRawRemove(PLDHashTable *table, PLDHashEntryHdr *entry)
 {
     PLDHashNumber keyHash;      /* load first in case clearEntry goofs it */
 
-    MOZ_ASSERT(RECURSION_LEVEL(table) != IMMUTABLE_RECURSION_LEVEL);
+    MOZ_ASSERT(table->recursionLevel != IMMUTABLE_RECURSION_LEVEL);
 
     NS_ASSERTION(PL_DHASH_ENTRY_IS_LIVE(entry),
                  "PL_DHASH_ENTRY_IS_LIVE(entry)");
     keyHash = entry->keyHash;
     table->ops->clearEntry(table, entry);
     if (keyHash & COLLISION_FLAG) {
         MARK_ENTRY_REMOVED(entry);
         table->removedCount++;
@@ -680,17 +670,17 @@ PL_DHashTableEnumerate(PLDHashTable *tab
                 didRemove = true;
             }
             if (op & PL_DHASH_STOP)
                 break;
         }
         entryAddr += entrySize;
     }
 
-    MOZ_ASSERT(!didRemove || RECURSION_LEVEL(table) == 1);
+    MOZ_ASSERT(!didRemove || table->recursionLevel == 1);
 
     /*
      * Shrink or compress if a quarter or more of all entries are removed, or
      * if the table is underloaded according to the minimum alpha, and is not
      * minimal-size already.  Do this only if we removed above, so non-removing
      * enumerations can count on stable table->entryStore until the next
      * non-lookup-Operate or removing-Enumerate.
      */
@@ -759,17 +749,17 @@ PL_DHashTableSizeOfIncludingThis(const P
            PL_DHashTableSizeOfExcludingThis(table, sizeOfEntryExcludingThis,
                                             mallocSizeOf, arg);
 }
 
 #ifdef DEBUG
 void
 PL_DHashMarkTableImmutable(PLDHashTable *table)
 {
-    RECURSION_LEVEL(table) = IMMUTABLE_RECURSION_LEVEL;
+    table->recursionLevel = IMMUTABLE_RECURSION_LEVEL;
 }
 #endif
 
 #ifdef PL_DHASHMETER
 #include <math.h>
 
 void
 PL_DHashTableDumpMeter(PLDHashTable *table, PLDHashEnumerator dump, FILE *fp)
--- a/xpcom/glue/pldhash.h
+++ b/xpcom/glue/pldhash.h
@@ -181,16 +181,23 @@ PL_DHASH_ENTRY_IS_LIVE(PLDHashEntryHdr* 
  * The moral of this story: there is no one-size-fits-all hash table scheme,
  * but for small table entry size, and assuming entry address stability is not
  * required, double hashing wins.
  */
 struct PLDHashTable {
     const PLDHashTableOps *ops;         /* virtual operations, see below */
     void                *data;          /* ops- and instance-specific data */
     int16_t             hashShift;      /* multiplicative hash shift */
+    /*
+     * |recursionLevel| is only used in debug builds, but is present in opt
+     * builds to avoid binary compatibility problems when mixing DEBUG and
+     * non-DEBUG components.  (Actually, even if it were removed,
+     * sizeof(PLDHashTable) wouldn't change, due to struct padding.)
+     */
+    uint16_t            recursionLevel; /* used to detect unsafe re-entry */
     uint32_t            entrySize;      /* number of bytes in an entry */
     uint32_t            entryCount;     /* number of entries in table */
     uint32_t            removedCount;   /* removed entry sentinels in table */
     uint32_t            generation;     /* entry storage generation number */
     char                *entryStore;    /* entry storage */
 #ifdef PL_DHASHMETER
     struct PLDHashStats {
         uint32_t        searches;       /* total number of table searches */