Bug 665007 - Remove the under-utilized background free path; r=jonco
authorTerrence Cole <terrence@mozilla.com>
Tue, 05 Aug 2014 14:06:34 -0700
changeset 198058 eab45b9fa85134807ffadfd95c34956d68cd2344
parent 198057 e4750e8e7a21c888d0623935a90efdc1fc4fb06e
child 198059 2af2a0050c300bbb82d06135b485b6b7d29ef789
push id1
push userroot
push dateMon, 20 Oct 2014 17:29:22 +0000
reviewersjonco
bugs665007
milestone34.0a1
Bug 665007 - Remove the under-utilized background free path; r=jonco
js/src/gc/GCRuntime.h
js/src/jsgc.cpp
js/src/jsgc.h
js/src/vm/Runtime.cpp
js/src/vm/Runtime.h
--- a/js/src/gc/GCRuntime.h
+++ b/js/src/gc/GCRuntime.h
@@ -308,17 +308,16 @@ class GCRuntime
     // Internal public interface
     js::gc::State state() { return incrementalState; }
     void recordNativeStackTop();
     void notifyRequestEnd() { conservativeGC.updateForRequestEnd(); }
     bool isBackgroundSweeping() { return helperState.isBackgroundSweeping(); }
     void waitBackgroundSweepEnd() { helperState.waitBackgroundSweepEnd(); }
     void waitBackgroundSweepOrAllocEnd() { helperState.waitBackgroundSweepOrAllocEnd(); }
     void startBackgroundAllocationIfIdle() { helperState.startBackgroundAllocationIfIdle(); }
-    void freeLater(void *p) { helperState.freeLater(p); }
 
 #ifdef DEBUG
 
     bool onBackgroundThread() { return helperState.onBackgroundThread(); }
 
     bool currentThreadOwnsGCLock() {
         return lockOwner == PR_GetCurrentThread();
     }
