The default bug view has changed. See this FAQ.

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

RESOLVED FIXED in mozilla10

Status

()

Core
JavaScript Engine
RESOLVED FIXED
6 years ago
6 years ago

People

(Reporter: billm, Unassigned)

Tracking

Trunk
mozilla10
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment)

(Reporter)

Description

6 years ago
Created attachment 565607 [details] [diff] [review]
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.
Attachment #565607 - Flags: review?(continuation)
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.
Attachment #565607 - Flags: review?(continuation) → review+
Version: unspecified → Trunk
(Reporter)

Comment 2

6 years ago
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.
Target Milestone: --- → mozilla10
Blocks: 653248

Comment 3

6 years ago
Nice work. Happy to see this being cleaned up. We came a long way since the original intertwined GC/CC state.
https://hg.mozilla.org/mozilla-central/rev/0042da9bc018
Status: NEW → RESOLVED
Last Resolved: 6 years ago
Resolution: --- → FIXED
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?
(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.
You need to log in before you can comment on or make changes to this bug.