790338 - GC: Sweep compartments in groups

Status: RESOLVED FIXED

(Core :: JavaScript Engine, enhancement)

Description

[Jon Coppeard (:jonco)]

The sweeping phase of GC involves a bunch of processing that has to happen for all collecting compartments as they transition from being marked to being swept, before returning to the caller.  This work cannot be performed incrementally and therefore can lead to noticeable pauses when many compartments are being collected.

One way round this is to not transition all compartments from marking to sweeping in one go.  There are however dependencies between compartments that mean there are constraints on the order compartments can be swept in, and that some compartments must be swept at the same time.

Specifically, if there pointer from an unmarked object in compartment A to unmarked object in compartment B then compartment B must not be swept before A.  If the these pointers are considered as edges in a graph, then the components that must be swept at the same time are the strongly connected components of the graph.

Comment 1

[Jon Coppeard (:jonco)]

Attachment: Part 1 - find strongly connected components

Comment 2

[Jon Coppeard (:jonco)]

Attachment: Part 2 - add Is*AboutToBeFinalized functions

Comment 3

[Jon Coppeard (:jonco)]

Attachment: Part 3 - split up XPC finalization callback

Comment 4

[Jon Coppeard (:jonco)]

Attachment: Part 4 - sweep compartments in groups

Comment 5

[Jon Coppeard (:jonco)]

Attachment: Part 1 - find strongly connected components

Comment 6

[Jon Coppeard (:jonco)]

Attachment: Part 2 - add Is*AboutToBeFinalized functions

Comment 7

[Jon Coppeard (:jonco)]

Attachment: Part 3 - split up XPC finalization callback

Comment 8

[Jon Coppeard (:jonco)]

Attachment: Part 4 - sweep compartments in groups

Comment 9

[Jon Coppeard (:jonco)]

Attachment: Part 1 - find strongly connected components

Comment 10

[Jon Coppeard (:jonco)]

Attachment: Part 2 - add Is*AboutToBeFinalized functions

Comment 11

[Jon Coppeard (:jonco)]

Attachment: Part 3 - split up XPC finalization callback

Comment 12

[Jon Coppeard (:jonco)]

Attachment: Part 4 - sweep compartments in groups

Comment 13

[Jon Coppeard (:jonco)]

Attachment: Part 1 - find strongly connected components

Comment 14

[Jon Coppeard (:jonco)]

Attachment: Part 4 - sweep compartments in groups

Comment 16

[Jon Coppeard (:jonco)]

Attachment: Part 4 - sweep compartments in groups

Comment 17

[Jon Coppeard (:jonco)]

Attachment: Part 5 - attempt to fix gray marking issues

Comment 18

[Jon Coppeard (:jonco)]

Attachment: Part 1 - find strongly connected components

Comment 19

[Jon Coppeard (:jonco)]

Attachment: Part 2 - add Is*AboutToBeFinalized functions

Comment 20

[Jon Coppeard (:jonco)]

Attachment: Part 3 - split up XPC finalization callback

Comment 21

[Jon Coppeard (:jonco)]

Attachment: Part 4 - sweep compartments in groups

Comment 22

[Jon Coppeard (:jonco)]

Attachment: Part 5 - attempt to fix gray marking issues

Comment 23

[Jon Coppeard (:jonco)]

Attachment: Part 1 - find strongly connected components

Comment 24

[Jon Coppeard (:jonco)]

Attachment: Part 2 - add Is*AboutToBeFinalized functions

Comment 25

[Jon Coppeard (:jonco)]

Attachment: Part 3 - split up XPC finalization callback

Comment 26

[Jon Coppeard (:jonco)]

Attachment: Part 4 - sweep compartments in groups

Comment 27

[Jon Coppeard (:jonco)]

Attachment: Part 5 - attempt to fix gray marking issues

Comment 28

[Jon Coppeard (:jonco)]

Attachment: Part 1 - find strongly connected components

Comment 29

[Jon Coppeard (:jonco)]

Attachment: Part 2 - add Is*AboutToBeFinalized functions

Comment 30

[Jon Coppeard (:jonco)]

Attachment: Part 3 - split up XPC finalization callback

Comment 31

[Jon Coppeard (:jonco)]

Attachment: Part 4 - sweep compartments in groups

Comment 32

[Jon Coppeard (:jonco)]

Attachment: Part 5 - Fix gray marking issues

Comment 33

[Jon Coppeard (:jonco)]

Attachment: Part 6 - sweep debuggers and their debugees in the same group

