bug 714562 - fixing races between the background arena decommit and with arena allocation. r=wmccloskey

--HG--
extra : rebase_source : 6f24d608f99b921c0454a38952211e6657dc6f3e

js/src/jsgc.cpp

@@ -629,17 +629,23 @@ GetAvailableChunkList(JSCompartment *comp)
 inline void
 Chunk::addToAvailableList(JSCompartment *comp)
 {
-    Chunk **listHeadp = GetAvailableChunkList(comp);
+    insertToAvailableList(GetAvailableChunkList(comp));
+}
+
+inline void
+Chunk::insertToAvailableList(Chunk **insertPoint)
+{
+    JS_ASSERT(hasAvailableArenas());
     JS_ASSERT(!info.prevp);
     JS_ASSERT(!info.next);
-    info.prevp = listHeadp;
-    Chunk *head = *listHeadp;
-    if (head) {
-        JS_ASSERT(head->info.prevp == listHeadp);
-        head->info.prevp = &info.next;
+    info.prevp = insertPoint;
+    Chunk *insertBefore = *insertPoint;
+    if (insertBefore) {
+        JS_ASSERT(insertBefore->info.prevp == insertPoint);
+        insertBefore->info.prevp = &info.next;
     }
-    info.next = head;
-    *listHeadp = this;
+    info.next = insertBefore;
+    *insertPoint = this;
 }
 
 inline void
@@ -2243,6 +2249,22 @@ DecommitArenasFromAvailableList(JSRuntime *rt, Chunk **availableListHeadp)
      *
      * We decommit from the tail of the list to minimize interference with the
      * main thread that may start to allocate things at this point.
+     *
+     * The arena that is been decommitted outside the GC lock must not be
+     * available for allocations either via the free list or via the
+     * decommittedArenas bitmap. For that we just fetch the arena from the
+     * free list before the decommit pretending as it was allocated. If this
+     * arena also is the single free arena in the chunk, then we must remove
+     * from the available list before we release the lock so the allocation
+     * thread would not see chunks with no free arenas on the available list.
+     *
+     * After we retake the lock, we mark the arena as free and decommitted if
+     * the decommit was successful. We must also add the chunk back to the
+     * available list if we removed it previously or when the main thread
+     * have allocated all remaining free arenas in the chunk.
+     *
+     * We also must make sure that the aheader is not accessed again after we
+     * decommit the arena.
      */
     JS_ASSERT(chunk->info.prevp == availableListHeadp);
     while (Chunk *next = chunk->info.next) {
@@ -2252,27 +2274,23 @@ DecommitArenasFromAvailableList(JSRuntime *rt, Chunk **availableListHeadp)
 
     for (;;) {
         while (chunk->info.numArenasFreeCommitted != 0) {
-            /*
-             * The arena that is been decommitted outside the GC lock must not
-             * be available for allocations either via the free list or via
-             * the decommittedArenas bitmap. For that we just fetch the arena
-             * from the free list before the decommit and then mark it as free
-             * and decommitted when we retake the GC lock. However, if this
-             * arena also is the single free arena in the chunk, then during
-             * the decommit the allocation thread may find the chunk as
-             * present on the available list yet having no arenas to allocate.
-             * To avoid complications in this case we decommit inside the lock.
-             *
-             * We also must make sure that the aheader is not accessed again
-             * after we decommit the arena.
-             */
             ArenaHeader *aheader = chunk->fetchNextFreeArena(rt);
+
+            Chunk **savedPrevp = chunk->info.prevp;
+            if (!chunk->hasAvailableArenas())
+                chunk->removeFromAvailableList();
+
             size_t arenaIndex = Chunk::arenaIndex(aheader->arenaAddress());
             bool ok;
             {
-                Maybe<AutoUnlockGC> maybayUnlock;
-                if (chunk->hasAvailableArenas())
-                    maybayUnlock.construct(rt);
+                /*
+                 * If the main thread waits for the decommit to finish, skip
+                 * potentially expensive unlock/lock pair on the contested
+                 * lock.
+                 */
+                Maybe<AutoUnlockGC> maybeUnlock;
+                if (!rt->gcRunning)
+                    maybeUnlock.construct(rt);
                 ok = DecommitMemory(aheader->getArena(), ArenaSize);
             }
 
@@ -2282,6 +2300,26 @@ DecommitArenasFromAvailableList(JSRuntime *rt, Chunk **availableListHeadp)
             } else {
                 chunk->addArenaToFreeList(rt, aheader);
             }
+            JS_ASSERT(chunk->hasAvailableArenas());
+            JS_ASSERT(!chunk->unused());
+            if (chunk->info.numArenasFree == 1) {
+                /*
+                 * Put the chunk back to the available list either at the
+                 * point where it was before to preserve the available list
+                 * that we enumerate, or, when the allocation thread has fully
+                 * used all the previous chunks, at the beginning of the
+                 * available list.
+                 */
+                Chunk **insertPoint = savedPrevp;
+                if (savedPrevp != availableListHeadp) {
+                    Chunk *prev = Chunk::fromPointerToNext(savedPrevp);
+                    if (!prev->hasAvailableArenas())
+                        insertPoint = availableListHeadp;
+                }
+                chunk->insertToAvailableList(insertPoint);
+            } else {
+                JS_ASSERT(chunk->info.prevp);
+            }
 
             if (rt->gcChunkAllocationSinceLastGC) {
                 /*
@@ -2293,8 +2331,8 @@ DecommitArenasFromAvailableList(JSRuntime *rt, Chunk **availableListHeadp)
         }
 
         /*
-         * prevp becomes null when the allocator thread consumed all chunks from
-         * the available list.
+         * chunk->info.prevp becomes null when the allocator thread consumed
+         * all chunks from the available list.
          */
         JS_ASSERT_IF(chunk->info.prevp, *chunk->info.prevp == chunk);
         if (chunk->info.prevp == availableListHeadp || !chunk->info.prevp)

js/src/jsgc.h

@@ -739,6 +739,7 @@ struct Chunk {
     }
 
     inline void addToAvailableList(JSCompartment *compartment);
+    inline void insertToAvailableList(Chunk **insertPoint);
     inline void removeFromAvailableList();
 
     ArenaHeader *allocateArena(JSCompartment *comp, AllocKind kind);
@@ -760,9 +761,14 @@ struct Chunk {
      */
     Chunk *getPrevious() {
         JS_ASSERT(info.prevp);
-        uintptr_t prevAddress = reinterpret_cast<uintptr_t>(info.prevp);
-        JS_ASSERT((prevAddress & ChunkMask) == offsetof(Chunk, info.next));
-        return reinterpret_cast<Chunk *>(prevAddress - offsetof(Chunk, info.next));
+        return fromPointerToNext(info.prevp);
+    }
+
+    /* Get the chunk from a pointer to its info.next field. */
+    static Chunk *fromPointerToNext(Chunk **nextFieldPtr) {
+        uintptr_t addr = reinterpret_cast<uintptr_t>(nextFieldPtr);
+        JS_ASSERT((addr & ChunkMask) == offsetof(Chunk, info.next));
+        return reinterpret_cast<Chunk *>(addr - offsetof(Chunk, info.next));
     }
 
   private: