Bug 1213977 - Don't reset an ongoing incremental GC if AutoKeepAtoms is set r=sfink
authorJon Coppeard <jcoppeard@mozilla.com>
Tue, 06 Dec 2016 17:25:02 -1000
changeset 325324 40c8129cffbf82cd89b4e22caf52aa042b5573e4
parent 325323 c30011abb0f2357132b5098c2d3e2ce35b880a12
child 325325 ca7fd1a24055581dbfcd48f2cd71b2abbbad33f3
push id24
push usermaklebus@msu.edu
push dateTue, 20 Dec 2016 03:11:33 +0000
reviewerssfink
bugs1213977
milestone53.0a1
Bug 1213977 - Don't reset an ongoing incremental GC if AutoKeepAtoms is set r=sfink
js/src/gc/Verifier.cpp
js/src/jsgc.cpp
js/src/jsgc.h
--- a/js/src/gc/Verifier.cpp
+++ b/js/src/gc/Verifier.cpp
@@ -170,17 +170,17 @@ NextNode(VerifyNode* node)
 }
 
 void
 gc::GCRuntime::startVerifyPreBarriers()
 {
     if (verifyPreData || isIncrementalGCInProgress())
         return;
 
-    if (IsIncrementalGCUnsafe(rt) != AbortReason::None)
+    if (IsIncrementalGCUnsafe(rt) != AbortReason::None || rt->keepAtoms())
         return;
 
     number++;
 
     VerifyPreTracer* trc = js_new<VerifyPreTracer>(rt);
     if (!trc)
         return;
 
@@ -337,17 +337,17 @@ gc::GCRuntime::endVerifyPreBarriers()
      * been discarded.
      */
     MOZ_ASSERT(trc->number == number);
     number++;
 
     verifyPreData = nullptr;
     incrementalState = State::NotActive;
 
-    if (!compartmentCreated && IsIncrementalGCUnsafe(rt) == AbortReason::None) {
+    if (!compartmentCreated && IsIncrementalGCUnsafe(rt) == AbortReason::None && !rt->keepAtoms()) {
         CheckEdgeTracer cetrc(rt);
 
         /* Start after the roots. */
         VerifyNode* node = NextNode(trc->root);
         while ((char*)node < trc->edgeptr) {
             cetrc.node = node;
             js::TraceChildren(&cetrc, node->thing, node->kind);
 
--- a/js/src/jsgc.cpp
+++ b/js/src/jsgc.cpp
@@ -3796,24 +3796,32 @@ GCRuntime::beginMarkPhase(JS::gcreason::
     }
 
     if (!rt->gc.cleanUpEverything) {
         if (JSCompartment* comp = jit::TopmostIonActivationCompartment(rt))
             comp->zone()->setPreservingCode(true);
     }
 
     /*
-     * Atoms are not in the cross-compartment map. So if there are any
-     * zones that are not being collected, we are not allowed to collect
-     * atoms. Otherwise, the non-collected zones could contain pointers
-     * to atoms that we would miss.
+     * Atoms are not in the cross-compartment map. If there are any zones that
+     * are not being collected then we cannot collect the atoms zone, otherwise
+     * the non-collected zones could contain pointers to atoms that we would
+     * miss.
      *
-     * keepAtoms() will only change on the main thread, which we are currently
-     * on. If the value of keepAtoms() changes between GC slices, then we'll
-     * cancel the incremental GC. See IsIncrementalGCSafe.
+     * If keepAtoms() is true then either an instance of AutoKeepAtoms is
+     * currently on the stack or parsing is currently happening on another
+     * thread. In either case we don't have information about which atoms are
+     * roots, so we must skip collecting atoms.
+     *
+     * Note that only affects the first slice of an incremental GC since root
+     * marking is completed before we return to the mutator.
+     *
+     * Off-main-thread parsing is inhibited after the start of GC which prevents
+     * races between creating atoms during parsing and sweeping atoms on the
+     * main thread.
      */
     if (isFull && !rt->keepAtoms()) {
         Zone* atomsZone = rt->atomsCompartment(lock)->zone();
         if (atomsZone->isGCScheduled()) {
             MOZ_ASSERT(!atomsZone->isCollecting());
             atomsZone->setGCState(Zone::Mark);
             any = true;
         }
@@ -5988,19 +5996,16 @@ GCRuntime::incrementalCollectSlice(Slice
     }
 }
 
 gc::AbortReason
 gc::IsIncrementalGCUnsafe(JSRuntime* rt)
 {
     MOZ_ASSERT(!rt->mainThread.suppressGC);
 
-    if (rt->keepAtoms())
-        return gc::AbortReason::KeepAtomsSet;
-
     if (!rt->gc.isIncrementalGCAllowed())
         return gc::AbortReason::IncrementalDisabled;
 
     return gc::AbortReason::None;
 }
 
 void
 GCRuntime::budgetIncrementalGC(JS::gcreason::Reason reason, SliceBudget& budget,
@@ -6162,16 +6167,20 @@ GCRuntime::gcCycle(bool nonincrementalBy
         // avoid taking the GC lock when manipulating the chunks during the GC.
         // The background alloc task can run between slices, so we must wait
         // for it at the start of every slice.
         allocTask.cancel(GCParallelTask::CancelAndWait);
     }
 
     State prevState = incrementalState;
 
+    // We don't allow off-main-thread parsing to start while we're doing an
+    // incremental GC.
+    MOZ_ASSERT_IF(rt->activeGCInAtomsZone(), !rt->exclusiveThreadsPresent());
+
     if (nonincrementalByAPI) {
         // Reset any in progress incremental GC if this was triggered via the
         // API. This isn't required for correctness, but sometimes during tests
         // the caller expects this GC to collect certain objects, and we need
         // to make sure to collect everything possible.
         if (reason != JS::gcreason::ALLOC_TRIGGER)
             resetIncrementalGC(gc::AbortReason::NonIncrementalRequested, session.lock);
 
--- a/js/src/jsgc.h
+++ b/js/src/jsgc.h
@@ -50,17 +50,17 @@ enum class State {
 #undef MAKE_STATE
 };
 
 // Reasons we reset an ongoing incremental GC or perform a non-incremental GC.
 #define GC_ABORT_REASONS(D) \
     D(None) \
     D(NonIncrementalRequested) \
     D(AbortRequested) \
-    D(KeepAtomsSet) \
+    D(Unused1) \
     D(IncrementalDisabled) \
     D(ModeChange) \
     D(MallocBytesTrigger) \
     D(GCBytesTrigger) \
     D(ZoneChange) \
     D(CompartmentRevived)
 enum class AbortReason {
 #define MAKE_REASON(name) name,