Comment 34

[Jon Coppeard (:jonco)]

Attachment: Part 7 - Update GC stats

Comment 35

[Jon Coppeard (:jonco)]

Attachment: Part 8 - Track live arraybufs per compartment

Comment 36

[Jon Coppeard (:jonco)]

Attachment: Part 9 - Refactor DebugScopes to be tracked per compartment

Comment 37

[Jon Coppeard (:jonco)]

Attachment: Part 10 - Track WeakMaps per compartment

Comment 38

[Jon Coppeard (:jonco)]

Attachment: Part 11 - Improve finding of Debugger edges

Comment 39

[Jon Coppeard (:jonco)]

Attachment: Part 12 - Mark Watchpoints per compartment

Comment 40

[Jon Coppeard (:jonco)]

Attachment: Part 13 - Handle nuked wrappers

Comment 41

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

Comment on attachment 683176 [details] [diff] [review]
Part 1 - find strongly connected components

Review of attachment 683176 [details] [diff] [review]:
-----------------------------------------------------------------

::: js/src/gc/FindSCCs.cpp
@@ +11,5 @@
> +namespace js {
> +namespace gc {
> +
> +ComponentFinder::ComponentFinder(uintptr_t sl) :
> +    clock(1),

The colon should go on this line.

@@ +62,5 @@
> +            w->gcNextGraphNode = firstComponent;
> +            firstComponent = w;
> +        }
> +
> +        return;

Not needed.

@@ +112,5 @@
> +            w->gcDiscoveryTime = Finished;
> +
> +            /*
> +             * Prepend the component to the beginning of the output list to
> +             * reverse the list and acheive the desired order.

achieve

::: js/src/gc/FindSCCs.h
@@ +19,5 @@
> +    unsigned       gcDiscoveryTime;
> +    unsigned       gcLowLink;
> +
> +    GraphNodeBase() :
> +        gcNextGraphNode(NULL),

Colon should go on the same line as the first initializer.

@@ +69,5 @@
> + *     void findGraphEdges(ComponentFinder& finder)
> + *     {
> + *         for edge in my_outgoing_edges:
> + *             if is_relevant(edge):
> + *                 finder.addEdgeCallback(v, edge.destination)

This now takes just one parameter.

@@ +102,5 @@
> +    static GraphNodeBase *removeFirstGroup(GraphNodeBase *resultsList);
> +    static void removeAllRemaining(GraphNodeBase *resultsList);
> +
> +  public:
> +    // call from implementation of GraphNodeBase::findEdges()

Please use this style to make it consistent with the other comments in the file:
  /* Call from ... findEdges(). */

@@ +113,5 @@
> +    /* Constant used to indicate an processed vertex that is no longer on the stack. */
> +    static const unsigned Finished = (unsigned)-1;
> +
> +    /* Amount of headroom to leave when checking for stack exhaustion. */
> +    static const unsigned stackHeadroom = 1024;

I think this is no longer needed.

@@ +115,5 @@
> +
> +    /* Amount of headroom to leave when checking for stack exhaustion. */
> +    static const unsigned stackHeadroom = 1024;
> +
> +  private:

This is redundant.

@@ +119,5 @@
> +  private:
> +    void processNode(GraphNodeBase *v);
> +    void checkStackFull();
> +
> +  private:

This is unused.

@@ +121,5 @@
> +    void checkStackFull();
> +
> +  private:
> +
> +  private:

This stuff must be super-duper private :-).

@@ +122,5 @@
> +
> +  private:
> +
> +  private:
> +    unsigned   clock;

Please match the indentation of the fields below.

