Bug 779183 - GC: Incremental sweeping of atoms table r=billm

* * *
Sweep atoms in a separate slice
* * *
Sweep atoms later in a separate slice
* * *
Code review followup

--HG--
extra : rebase_source : 774ad569a76425637483fcad093f04e30ad6912b

js/src/jit-test/tests/gc/incremental-atom-sweep.js

@@ -0,0 +1,11 @@
+/*
+ * Exercise finding dead atoms in the atom set before they are removed by
+ * SweepAtomState() - in AtomizeInline().
+ */
+o = {};
+for (var i = 0 ; i < 10 ; ++i) {
+    o["atom" + (i + 2)] = 1;
+    delete o["atom" + (i + 1)];
+    o["atom" + i] = 1;
+    gcslice(1000000);
+}

js/src/jsatom.cpp

@@ -182,8 +182,11 @@ js_FinishAtomState(JSRuntime *rt)
     }
 
     FreeOp fop(rt, false, false);
-    for (AtomSet::Range r = state->atoms.all(); !r.empty(); r.popFront())
-        r.front().asPtr()->finalize(&fop);
+    for (AtomSet::Range r = state->atoms.all(); !r.empty(); r.popFront()) {
+        JSAtom *atom = r.front().asPtr();
+        JS_ASSERT(atom);  /* Can never be called during a GC. */
+        atom->finalize(&fop);
+    }
 }
 
 bool
@@ -211,27 +214,20 @@ js::FinishCommonAtoms(JSRuntime *rt)
 }
 
 void
-js::MarkAtomState(JSTracer *trc, bool markAll)
+js::MarkAtomState(JSTracer *trc)
 {
     JSRuntime *rt = trc->runtime;
     JSAtomState *state = &rt->atomState;
 
-    if (markAll) {
-        for (AtomSet::Range r = state->atoms.all(); !r.empty(); r.popFront()) {
-            JSAtom *tmp = r.front().asPtr();
-            MarkStringRoot(trc, &tmp, "locked_atom");
-            JS_ASSERT(tmp == r.front().asPtr());
-        }
-    } else {
-        for (AtomSet::Range r = state->atoms.all(); !r.empty(); r.popFront()) {
-            AtomStateEntry entry = r.front();
-            if (!entry.isTagged())
-                continue;
+    for (AtomSet::Range r = state->atoms.all(); !r.empty(); r.popFront()) {
+        AtomStateEntry entry = r.front();
+        if (!entry.isTagged())
+            continue;
 
-            JSAtom *tmp = entry.asPtr();
-            MarkStringRoot(trc, &tmp, "interned_atom");
-            JS_ASSERT(tmp == entry.asPtr());
-        }
+        JSAtom *tmp = entry.asPtr();
+        JS_ASSERT(tmp);
+        MarkStringRoot(trc, &tmp, "interned_atom");
+        JS_ASSERT(tmp == entry.asPtr());
     }
 }
 
@@ -242,16 +238,13 @@ js::SweepAtomState(JSRuntime *rt)
 
     for (AtomSet::Enum e(state->atoms); !e.empty(); e.popFront()) {
         AtomStateEntry entry = e.front();
-        JSAtom *atom = entry.asPtr();
-        bool isMarked = IsStringMarked(&atom);
+        JSAtom *atom = entry.asPtr();  /* Returns NULL if the atom was umarked. */
 
         /* Pinned or interned key cannot be finalized. */
-        JS_ASSERT_IF(entry.isTagged(), isMarked);
+        JS_ASSERT_IF(entry.isTagged(), atom);
 
-        if (!isMarked)
+        if (!atom)
             e.removeFront();
-        else
-            e.rekeyFront(AtomHasher::Lookup(atom), AtomStateEntry(atom, entry.isTagged()));
     }
 }
 
@@ -441,12 +434,14 @@ js_DumpAtoms(JSContext *cx, FILE *fp)
     unsigned number = 0;
     for (AtomSet::Range r = state->atoms.all(); !r.empty(); r.popFront()) {
         AtomStateEntry entry = r.front();
-        fprintf(fp, "%3u ", number++);
         JSAtom *key = entry.asPtr();
-        FileEscapedString(fp, key, '"');
-        if (entry.isTagged())
-            fputs(" interned", fp);
-        putc('\n', fp);
+        if (key) {
+            fprintf(fp, "%3u ", number++);
+            FileEscapedString(fp, key, '"');
+            if (entry.isTagged())
+                fputs(" interned", fp);
+            putc('\n', fp);
+        }
     }
     putc('\n', fp);
 }