--- a/js/src/jsgc.cpp
+++ b/js/src/jsgc.cpp
@@ -1964,17 +1964,17 @@ ArenaLists::wipeDuringParallelExecution(
     for (unsigned i = 0; i < FINALIZE_LAST; i++) {
         AllocKind thingKind = AllocKind(i);
         if (!IsBackgroundFinalized(thingKind) && !arenaLists[thingKind].isEmpty())
             return;
     }
 
     // Finalize all background finalizable objects immediately and
     // return the (now empty) arenas back to arena list.
-    FreeOp fop(rt, false);
+    FreeOp fop(rt);
     for (unsigned i = 0; i < FINALIZE_OBJECT_LAST; i++) {
         AllocKind thingKind = AllocKind(i);
 
         if (!IsBackgroundFinalized(thingKind))
             continue;
 
         if (!arenaLists[i].isEmpty()) {
             purge(thingKind);
@@ -2605,17 +2605,17 @@ GCRuntime::expireChunksAndArenas(bool sh
 
 void
 GCRuntime::sweepBackgroundThings(bool onBackgroundThread)
 {
     /*
      * We must finalize in the correct order, see comments in
      * finalizeObjects.
      */
-    FreeOp fop(rt, false);
+    FreeOp fop(rt);
     for (int phase = 0 ; phase < BackgroundPhaseCount ; ++phase) {
         for (Zone *zone = sweepingZones; zone; zone = zone->gcNextGraphNode) {
             for (int index = 0 ; index < BackgroundPhaseLength[phase] ; ++index) {
                 AllocKind kind = BackgroundPhases[phase][index];
                 ArenaHeader *arenas = zone->allocator.arenas.arenaListsToSweep[kind];
                 if (arenas)
                     ArenaLists::backgroundFinalize(&fop, arenas, onBackgroundThread);
             }
@@ -2842,58 +2842,26 @@ GCHelperState::waitBackgroundSweepOrAllo
 /* Must be called with the GC lock taken. */
 inline void
 GCHelperState::startBackgroundAllocationIfIdle()
 {
     if (state_ == IDLE)
         startBackgroundThread(ALLOCATING);
 }
 
-void
-GCHelperState::replenishAndFreeLater(void *ptr)
-{
-    JS_ASSERT(freeCursor == freeCursorEnd);
-    do {
-        if (freeCursor && !freeVector.append(freeCursorEnd - FREE_ARRAY_LENGTH))
-            break;
-        freeCursor = (void **) js_malloc(FREE_ARRAY_SIZE);
-        if (!freeCursor) {
-            freeCursorEnd = nullptr;
-            break;
-        }
-        freeCursorEnd = freeCursor + FREE_ARRAY_LENGTH;
-        *freeCursor++ = ptr;
-        return;
-    } while (false);
-    js_free(ptr);
-}
-
 /* Must be called with the GC lock taken. */
 void
 GCHelperState::doSweep()
 {
     if (sweepFlag) {
         sweepFlag = false;
         AutoUnlockGC unlock(rt);
 
         rt->gc.sweepBackgroundThings(true);
 
-        if (freeCursor) {
-            void **array = freeCursorEnd - FREE_ARRAY_LENGTH;
-            freeElementsAndArray(array, freeCursor);
-            freeCursor = freeCursorEnd = nullptr;
-        } else {
-            JS_ASSERT(!freeCursorEnd);
-        }
-        for (void ***iter = freeVector.begin(); iter != freeVector.end(); ++iter) {
-            void **array = *iter;
-            freeElementsAndArray(array, array + FREE_ARRAY_LENGTH);
-        }
-        freeVector.resize(0);
-
         rt->freeLifoAlloc.freeAll();
     }
 
     bool shrinking = shrinkFlag;
     rt->gc.expireChunksAndArenas(shrinking);
 
     /*
      * The main thread may have called ShrinkGCBuffers while
@@ -4134,17 +4102,17 @@ GCRuntime::beginSweepingZoneGroup()
         if (rt->sweepZoneCallback)
             rt->sweepZoneCallback(zone);
 
         zone->gcLastZoneGroupIndex = zoneGroupIndex;
     }
 
     validateIncrementalMarking();
 
-    FreeOp fop(rt, sweepOnBackgroundThread);
+    FreeOp fop(rt);
 
     {
         gcstats::AutoPhase ap(stats, gcstats::PHASE_FINALIZE_START);
         for (Callback<JSFinalizeCallback> *p = rt->gc.finalizeCallbacks.begin();
              p < rt->gc.finalizeCallbacks.end(); p++)
         {
             p->op(&fop, JSFINALIZE_GROUP_START, !isFull /* unused */, p->data);
         }
@@ -4332,17 +4300,17 @@ GCRuntime::drainMarkStack(SliceBudget &s
     gcstats::AutoPhase ap(stats, phase);
     return marker.drainMarkStack(sliceBudget);
 }
 
 bool
 GCRuntime::sweepPhase(SliceBudget &sliceBudget)
 {
     gcstats::AutoPhase ap(stats, gcstats::PHASE_SWEEP);
-    FreeOp fop(rt, sweepOnBackgroundThread);
+    FreeOp fop(rt);
 
     bool finished = drainMarkStack(sliceBudget, gcstats::PHASE_SWEEP_MARK);
     if (!finished)
         return false;
 
     for (;;) {
         /* Finalize foreground finalized things. */
         for (; finalizePhase < FinalizePhaseCount ; ++finalizePhase) {
@@ -4401,17 +4369,17 @@ GCRuntime::sweepPhase(SliceBudget &slice
         beginSweepingZoneGroup();
     }
 }
 
 void
 GCRuntime::endSweepPhase(JSGCInvocationKind gckind, bool lastGC)
 {
     gcstats::AutoPhase ap(stats, gcstats::PHASE_SWEEP);
-    FreeOp fop(rt, sweepOnBackgroundThread);
+    FreeOp fop(rt);
 
     JS_ASSERT_IF(lastGC, !sweepOnBackgroundThread);
 
     JS_ASSERT(marker.isDrained());
     marker.stop();
 
     /*
      * Recalculate whether GC was full or not as this may have changed due to
--- a/js/src/jsgc.h
+++ b/js/src/jsgc.h
@@ -1085,29 +1085,16 @@ class GCHelperState
 {
     enum State {
         IDLE,
         SWEEPING,
         ALLOCATING,
         CANCEL_ALLOCATION
     };
 
-    /*
-     * During the finalization we do not free immediately. Rather we add the
-     * corresponding pointers to a buffer which we later release on a
-     * separated thread.
-     *
-     * The buffer is implemented as a vector of 64K arrays of pointers, not as
-     * a simple vector, to avoid realloc calls during the vector growth and to
-     * not bloat the binary size of the inlined freeLater method. Any OOM
-     * during buffer growth results in the pointer being freed immediately.
-     */
-    static const size_t FREE_ARRAY_SIZE = size_t(1) << 16;
-    static const size_t FREE_ARRAY_LENGTH = FREE_ARRAY_SIZE / sizeof(void *);
-
     // Associated runtime.
     JSRuntime *const rt;
 
     // Condvar for notifying the main thread when work has finished. This is
     // associated with the runtime's GC lock --- the worker thread state
     // condvars can't be used here due to lock ordering issues.
     PRCondVar *done;
 
@@ -1121,27 +1108,20 @@ class GCHelperState
     void waitForBackgroundThread();
 
     State state();
     void setState(State state);
 
     bool              sweepFlag;
     bool              shrinkFlag;
 
-    Vector<void **, 16, js::SystemAllocPolicy> freeVector;
-    void            **freeCursor;
-    void            **freeCursorEnd;
-
     bool              backgroundAllocation;
 
     friend class js::gc::ArenaLists;
 
-    void
-    replenishAndFreeLater(void *ptr);
-
     static void freeElementsAndArray(void **array, void **end) {
         JS_ASSERT(array <= end);
         for (void **p = array; p != end; ++p)
             js_free(*p);
         js_free(array);
     }
 
     /* Must be called with the GC lock taken. */
@@ -1150,18 +1130,16 @@ class GCHelperState
   public:
     explicit GCHelperState(JSRuntime *rt)
       : rt(rt),
         done(nullptr),
         state_(IDLE),
         thread(nullptr),
         sweepFlag(false),
         shrinkFlag(false),
-        freeCursor(nullptr),
-        freeCursorEnd(nullptr),
         backgroundAllocation(true)
     { }
 
     bool init();
     void finish();
 
     void work();
 
@@ -1197,24 +1175,16 @@ class GCHelperState
     bool isBackgroundSweeping() const {
         return state_ == SWEEPING;
     }
 
     bool shouldShrink() const {
         JS_ASSERT(isBackgroundSweeping());
         return shrinkFlag;
     }
-
-    void freeLater(void *ptr) {
-        JS_ASSERT(!isBackgroundSweeping());
-        if (freeCursor != freeCursorEnd)
-            *freeCursor++ = ptr;
-        else
-            replenishAndFreeLater(ptr);
-    }
 };
 
 struct GCChunkHasher {
     typedef gc::Chunk *Lookup;
 
     /*
      * Strip zeros for better distribution after multiplying by the golden
      * ratio.
--- a/js/src/vm/Runtime.cpp
+++ b/js/src/vm/Runtime.cpp
@@ -175,17 +175,17 @@ JSRuntime::JSRuntime(JSRuntime *parentRu
     debugMode(false),
     spsProfiler(thisFromCtor()),
     profilingScripts(false),
     hadOutOfMemory(false),
     haveCreatedContext(false),
     data(nullptr),
     signalHandlersInstalled_(false),
     canUseSignalHandlers_(false),
-    defaultFreeOp_(thisFromCtor(), false),
+    defaultFreeOp_(thisFromCtor()),
     debuggerMutations(0),
     securityCallbacks(const_cast<JSSecurityCallbacks *>(&NullSecurityCallbacks)),
     DOMcallbacks(nullptr),
     destroyPrincipals(nullptr),
     structuredCloneCallbacks(nullptr),
     telemetryCallback(nullptr),
     propertyRemovals(0),
 #if !EXPOSE_INTL_API
--- a/js/src/vm/Runtime.h
+++ b/js/src/vm/Runtime.h
@@ -376,53 +376,36 @@ struct RegExpTestCache
 
 /*
  * A FreeOp can do one thing: free memory. For convenience, it has delete_
  * convenience methods that also call destructors.
  *
  * FreeOp is passed to finalizers and other sweep-phase hooks so that we do not
  * need to pass a JSContext to those hooks.
  */
-class FreeOp : public JSFreeOp {
-    bool        shouldFreeLater_;
-
+class FreeOp : public JSFreeOp
+{
   public:
     static FreeOp *get(JSFreeOp *fop) {
         return static_cast<FreeOp *>(fop);
     }
 
-    FreeOp(JSRuntime *rt, bool shouldFreeLater)
-      : JSFreeOp(rt),
-        shouldFreeLater_(shouldFreeLater)
-    {
-    }
-
-    bool shouldFreeLater() const {
-        return shouldFreeLater_;
-    }
+    FreeOp(JSRuntime *rt)
+      : JSFreeOp(rt)
+    {}
 
     inline void free_(void *p);
 
     template <class T>
     inline void delete_(T *p) {
         if (p) {
             p->~T();
             free_(p);
         }
     }
-
-    static void staticAsserts() {
-        /*
-         * Check that JSFreeOp is the first base class for FreeOp and we can
-         * reinterpret a pointer to JSFreeOp as a pointer to FreeOp without
-         * any offset adjustments. JSClass::finalize <-> Class::finalize depends
-         * on this.
-         */
-        JS_STATIC_ASSERT(offsetof(FreeOp, shouldFreeLater_) == sizeof(JSFreeOp));
-    }
 };
 
 } /* namespace js */
 
 namespace JS {
 struct RuntimeSizes;
 }
 
@@ -1454,20 +1437,16 @@ static inline bool
 VersionIsKnown(JSVersion version)
 {
     return VersionNumber(version) != JSVERSION_UNKNOWN;
 }
 
 inline void
 FreeOp::free_(void *p)
 {
-    if (shouldFreeLater()) {
-        runtime()->gc.freeLater(p);
-        return;
-    }
     js_free(p);
 }
 
 class AutoLockGC
 {
   public:
     explicit AutoLockGC(JSRuntime *rt = nullptr
                         MOZ_GUARD_OBJECT_NOTIFIER_PARAM)