::: js/src/jsapi-tests/testFindSCCs.cpp
@@ +31,5 @@
> +
> +void
> +TestNode::findGraphEdges(ComponentFinder& finder)
> +{
> +    for (unsigned i = 0; i < MaxVertices; ++i)

Braces for this loop, since it encompasses multiple lines.

Comment 42

[Jon Coppeard (:jonco)]

Attachment: Part 1 - find strongly connected components, comments addressed

Comment 43

[Jon Coppeard (:jonco)]

Attachment: Part 4 - sweep compartments in groups

Comment 44

[Jon Coppeard (:jonco)]

Attachment: Part 5 - Fix gray marking issues

Comment 45

[Jon Coppeard (:jonco)]

Attachment: Part 7 - Update GC stats

Comment 46

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

Comment on attachment 683178 [details] [diff] [review]
Part 3 - split up XPC finalization callback

Review of attachment 683178 [details] [diff] [review]:
-----------------------------------------------------------------

I think we should be able to remove all this mThreadRunningGC business, but I guess that can be a separate bug.

Comment 47

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

Comment on attachment 683502 [details] [diff] [review]
Part 1 - find strongly connected components, comments addressed

Review of attachment 683502 [details] [diff] [review]:
-----------------------------------------------------------------

::: js/src/gc/FindSCCs.h
@@ +24,5 @@
> +        gcDiscoveryTime(0),
> +        gcLowLink(0) {}
> +
> +    virtual ~GraphNodeBase() {}
> +    virtual void findGraphEdges(ComponentFinder& finder) = 0;

One more thing: I realized it would be clearer if this were called findOutgoingEdges.

@@ +103,5 @@
> +    static void removeAllRemaining(GraphNodeBase *resultsList);
> +
> +  public:
> +    /* Call from implementation of GraphNodeBase::findEdges(). */
> +    void addEdgeCallback(GraphNodeBase *w);

...and if this were called addEdgeTo.

Comment 48

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

Comment on attachment 683181 [details] [diff] [review]
Part 6 - sweep debuggers and their debugees in the same group

Review of attachment 683181 [details] [diff] [review]:
-----------------------------------------------------------------

::: js/src/jsweakmap.h
@@ +204,5 @@
>              }
>          }
>      }
> +
> +protected:

This should be indented two spaces.

::: js/src/vm/Debugger.cpp
@@ +1555,5 @@
>      while (!debuggers->empty())
>          debuggers->back()->removeDebuggeeGlobal(fop, global, compartmentEnum, NULL);
>  }
>  
> +void Debugger::findCompartmentEdges(JSCompartment *v, js::gc::ComponentFinder &finder)

void should be on its own line. Also, we usually write /* static */ before void so it's clear it's a static method. Also, please call the compartment |comp| instead of |v|.

Also, I think this should have a comment explaining that it's adding edges in the reverse direction to ensure the debugger and debuggee are always in the same compartment group.

::: js/src/vm/Debugger.h
@@ +72,5 @@
> +    bool init(uint32_t len = 16) {
> +        return Base::init(len) && compartmentCounts.init();
> +    }
> +
> +    AddPtr lookupForAdd(const Lookup &l) const          { return Base::lookupForAdd(l); }

Usually we'd write this as:
    AddPtr lookupForAdd(const Lookup &l) const {
        return Base::lookupForAdd(l);
    }

Sometimes we do use these sorts of aligned one-liners, but only when there are several of them in a row.

There are a few other instances of this below, too.