js/src/jsatom.h

@@ -364,7 +364,7 @@ js_FinishAtomState(JSRuntime *rt);
 namespace js {
 
 extern void
-MarkAtomState(JSTracer *trc, bool markAll);
+MarkAtomState(JSTracer *trc);
 
 extern void
 SweepAtomState(JSRuntime *rt);

js/src/jsatominlines.h

@@ -11,6 +11,7 @@
 #include "jsnum.h"
 #include "jsobj.h"
 #include "jsstr.h"
+#include "jscompartment.h"
 
 #include "mozilla/RangedPtr.h"
 #include "vm/String.h"
@@ -20,6 +21,15 @@ js::AtomStateEntry::asPtr() const
 {
     JS_ASSERT(bits != 0);
     JSAtom *atom = reinterpret_cast<JSAtom *>(bits & NO_TAG_MASK);
+    if (atom->compartment()->isGCSweeping() && !atom->isMarked() &&
+        !atom->arenaHeader()->allocatedDuringIncremental)
+    {
+        /*
+         * We are in the sweep phase of garbage collecting, and the atom's data
+         * has been freed but it has not yet been removed from the atom set.
+         */
+        return NULL;
+    }
     JSString::readBarrier(atom);
     return atom;
 }
@@ -143,6 +153,8 @@ inline bool
 AtomHasher::match(const AtomStateEntry &entry, const Lookup &lookup)
 {
     JSAtom *key = entry.asPtr();
+    if (!key)
+        return false;
     if (lookup.atom)
         return lookup.atom == key;
     if (key->length() != lookup.length)

js/src/jscompartment.h

@@ -234,7 +234,7 @@ struct JSCompartment
     }
 
     bool isGCSweeping() {
-        return wasGCStarted() && rt->gcIncrementalState == js::gc::SWEEP;
+        return wasGCStarted() && rt->gcIncrementalState > js::gc::MARK;
     }
 
     size_t                       gcBytes;

js/src/jsgc.cpp

@@ -2502,13 +2502,8 @@ MarkRuntime(JSTracer *trc, bool useSavedRoots = false)
             MarkScriptRoot(trc, &vec[i].script, "scriptAndCountsVector");
     }
 
-    /*
-     * Atoms are not in the cross-compartment map. So if there are any
-     * compartments that are not being collected, we are not allowed to collect
-     * atoms. Otherwise, the non-collected compartments could contain pointers
-     * to atoms that we would miss.
-     */
-    MarkAtomState(trc, rt->gcKeepAtoms || (IS_GC_MARKING_TRACER(trc) && !rt->gcIsFull));
+    if (!IS_GC_MARKING_TRACER(trc) || rt->atomsCompartment->isCollecting())
+        MarkAtomState(trc);
     rt->staticStrings.trace(trc);
 
     for (ContextIter acx(rt); !acx.done(); acx.next())
@@ -3216,6 +3211,17 @@ BeginMarkPhase(JSRuntime *rt, bool isIncremental)
         c->setPreservingCode(ShouldPreserveJITCode(c, currentTime));
     }
 
+    /*
+     * Atoms are not in the cross-compartment map. So if there are any
+     * compartments that are not being collected, we are not allowed to collect
+     * atoms. Otherwise, the non-collected compartments could contain pointers
+     * to atoms that we would miss.
+     */
+    if (rt->atomsCompartment->isCollecting() && (rt->gcKeepAtoms || !rt->gcIsFull)) {
+        rt->atomsCompartment->setCollecting(false);
+        rt->atomsCompartment->setGCStarted(false);
+    }
+
     rt->gcMarker.start(rt);
     JS_ASSERT(!rt->gcMarker.callback);
     JS_ASSERT(IS_GC_MARKING_TRACER(&rt->gcMarker));
@@ -3488,6 +3494,7 @@ BeginSweepPhase(JSRuntime *rt)
         if (!c->isCollecting())
             isFull = false;
     }
+    JS_ASSERT_IF(isFull, rt->gcIsFull);
 
     rt->gcSweepOnBackgroundThread =
         (rt->hasContexts() && rt->gcHelperThread.prepareForBackgroundSweep());
@@ -3508,11 +3515,6 @@ BeginSweepPhase(JSRuntime *rt)
     WeakMapBase::sweepAll(&rt->gcMarker);
     rt->debugScopes->sweep();
 
