Make enumeration const unless PL_DHASH_REMOVE is returned by the etor callback; improve several comments (174859, r=waterson, sr=beard).

js/src/jsdhash.c

@@ -586,6 +586,7 @@ JS_DHashTableEnumerate(JSDHashTable *table, JSDHashEnumerator etor, void *arg)
 {
     char *entryAddr, *entryLimit;
     uint32 i, capacity, entrySize;
+    JSBool didRemove;
     JSDHashEntryHdr *entry;
     JSDHashOperator op;
 
@@ -594,6 +595,7 @@ JS_DHashTableEnumerate(JSDHashTable *table, JSDHashEnumerator etor, void *arg)
     capacity = JS_DHASH_TABLE_SIZE(table);
     entryLimit = entryAddr + capacity * entrySize;
     i = 0;
+    didRemove = JS_FALSE;
     while (entryAddr < entryLimit) {
         entry = (JSDHashEntryHdr *)entryAddr;
         if (ENTRY_IS_LIVE(entry)) {
@@ -601,6 +603,7 @@ JS_DHashTableEnumerate(JSDHashTable *table, JSDHashEnumerator etor, void *arg)
             if (op & JS_DHASH_REMOVE) {
                 METER(table->stats.removeEnums++);
                 JS_DHashTableRawRemove(table, entry);
+                didRemove = JS_TRUE;
             }
             if (op & JS_DHASH_STOP)
                 break;
@@ -611,11 +614,14 @@ JS_DHashTableEnumerate(JSDHashTable *table, JSDHashEnumerator etor, void *arg)
     /*
      * Shrink or compress if a quarter or more of all entries are removed, or
      * if the table is underloaded according to the configured minimum alpha,
-     * and is not minimal-size already.
+     * 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 Operate or removing-Enumerate.
      */
-    if (table->removedCount >= capacity >> 2 ||
+    if (didRemove &&
+        (table->removedCount >= capacity >> 2 ||
          (capacity > JS_DHASH_MIN_SIZE &&
-         table->entryCount <= MIN_LOAD(table, capacity))) {
+          table->entryCount <= MIN_LOAD(table, capacity)))) {
         METER(table->stats.enumShrinks++);
         capacity = table->entryCount;
         capacity += capacity >> 1;

js/src/jsdhash.h

@@ -323,6 +323,8 @@ typedef void
  * newly created by the JS_DHASH_ADD call that just succeeded.  If placement
  * new or similar initialization is required, define an initEntry hook.  Of
  * course, the clearEntry hook must zero or null appropriately.
+ *
+ * XXX assumes 0 is null for pointer types.
  */
 struct JSDHashTableOps {
     /* Mandatory hooks.  All implementations must provide these. */
@@ -476,7 +478,7 @@ typedef enum JSDHashOperator {
  * Otherwise, entry->keyHash has been set so that JS_DHASH_ENTRY_IS_BUSY(entry)
  * is true, and it is up to the caller to initialize the key and value parts
  * of the entry sub-type, if they have not been set already (i.e. if entry was
- * not already in the table).
+ * not already in the table, and if the optional initEntry hook was not used).
  *
  * To remove an entry identified by key from table, call:
  *
@@ -522,6 +524,23 @@ JS_DHashTableRawRemove(JSDHashTable *table, JSDHashEntryHdr *entry);
  *
  * If etor calls JS_DHashTableOperate on table, it must return JS_DHASH_STOP;
  * otherwise undefined behavior results.
+ *
+ * If any enumerator returns JS_DHASH_REMOVE, table->entryStore may be shrunk
+ * or compressed after enumeration, but before JS_DHashTableEnumerate returns.
+ * Such an enumerator therefore can't safely set aside entry pointers, but an
+ * enumerator that never returns JS_DHASH_REMOVE can set pointers to entries
+ * aside, e.g., to avoid copying live entries into an array of the entry type.
+ * Copying entry pointers is cheaper, and safe so long as the caller of such a
+ * "stable" Enumerate doesn't use the set-aside pointers after any call either
+ * to PL_DHashTableOperate, or to an "unstable" form of Enumerate, which might
+ * grow or shrink entryStore.
+ *
+ * If your enumerator wants to remove certain entries, but set aside pointers
+ * to other entries that it retains, it can use JS_DHashTableRawRemove on the
+ * entries to be removed, returning JS_DHASH_NEXT to skip them.  Likewise, if
+ * you want to remove entries, but for some reason you do not want entryStore
+ * to be shrunk or compressed, you can call JS_DHashTableRawRemove safely on
+ * the entry being enumerated, rather than returning JS_DHASH_REMOVE.
  */
 typedef JSDHashOperator
 (* JS_DLL_CALLBACK JSDHashEnumerator)(JSDHashTable *table, JSDHashEntryHdr *hdr,

xpcom/ds/pldhash.c

@@ -587,6 +587,7 @@ PL_DHashTableEnumerate(PLDHashTable *table, PLDHashEnumerator etor, void *arg)
 {
     char *entryAddr, *entryLimit;
     PRUint32 i, capacity, entrySize;
+    PRBool didRemove;
     PLDHashEntryHdr *entry;
     PLDHashOperator op;
 
@@ -595,6 +596,7 @@ PL_DHashTableEnumerate(PLDHashTable *table, PLDHashEnumerator etor, void *arg)
     capacity = PL_DHASH_TABLE_SIZE(table);
     entryLimit = entryAddr + capacity * entrySize;
     i = 0;
+    didRemove = PR_FALSE;
     while (entryAddr < entryLimit) {
         entry = (PLDHashEntryHdr *)entryAddr;
         if (ENTRY_IS_LIVE(entry)) {
@@ -602,6 +604,7 @@ PL_DHashTableEnumerate(PLDHashTable *table, PLDHashEnumerator etor, void *arg)
             if (op & PL_DHASH_REMOVE) {
                 METER(table->stats.removeEnums++);
                 PL_DHashTableRawRemove(table, entry);
+                didRemove = PR_TRUE;
             }
             if (op & PL_DHASH_STOP)
                 break;
@@ -612,11 +615,14 @@ PL_DHashTableEnumerate(PLDHashTable *table, PLDHashEnumerator etor, void *arg)
     /*
      * Shrink or compress if a quarter or more of all entries are removed, or
      * if the table is underloaded according to the configured minimum alpha,
-     * and is not minimal-size already.
+     * 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 Operate or removing-Enumerate.
      */
-    if (table->removedCount >= capacity >> 2 ||
+    if (didRemove &&
+        (table->removedCount >= capacity >> 2 ||
          (capacity > PL_DHASH_MIN_SIZE &&
-         table->entryCount <= MIN_LOAD(table, capacity))) {
+          table->entryCount <= MIN_LOAD(table, capacity)))) {
         METER(table->stats.enumShrinks++);
         capacity = table->entryCount;
         capacity += capacity >> 1;

xpcom/ds/pldhash.h

@@ -324,6 +324,8 @@ typedef void
  * newly created by the PL_DHASH_ADD call that just succeeded.  If placement
  * new or similar initialization is required, define an initEntry hook.  Of
  * course, the clearEntry hook must zero or null appropriately.
+ *
+ * XXX assumes 0 is null for pointer types.
  */
 struct PLDHashTableOps {
     /* Mandatory hooks.  All implementations must provide these. */
@@ -477,7 +479,7 @@ typedef enum PLDHashOperator {
  * Otherwise, entry->keyHash has been set so that PL_DHASH_ENTRY_IS_BUSY(entry)
  * is true, and it is up to the caller to initialize the key and value parts
  * of the entry sub-type, if they have not been set already (i.e. if entry was
- * not already in the table).
+ * not already in the table, and if the optional initEntry hook was not used).
  *
  * To remove an entry identified by key from table, call:
  *
@@ -523,6 +525,23 @@ PL_DHashTableRawRemove(PLDHashTable *table, PLDHashEntryHdr *entry);
  *
  * If etor calls PL_DHashTableOperate on table, it must return PL_DHASH_STOP;
  * otherwise undefined behavior results.
+ *
+ * If any enumerator returns PL_DHASH_REMOVE, table->entryStore may be shrunk
+ * or compressed after enumeration, but before PL_DHashTableEnumerate returns.
+ * Such an enumerator therefore can't safely set aside entry pointers, but an
+ * enumerator that never returns PL_DHASH_REMOVE can set pointers to entries
+ * aside, e.g., to avoid copying live entries into an array of the entry type.
+ * Copying entry pointers is cheaper, and safe so long as the caller of such a
+ * "stable" Enumerate doesn't use the set-aside pointers after any call either
+ * to PL_DHashTableOperate, or to an "unstable" form of Enumerate, which might
+ * grow or shrink entryStore.
+ *
+ * If your enumerator wants to remove certain entries, but set aside pointers
+ * to other entries that it retains, it can use PL_DHashTableRawRemove on the
+ * entries to be removed, returning PL_DHASH_NEXT to skip them.  Likewise, if
+ * you want to remove entries, but for some reason you do not want entryStore
+ * to be shrunk or compressed, you can call PL_DHashTableRawRemove safely on
+ * the entry being enumerated, rather than returning PL_DHASH_REMOVE.
  */
 typedef PLDHashOperator
 (* PR_CALLBACK PLDHashEnumerator)(PLDHashTable *table, PLDHashEntryHdr *hdr,