@@ +77,5 @@
> +
> +    template<typename KeyInput, typename ValueInput>
> +    bool relookupOrAdd(AddPtr &p, const KeyInput &k, const ValueInput &v) {
> +        if (!valueCompartment)
> +            valueCompartment = v->compartment();

It seems cleaner to me to pass this into the constructor or init(). You can get it via dbg->object->compartment().

@@ +93,5 @@
> +    void remove(const Lookup &l) {
> +        Base::remove(l);
> +        decCompartmentCount(l->compartment());
> +        if (Base::count() == 0)
> +            valueCompartment = NULL;

I don't think this should ever be needed. As far as I know, we never change the compartment a Debugger object is in.

@@ +109,5 @@
> +            JS_ASSERT(key == r.front().key);
> +        }
> +    }
> +
> +    void findCompartmentEdges(JSCompartment *v, js::gc::ComponentFinder &finder) {

I think it would be cleaner if you had a function called |hasKeyInCompartment(comp)| that would return a bool. Then you could call that from Debugger::findCompartmentEdges. I think it's cleaner this way because the logic about debugger edges would be more centrally-located in Debugger.cpp. The way it is now, it's hard to follow without knowing the circumstances in which Debugger::findCompartmentEdges calls this function.

Comment 49

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

Comment on attachment 683674 [details] [diff] [review]
Part 7 - Update GC stats

Review of attachment 683674 [details] [diff] [review]:
-----------------------------------------------------------------

::: js/src/gc/Statistics.cpp
@@ +1,2 @@
>  /* -*- Mode: C++; tab-width: 8; indent-tabs-mode: nil; c-basic-offset: 4 -*-
> +s * vim: set ts=8 sw=4 et tw=78:

There's an extra 's' at the beginning of the line.

Comment 50

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

Comment on attachment 683187 [details] [diff] [review]
Part 9 - Refactor DebugScopes to be tracked per compartment

Review of attachment 683187 [details] [diff] [review]:
-----------------------------------------------------------------

::: js/src/jscompartment.cpp
@@ +676,5 @@
>          }
>      }
>  
> +    if (debugScopes)
> +        debugScopes->sweep(rt);

It would be nice if this were timed as part of SWEEP_TABLES.

::: js/src/vm/ScopeObject.cpp
@@ +1527,5 @@
>  
> +DebugScopes::DebugScopes(JSContext *c)
> + : proxiedScopes(c),
> +   missingScopes(c),
> +   liveScopes(c)

This won't work. When you pass a JSContext to a hashtable constructor, it hangs onto it so that it can automatically report OOM errors for you. However, the JSContext we pass in here may die before the compartment dies, in which case we'll have a dead reference.

However, I don't think there's any reason why you can't continue to pass in a JSRuntime here. You can get a JSRuntime via cx->runtime or via compartment->rt.

Comment 51

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

Hmm, it looks like I was wrong about the context thing in comment 50. We explicitly instantiate hashtables with a RuntimeAllocPolicy, which just pulls the runtime off the context. So your change can stay, although I'm still unsure why you can't keep the old behavior. If you do decide to pass in a JSContext, it should be called |cx| instead of |c|.

Comment 52

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

Comment on attachment 683189 [details] [diff] [review]
Part 10 - Track WeakMaps per compartment

Review of attachment 683189 [details] [diff] [review]:
-----------------------------------------------------------------

I like how you handled the saving and restoring of the weakmaps. That's much cleaner than what I was thinking.

::: js/src/jscompartment.cpp
@@ +100,1 @@
>          js_delete(debugScopes);

Indentation is messed up here.

@@ +674,5 @@
>          }
>      }
>  
> +    /* Finalize unreachable (key,value) pairs in all weak maps. */
> +    WeakMapBase::sweepCompartment(this);

Would be nice if this could be timed under SWEEP_TABLES too.

::: js/src/jsgc.cpp
@@ +3496,5 @@
>      }
>      rt->gcFoundBlackGrayEdges = false;
>  }
>  
> +bool MarkWeakMapsIteratively(JSRuntime *rt)

bool should go on a separate line.

