692884 - Manage GC's black/gray color transitions from JS, not xpconnect

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

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

Attachment: patch

This patch splits apart the gcExtraGCRootsTraceOp into two. One only does black marking and one only does gray marking. This makes it so that XPConnect doesn't poke into our internals too much.

Also, when Andrew finishes the weakmap/CC integration, we'll be able to add an assertion to ensure that we never switch from marking gray to marking black.

Comment 1

[Andrew McCreight [:mccr8]]

Comment on attachment 565607 [details] [diff] [review]
patch

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

Looks good.  Just a few minor suggestions.  It will be nice to have this wrapped up so people can't trip on it.  I also like how XPConnect now uses the GC's definition of gray instead of remaking its own, and hoping it is the same.  That always made me a little nervous.

::: js/src/jscntxt.h
@@ +513,5 @@
>      volatile ptrdiff_t  gcMallocBytes;
>  
>    public:
>      /*
> +     * The trace operations and their datas argument to trace embedding-specific

"their datas argument" looks weird to me, but maybe this is one of those things where data is plural but I think of it as singular.

::: js/src/jsgc.h
@@ +1589,5 @@
> +    /*
> +     * The only valid color transition during a GC is from black to gray. It is
> +     * wrong to switch the mark color from gray to black. We don't assert this
> +     * yet, but we should.
> +     */

Would it make sense to change setMarkColor to a protected method, and then set EndMarkPhase as a friend method?  I don't know if friend methods are looked down upon or not, but it would be nice if we could keep random callers in XPConnect or even the GC from calling setMarkColor.  Or at least give people a little more pause before they do so.

There should also be a comment somewhere (maybe here) explaining why changing the mark color from gray to black is so dangerous.  I don't think it is explicitly laid out anywhere.  The reason is that the cycle collector depends on the invariant that there are no black to gray edges in the GC heap.  This invariant lets the CC not trace through black objects.  If this invariant is violated, the cycle collector may free objects that are still reachable.  Or something like that.  If you think that's too CC-specific, you could add the explanation into nsXPConnect.cpp somewhere (maybe at the end of the comment at the start of nsXPConnect::Collect) and just tell people to look there for an explanation.

As an aside, it looks like that when I drop the black marking of weak references, in the cycle collector patch, I can change this to setMarkColorGray(), with the assert that color is BLACK, as you have here.

@@ +1594,2 @@
>      void setMarkColor(uint32 newColor) {
> +        //JS_ASSERT(color == BLACK);

Maybe change this commented out assert to color == BLACK && newColor == GRAY?  Or two assertions, if that's more appropriate.

Comment 2

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

https://hg.mozilla.org/integration/mozilla-inbound/rev/0042da9bc018

I didn't do the friend thing since it seems a little gross to me. We're making a lot of progress cleaning up header usage in xpconnect, so pretty soon we won't have to worry about it reaching into our guts.

Comment 3

[Andreas Gal :gal]

Nice work. Happy to see this being cleaned up. We came a long way since the original intertwined GC/CC state.

Comment 4

[Marco Bonardo [:mak] (Away Apr 25 - May 5)]

https://hg.mozilla.org/mozilla-central/rev/0042da9bc018

Comment 5

[Peter Van der Beken [:peterv]]

This is slightly annoying. To avoid adding objects used off of the main thread to the CC I was planning on switching back and forth between black and gray. I guess this means I'll have to split up the hashes into two.

(In reply to Andrew McCreight [:mccr8] from comment #1)
> There should also be a comment somewhere (maybe here) explaining why
> changing the mark color from gray to black is so dangerous.  I don't think
> it is explicitly laid out anywhere.  The reason is that the cycle collector
> depends on the invariant that there are no black to gray edges in the GC
> heap.  This invariant lets the CC not trace through black objects.

That's only when traversing, not when adding roots. Right?

Comment 6

[Andrew McCreight [:mccr8]]

(In reply to Peter Van der Beken [:peterv] from comment #5)
> This is slightly annoying. To avoid adding objects used off of the main
> thread to the CC I was planning on switching back and forth between black
> and gray. I guess this means I'll have to split up the hashes into two.

It is unfortunate, but I discussed this with Bill and we think it isn't sound.

> (In reply to Andrew McCreight [:mccr8] from comment #1)
> > There should also be a comment somewhere (maybe here) explaining why
> > changing the mark color from gray to black is so dangerous.  I don't think
> > it is explicitly laid out anywhere.  The reason is that the cycle collector
> > depends on the invariant that there are no black to gray edges in the GC
> > heap.  This invariant lets the CC not trace through black objects.
> 
> That's only when traversing, not when adding roots. Right?

Do you mean in the garbage collector?  In the garbage collector there's no real distinction, because when you changed colors before, it did drainMarkStack(), which does a traversal.

Say we're in the middle of adding roots, and there are three objects (A, B and C), where A and B both point to C.  A is a gray root and B is a black root.  We happen to visit A before B.  A goes on the mark stack.  We then decide we want to mark B black, so we must drainMarkStack() so we can safely change the color. This causes us to visit all of A's children, while we're still gray, so C gets marked gray.  Then we switch color to black, and mark B.  C is already marked, so we don't re-mark it black.