-    {
-        gcstats::AutoPhase ap2(rt->gcStats, gcstats::PHASE_SWEEP_ATOMS);
-        SweepAtomState(rt);
-    }
-
     /* Collect watch points associated with unreachable objects. */
     WatchpointMap::sweepAll(rt);
 
@@ -3756,7 +3758,7 @@ ResetIncrementalGC(JSRuntime *rt, const char *reason)
     if (rt->gcIncrementalState == NO_INCREMENTAL)
         return;
 
-    if (rt->gcIncrementalState == SWEEP) {
+    if (rt->gcIncrementalState == SWEEP || rt->gcIncrementalState == SWEEP_ATOMS) {
         /* If we've finished marking then sweep to completion here. */
         IncrementalCollectSlice(rt, SliceBudget::Unlimited, gcreason::RESET, GC_NORMAL);
         gcstats::AutoPhase ap(rt->gcStats, gcstats::PHASE_WAIT_BACKGROUND_THREAD);
@@ -3816,13 +3818,15 @@ AutoGCSlice::AutoGCSlice(JSRuntime *rt)
 
 AutoGCSlice::~AutoGCSlice()
 {
+    JS_ASSERT(runtime->gcIncrementalState == NO_INCREMENTAL ||
+              runtime->gcIncrementalState == MARK ||
+              runtime->gcIncrementalState == SWEEP_ATOMS ||
+              runtime->gcIncrementalState == SWEEP);
     for (GCCompartmentsIter c(runtime); !c.done(); c.next()) {
         if (runtime->gcIncrementalState == MARK) {
             c->setNeedsBarrier(true);
             c->arenas.prepareForIncrementalGC(runtime);
         } else {
-            JS_ASSERT(runtime->gcIncrementalState == NO_INCREMENTAL ||
-                      runtime->gcIncrementalState == SWEEP);
             c->setNeedsBarrier(false);
         }
     }
@@ -3944,27 +3948,49 @@ IncrementalCollectSlice(JSRuntime *rt,
         }
 
         EndMarkPhase(rt, isIncremental);
-
-        rt->gcIncrementalState = SWEEP;
-
-        /*
-         * This runs to completion, but we don't continue if the budget is
-         * now exhasted.
-         */
         BeginSweepPhase(rt);
-        if (sliceBudget.isOverBudget())
-            break;
+
+        rt->gcIncrementalState = SWEEP_ATOMS;
 
         /*
-         * Always yield here when running in incremental multi-slice zeal
-         * mode, so RunDebugGC can reset the slice buget.
+         * We yield here if we are doing an incremental GC and one of the
+         * following holds:
+         *
+         *  - we have exhausted our budget
+         *
+         *  - we need to sweep the atoms (this is not incremental and so gets its
+         *    own slice)
+         *
+         *  - we are in incremental multi-slice zeal mode, to allow RunDebugGC
+         *    to reset the slice buget.
          */
-        if (budget != SliceBudget::Unlimited && zeal == ZealIncrementalMultipleSlices)
+        if (budget != SliceBudget::Unlimited &&
+            (sliceBudget.isOverBudget() ||
+             rt->atomsCompartment->isCollecting() ||
+             zeal == ZealIncrementalMultipleSlices))
+        {
             break;
+        }
 
         /* fall through */
       }
 
+      case SWEEP_ATOMS:
+
+        rt->gcIncrementalState = SWEEP;
+
+        if (rt->atomsCompartment->isCollecting()) {
+            gcstats::AutoPhase ap(rt->gcStats, gcstats::PHASE_SWEEP_ATOMS);
+            uint32_t initialCount = rt->atomState.atoms.count();
+            SweepAtomState(rt);
+            sliceBudget.step(initialCount);
+
+            if (sliceBudget.isOverBudget())
+                break;
+        }
+
+        /* fall through */
+
       case SWEEP: {
 #ifdef DEBUG
         for (CompartmentsIter c(rt); !c.done(); c.next())
@@ -4519,7 +4545,7 @@ RunDebugGC(JSContext *cx)
          * phase.
          */
         if (type == ZealIncrementalMultipleSlices &&
-            initialState == MARK && rt->gcIncrementalState == SWEEP)
+            initialState == MARK && rt->gcIncrementalState == SWEEP_ATOMS)
         {
             rt->gcIncrementalLimit = rt->gcZealFrequency / 2;
         }

js/src/jsgc.h

@@ -49,6 +49,7 @@ enum State {
     NO_INCREMENTAL,
     MARK_ROOTS,
     MARK,
+    SWEEP_ATOMS,
     SWEEP,
     INVALID
 };