@@ +3501,5 @@
> +{
> +    bool markedAny = false;
> +    GCMarker *gcmarker = &rt->gcMarker;
> +    for (GCCompartmentGroupIter c(rt); !c.done(); c.next()) {
> +        if (WeakMapBase::markCompartmentIteratively(c, gcmarker))

You can just use |= here.

@@ +3586,1 @@
>          return;

Indentation.

::: js/src/jsweakmap.cpp
@@ +63,1 @@
>          m->traceMappings(tracer);

Indentation broken (and the next line).

Comment 53

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

Comment on attachment 683191 [details] [diff] [review]
Part 11 - Improve finding of Debugger edges

Review of attachment 683191 [details] [diff] [review]:
-----------------------------------------------------------------

Just saw this :-). Thanks.

Comment 54

[Jon Coppeard (:jonco)]

(In reply to Bill McCloskey (:billm) from comment #51)

Thanks for all the reviews.

The reason I changed the DebugScopes constructor is that proxiedScopes is a WeakMap and I'm getting rid of the WeakMap constructor that takes a JSRuntime in the next patch. I think changed the others just to make it look cleaner.

Comment 55

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

Comment on attachment 683671 [details] [diff] [review]
Part 4 - sweep compartments in groups

Review of attachment 683671 [details] [diff] [review]:
-----------------------------------------------------------------

::: js/src/jscompartment.h
@@ +210,2 @@
>              return needsBarrier();
>          }

Indentation.

@@ +253,1 @@
>          return gcState == Mark;

Indentation.

@@ +378,5 @@
>      void sweep(js::FreeOp *fop, bool releaseTypes);
>      void sweepCrossCompartmentWrappers();
>      void purge();
>  
> +    void findGraphEdges(js::gc::ComponentFinder& finder);

I guess it's not required, but this should be declared virtual.

::: js/src/jsgc.cpp
@@ +3629,3 @@
>  #endif
>  
> +static void DropStringWrappers(JSRuntime *rt)

static and void should be on a separate line.

@@ +3672,1 @@
>          if (e.front().key.kind == CrossCompartmentKey::StringWrapper)

Can we now assert that this doesn't happen?

@@ +3675,5 @@
>          Cell *other = e.front().key.wrapped;
> +        if (!other->isMarked(BLACK) || other->isMarked(GRAY)) {
> +            JSCompartment *w = other->compartment();
> +            if (w->isGCMarking())
> +            finder.addEdgeCallback(w);

Indentation.

@@ +3744,5 @@
> +            Cell *dst = e.front().key.wrapped;
> +            Cell *src = ToMarkable(e.front().value);
> +            JS_ASSERT(src->compartment() == c);
> +            if (IsCellMarked(&src) && !src->isMarked(GRAY) && dst->isMarked(GRAY)) {
> +                //JS_ASSERT(!dst->compartment()->isCollecting());

Can this happen in more circumstances than it could before? Why did the assert need to be removed?

@@ +3767,1 @@
>                  c->setGCState(JSCompartment::Sweep);

Indentation, here and the rest of this function.

@@ +3781,5 @@
> +            rt->gcFinalizeCallback(&fop, JSFINALIZE_GROUP_START, !rt->gcIsFull /* unused */);
> +    }
> +
> +    if (sweepingAtoms) {
> +        gcstats::AutoPhase ap2(rt->gcStats, gcstats::PHASE_SWEEP_ATOMS);

This can be called ap.

@@ +3804,5 @@
>  
> +#ifdef JS_METHODJIT
> +        /* We need to expand inline frames before stack scanning. */
> +        for (CompartmentsIter c(rt); !c.done(); c.next())
> +            mjit::ExpandInlineFrames(c);

This code here is no longer necessary.

@@ +3907,5 @@
>  {
>      gcstats::AutoPhase ap(rt->gcStats, gcstats::PHASE_SWEEP);
>      FreeOp fop(rt, rt->gcSweepOnBackgroundThread);
>  
> +    for (;;) {

Indentation is messed up in this function.

@@ +4385,5 @@
>          /* fall through */
>        }
>  
>        case SWEEP: {
> +

Can this blank line be removed? Also, this indentation is messed up below.

::: js/src/jsinfer.cpp
@@ +6000,5 @@
>          return;
>      }
>  
> +    TypeObject *tmp = this;
> +    if (IsTypeObjectAboutToBeFinalized(&tmp)) {

I think this can stay as it is because we're guaranteed to be sweeping this compartment.

@@ +6117,5 @@
>              bool remove = false;
> +            TypeObject *typeObject = NULL;
> +            if (key.type.isTypeObject()) {
> +                typeObject = key.type.typeObject();
> +                if (IsTypeObjectAboutToBeFinalized(&typeObject))

Indentation.

@@ +6125,5 @@
>                  remove = true;
>  
>              if (remove)
>                  e.removeFront();
> +            else if (typeObject && typeObject != key.type.typeObject()) {

Need braces around the if part too.

@@ +6159,1 @@
>                      remove = true;

Indentation.

::: js/src/jspropertytree.cpp
@@ +203,5 @@
>      if (!inDictionary()) {
>          /*
> +         * We detach the child from the parent if the parent is reachable.
> +         *
> +         * Note that due to incremental sweeping, the parent pointer may point

I think you want to add "to" to the end of the line.

::: js/src/jstypedarray.cpp
@@ +534,5 @@
>              MarkObjectUnbarriered(trc, views, "arraybuffer.singleview");
> +    }
> +    // Multiple views: do not mark, remove unreachable views in sweepAll.
> +
> +    // Additionally, append buffer to list.

I don't think this needs to be changed. Please revert to the original.

@@ +581,5 @@
> +            if (lastBufferViews)
> +                SetBufferLink(lastBufferViews, buffer);
> +            else
> +                rt->liveArrayBuffers = buffer;
> +            lastBufferViews = *views;

I don't understand these changes. Maybe they were needed before we had a per-compartment list of buffers? If so, please revert to the original.

@@ +591,5 @@
> +    // Terminate the buffer list.
> +    if (lastBufferViews)
> +        SetBufferLink(lastBufferViews, NULL);
> +    else
> +        rt->liveArrayBuffers = NULL;

Same here.

::: js/src/jsweakmap.h
@@ +179,5 @@
>      void sweep(JSTracer *trc) {
>          /* Remove all entries whose keys remain unmarked. */
>          for (Enum e(*this); !e.empty(); e.popFront()) {
>              Key k(e.front().key);
> +            Value v(e.front().value);

Is this needed?

Comment 56

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

Jon, I'd hoped to get through all of the reviews today, but I haven't had time. I should have some time to finish them tomorrow. That will also give me a chance to review the RenewWrapper fix.

Comment 57

[Jon Coppeard (:jonco)]

(In reply to Bill McCloskey (:billm) from comment #55)
> @@ +3744,5 @@
> > +            Cell *dst = e.front().key.wrapped;
> > +            Cell *src = ToMarkable(e.front().value);
> > +            JS_ASSERT(src->compartment() == c);
> > +            if (IsCellMarked(&src) && !src->isMarked(GRAY) && dst->isMarked(GRAY)) {
> > +                //JS_ASSERT(!dst->compartment()->isCollecting());
> 
> Can this happen in more circumstances than it could before? Why did the
> assert need to be removed?

Gray marking doesn't work at this point - it's fixed in the next patch, and the assertion reinstated.


> @@ +581,5 @@
> > +            if (lastBufferViews)
> > +                SetBufferLink(lastBufferViews, buffer);
> > +            else
> > +                rt->liveArrayBuffers = buffer;
> > +            lastBufferViews = *views;
> 
> I don't understand these changes. Maybe they were needed before we had a
> per-compartment list of buffers? If so, please revert to the original.

You're right, this was necessary when we had a single list of buffers.  I'll remove it.

Other comments addressed.

Jon

Comment 58

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

Comment on attachment 683672 [details] [diff] [review]
Part 5 - Fix gray marking issues

Review of attachment 683672 [details] [diff] [review]:
-----------------------------------------------------------------

It seems like there's a lot of extra whitespace at the end of lines in this patch (especially empty lines). You can do
  (setq show-trailing-whitespace t)
in your .emacs and it will show it in red so it's easy to see.

::: js/src/gc/Marking.cpp
@@ +537,5 @@
>      }
>  }
>  
> +static bool
> +ShouldMarkCrossCompartment(JSTracer *trc, RawObject src, Cell *cell)

Could you rename |cell| to |dst|?

@@ +545,3 @@
>  
> +    JSCompartment *c = cell->compartment();
> +    uint32_t color = ((GCMarker *)trc)->getMarkColor();

We try to use static_cast for these sorts of casts. Although, come to think of it, it might be nice to add a function in jsgc.h that turns a JSTracer into a GCMarker while asserting IS_GC_MARKING_TRACER. Could you do that?

::: js/src/jscompartment.h
@@ +250,5 @@
>      }
>  
>      bool isGCMarking() {
>          if (rt->isHeapCollecting())
> +        return gcState == Mark || gcState == MarkGray;

Indentation.

::: js/src/jsgc.cpp
@@ +1533,5 @@
>              if (JS_UNLIKELY(comp->wasGCStarted())) {
>                  if (comp->needsBarrier()) {
>                      aheader->allocatedDuringIncremental = true;
>                      comp->rt->gcMarker.delayMarkingArena(aheader);
> +                } else if (comp->isGCSweeping() || comp->isGCFinished()) {

I don't understand why we would want to do this if isGCFinished() is true. We're already done sweeping that compartment, so there's no point in marking it any more.

@@ +1566,5 @@
>      if (JS_UNLIKELY(comp->wasGCStarted())) {
>          if (comp->needsBarrier()) {
>              aheader->allocatedDuringIncremental = true;
>              comp->rt->gcMarker.delayMarkingArena(aheader);
> +        } else if (comp->isGCSweeping() || comp->isGCFinished()) {

Same here.

@@ +1961,5 @@
>  
>      JS_ASSERT(!unmarkedArenaStackTop);
>      JS_ASSERT(markLaterArenas == 0);
>  
> +    grayRoots.clearAndFree();

Why can't we assert that grayRoots is empty any more? (The reason for the clearAndFree() is that the vector can use memory even if it's empty; clearAndFree() will free the memory.)

@@ +2134,1 @@
>          JS_SET_TRACING_LOCATION(this, (void *)&elem->thing);

Please fix the indentation here.

Also, we may eventually want a per-compartment list of gray roots. But this seems fine to me for now. I don't think we typically have very many.

@@ +3372,5 @@
>       * 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.
>       */
> +    JSCompartment* atomsComp = rt->atomsCompartment;

The * should go after the space.

@@ +3631,5 @@
> +                 * If the cycle collector isn't allowed to collect an object
> +                 * after a non-incremental GC has run, then it isn't allowed to
> +                 * collected it after an incremental GC.
> +                 */
> +                JS_ASSERT_IF(!bitmap->isMarked(cell, GRAY), !incBitmap.isMarked(cell, GRAY));

Excellent! It's nice to know that this is passing now.

@@ +3741,5 @@
>  
> +/*
> + * The following code makes use of a per-compartment singly-linked list of
> + * incoming cross compartment pointers whose referents are due to be marked
> + * gray.

Can you give some more background here about why we need this? Something about how we must mark gray after black, so we can't cross into a different compartment group while marking gray, since we may still need to mark some of its objects black.

@@ +3759,5 @@
> + * MarkIncomingCrossCompartmentPointers.
> + */
> +
> +static unsigned
> +GcGrayLinkSlot(RawObject o)

Please call it GCGrayLinkSlot (or maybe just GrayLinkSlot).

@@ +3761,5 @@
> +
> +static unsigned
> +GcGrayLinkSlot(RawObject o)
> +{
> +    return IsCrossCompartmentWrapper(o) ? JSSLOT_PROXY_EXTRA + 1 : Debugger::gcGrayLinkSlot();

Could you assert that it's either a cross-compartment wrapper or a debug wrapper thing (you might need to add a test for this somewhere)?

@@ +3767,5 @@
> +
> +static Cell *
> +CrossCompartmentPointerReferent(RawObject o)
> +{
> +    return (Cell*)(IsCrossCompartmentWrapper(o) ? GetProxyPrivate(o).toGCThing() : o->getPrivate());

Same assertion here too.

@@ +3777,5 @@
> +    unsigned slot = GcGrayLinkSlot(prev);
> +    RawObject next = prev->getReservedSlot(slot).toObjectOrNull();
> +
> +    if (unlink)
> +        prev->setSlot(slot, UndefinedValue());

Instead of the unlink param, you can use getSlotRef, which will return a reference to the slot. Then the caller can set it to undefined if it chooses. That seems cleaner to me.

@@ +3783,5 @@
> +    return next;
> +}
> +
> +void
> +js::DelayCrossCompartmentGrayMarking(RawObject src, Cell *cell)

You can change the name of |cell| to |dst|?

@@ +3850,5 @@
> +
> +        /*
> +         * Mark any incoming black pointers from previously swept compartments
> +         * whose referents are not marked. This can occur when gray cells become
> +         * black by the action of unmarkGray.

UnmarkGray, not unmarkGray.

::: js/src/jsgc.h
@@ +546,5 @@
>  
> +extern void
> +DelayCrossCompartmentGrayMarking(RawObject src, gc::Cell *cell);
> +
> +

Only one blank line please.

::: js/src/jsproxy.cpp
@@ +2853,1 @@
>      MarkSlot(trc, &obj->getReservedSlotRef(JSSLOT_PROXY_EXTRA + 1), "extra1");

Indent.

::: js/src/jsweakmap.h
@@ +129,5 @@
>      bool markValue(JSTracer *trc, Value *x) {
>          if (gc::IsMarked(x))
>              return false;
>          gc::Mark(trc, x, "WeakMap entry");
> +        /* If compartment is not currently being marked, Mark() may have no effect. */

If the compartment isn't being marked, won't the IsMarked check above return true?

::: js/src/vm/Debugger.cpp
@@ +409,5 @@
>  
> +JS_STATIC_ASSERT(unsigned(JSSLOT_DEBUGENV_GC_GRAY_LINK) ==
> +                 unsigned(JSSLOT_DEBUGOBJECT_GC_GRAY_LINK));
> +JS_STATIC_ASSERT(unsigned(JSSLOT_DEBUGENV_GC_GRAY_LINK) ==
> +                 unsigned(JSSLOT_DEBUGSCRIPT_GC_GRAY_LINK));

It probably makes sense to put this assertion directly above the definition of gcGrayLinkSlot.

Comment 59

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

Comment on attachment 683193 [details] [diff] [review]
Part 13 - Handle nuked wrappers

Review of attachment 683193 [details] [diff] [review]:
-----------------------------------------------------------------

Looks great. Good luck!

::: js/src/jsgc.cpp
@@ +3808,5 @@
> +    return IsCrossCompartmentWrapper(o) ||
> +           IsDeadProxyObject(o) ||
> +           c == &DebuggerObject_class ||
> +           c == &DebuggerEnv_class ||
> +           c == &DebuggerScript_class;

Could you add code to Debugger.{cpp,h} for a function named IsDebugWrapper that would check the last three cases?

@@ +4100,3 @@
>          JS_ASSERT(!c->gcIncomingGrayPointers);
> +        for (WrapperMap::Enum e(c->crossCompartmentWrappers); !e.empty(); e.popFront()) {
> +            const Value& wrapper = e.front().value.get();

The & should go after the space.

@@ +4101,5 @@
> +        for (WrapperMap::Enum e(c->crossCompartmentWrappers); !e.empty(); e.popFront()) {
> +            const Value& wrapper = e.front().value.get();
> +            if (wrapper.isObject()) {
> +                RawObject o = &wrapper.toObject();
> +                if (IsCrossCompartmentWrapper(o))

Could we make this more general and check that GCGrayListSlot is undefined regardless of the kind of wrapper?

@@ +4279,3 @@
>          JS_ASSERT(!c->gcIncomingGrayPointers);
> +        for (WrapperMap::Enum e(c->crossCompartmentWrappers); !e.empty(); e.popFront()) {
> +            const Value& wrapper = e.front().value.get();

Ampersand placement.

@@ +4280,5 @@
> +        for (WrapperMap::Enum e(c->crossCompartmentWrappers); !e.empty(); e.popFront()) {
> +            const Value& wrapper = e.front().value.get();
> +            if (wrapper.isObject()) {
> +                RawObject o = &wrapper.toObject();
> +                if (IsCrossCompartmentWrapper(o))

Same here as above.

Comment 60

[Jon Coppeard (:jonco)]

(In reply to Bill McCloskey (:billm) from comment #58)

> Why can't we assert that grayRoots is empty any more? (The reason for the
> clearAndFree() is that the vector can use memory even if it's empty;
> clearAndFree() will free the memory.)

The original code cleared grayRoots after marking its contents, but now we only remove the gray roots for compartments we actually mark.  I assume it's possible for grayRoots to contain roots in compartments that we aren't currently collecting.

> @@ +3777,5 @@
> > +    unsigned slot = GcGrayLinkSlot(prev);
> > +    RawObject next = prev->getReservedSlot(slot).toObjectOrNull();
> > +
> > +    if (unlink)
> > +        prev->setSlot(slot, UndefinedValue());
> 
> Instead of the unlink param, you can use getSlotRef, which will return a
> reference to the slot. Then the caller can set it to undefined if it
> chooses. That seems cleaner to me.

I tried this but it turns out that you need to pass the object and slot number to HeapSlot::set anyway, so it doesn't really end up any cleaner.

Other comments addressed.

Cheers,

Jon

Comment 61

[Jon Coppeard (:jonco)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/59692d8cd68e
https://hg.mozilla.org/integration/mozilla-inbound/rev/ba392e23a458
https://hg.mozilla.org/integration/mozilla-inbound/rev/067ed4955e29
https://hg.mozilla.org/integration/mozilla-inbound/rev/f55177f70719
https://hg.mozilla.org/integration/mozilla-inbound/rev/2fd089d5fff4
https://hg.mozilla.org/integration/mozilla-inbound/rev/481687026c08
https://hg.mozilla.org/integration/mozilla-inbound/rev/74f07744018a
https://hg.mozilla.org/integration/mozilla-inbound/rev/a65bfc6d1975
https://hg.mozilla.org/integration/mozilla-inbound/rev/344fff01645b
https://hg.mozilla.org/integration/mozilla-inbound/rev/1bdaf0020ab9
https://hg.mozilla.org/integration/mozilla-inbound/rev/4c0098a5bd92
https://hg.mozilla.org/integration/mozilla-inbound/rev/3bf6ca8d603b
https://hg.mozilla.org/integration/mozilla-inbound/rev/26172ff887ae

Comment 62

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/59692d8cd68e
https://hg.mozilla.org/mozilla-central/rev/ba392e23a458
https://hg.mozilla.org/mozilla-central/rev/067ed4955e29
https://hg.mozilla.org/mozilla-central/rev/f55177f70719
https://hg.mozilla.org/mozilla-central/rev/2fd089d5fff4
https://hg.mozilla.org/mozilla-central/rev/481687026c08
https://hg.mozilla.org/mozilla-central/rev/74f07744018a
https://hg.mozilla.org/mozilla-central/rev/a65bfc6d1975
https://hg.mozilla.org/mozilla-central/rev/344fff01645b
https://hg.mozilla.org/mozilla-central/rev/1bdaf0020ab9
https://hg.mozilla.org/mozilla-central/rev/4c0098a5bd92
https://hg.mozilla.org/mozilla-central/rev/3bf6ca8d603b
https://hg.mozilla.org/mozilla-central/rev/26172ff887ae