Last Comment Bug 743396 - Don't unmark non-collected compartments during GC
: Don't unmark non-collected compartments during GC
Status: RESOLVED FIXED
:
Product: Core
Classification: Components
Component: JavaScript Engine (show other bugs)
: unspecified
: All All
: -- normal (vote)
: mozilla16
Assigned To: Bill McCloskey (:billm)
:
Mentors:
: 759522 (view as bug list)
Depends on: 742841
Blocks:
  Show dependency treegraph
 
Reported: 2012-04-06 16:59 PDT by Bill McCloskey (:billm)
Modified: 2012-06-15 16:24 PDT (History)
4 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
patch (4.87 KB, patch)
2012-04-06 16:59 PDT, Bill McCloskey (:billm)
no flags Details | Diff | Review
patch v2 (5.06 KB, patch)
2012-04-06 17:42 PDT, Bill McCloskey (:billm)
no flags Details | Diff | Review
patch v3 (5.08 KB, patch)
2012-05-29 16:27 PDT, Bill McCloskey (:billm)
no flags Details | Diff | Review
patch v4 (5.12 KB, patch)
2012-06-10 16:14 PDT, Bill McCloskey (:billm)
continuation: review+
Details | Diff | Review

Description Bill McCloskey (:billm) 2012-04-06 16:59:36 PDT
Created attachment 613027 [details] [diff] [review]
patch

If we want to do more compartmental GC, we need to be clearing the mark bits for compartments that aren't collected. Otherwise, the cycle collector isn't able to collect cycles that touch those compartments.

However, there's a tricky problem here. Let's say we have this configuration:
  Compartment C1: contains object A that points to B
  Compartment C2: contains object B
Now let's say that originally A and B were both gray. Imagine we decide to collect C1, but not C2. It's possible that A could be marked black -- maybe it was found by the conservative stack scanner.

In that case, we have an edge from a black object to a gray object. In theory, this isn't supposed to happen. This patch handles that problem by searching for such edges and clearing the mark bits in compartment C2 (which is what we normally do now). It should be very rare for this to happen.
Comment 1 Andrew McCreight [:mccr8] 2012-04-06 17:14:23 PDT
Comment on attachment 613027 [details] [diff] [review]
patch

What if C2 contains edges to a third compartment C3?
Comment 2 Bill McCloskey (:billm) 2012-04-06 17:16:46 PDT
Comment on attachment 613027 [details] [diff] [review]
patch

Yeah, you're right. That's annoying. I guess I can fix it.
Comment 3 Andrew McCreight [:mccr8] 2012-04-06 17:21:52 PDT
Is this going to just bleed into everything?  If every compartment has edges to and from chrome, then you'll just end up clearing all the mark bits.
Comment 4 Bill McCloskey (:billm) 2012-04-06 17:25:07 PDT
(In reply to Andrew McCreight [:mccr8] from comment #3)
> Is this going to just bleed into everything?  If every compartment has edges
> to and from chrome, then you'll just end up clearing all the mark bits.

Perhaps the best thing to do is if we find anything then just unmark all the uncollected compartments.

We expect this to be a rare situation, so I think this is fine.
Comment 5 Andrew McCreight [:mccr8] 2012-04-06 17:27:49 PDT
Ah, so the initial situation of having a (new) black-gray edge that spans compartments should be rare.  That makes sense, though I haven't investigated that myself.
Comment 6 Bill McCloskey (:billm) 2012-04-06 17:42:08 PDT
Created attachment 613039 [details] [diff] [review]
patch v2

Here's a new patch. It would be useful to get info on how common these edges are. Maybe with telemetry or something.
Comment 7 Bill McCloskey (:billm) 2012-04-06 17:53:23 PDT
Comment on attachment 613039 [details] [diff] [review]
patch v2

Actually, maybe I'll test this some more.
Comment 8 Igor Bukanov 2012-04-08 03:48:22 PDT
(In reply to Bill McCloskey (:billm) from comment #0)
> Now let's say that originally A and B were both gray. Imagine we decide to
> collect C1, but not C2. It's possible that A could be marked black -- maybe
> it was found by the conservative stack scanner.
> 
> In that case, we have an edge from a black object to a gray object. In
> theory, this isn't supposed to happen.

This can only happen if after an initial full GC we want to run a series of compartment GC before running the CC, right? If so, why not to mark propagate the black bit recursively if we ever find a gray object when navigating from the compartment roots? The complication with that approach is what to to do with a gray edge into the compartment. Currently AFAIK we do not distinguish such edges treating anything reachable from outside as black.
Comment 9 Bill McCloskey (:billm) 2012-05-29 16:27:54 PDT
Created attachment 628138 [details] [diff] [review]
patch v3

Fixed a few bugs in the previous version.
Comment 10 Andrew McCreight [:mccr8] 2012-05-30 11:37:10 PDT
Comment on attachment 628138 [details] [diff] [review]
patch v3

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

Here are my comments that you probably don't want.

::: js/src/jsgc.cpp
@@ +3093,5 @@
> +     * an edge to an uncollected compartment: it's possible that the source and
> +     * destination of the cross-compartment edge should be gray, but the source
> +     * was marked black by the conservative scanner. To avoid the black->gray
> +     * edge, we completely clear the mark bits of the destination object's
> +     * compartment. This is safe because the destination compartment isn't being

The second-to-last sentence of this comment isn't quite precise any more.  The code doesn't clear the mark bits of the destination object's compartment, it clears the mark bits of all non-collected compartments.  Maybe it would make sense to move that comment down to the |if (foundBlackGray)) section?

Does UnmarkGray actually go through wrappers?  I guess I'm not sure exactly how that works.  Does an object actually contain a raw pointer to the other compartment, or does it point to the wrapper... somehow...?  And in the latter case, the JS_TraceChildren knows how to look for it?

@@ +3094,5 @@
> +     * destination of the cross-compartment edge should be gray, but the source
> +     * was marked black by the conservative scanner. To avoid the black->gray
> +     * edge, we completely clear the mark bits of the destination object's
> +     * compartment. This is safe because the destination compartment isn't being
> +     * collected.

Without bug 742841, this will miss cross-compartment black-gray edges involving the debugger, right?

@@ +3111,3 @@
>      }
> +
> +    if (foundBlackGray) {

Is this actually rare?  In other words, if you browse around a little, do you tend to hit it?

@@ +3113,5 @@
> +    if (foundBlackGray) {
> +        for (CompartmentsIter c(rt); !c.done(); c.next()) {
> +            if (!c->isCollecting())
> +                c->arenas.unmarkAll();
> +        }

So the idea here is that if you see any black-gray edge, you just clear all mark bits, thus replicating our current behavior?
Comment 11 Bill McCloskey (:billm) 2012-05-30 13:31:45 PDT
I'll fix the comment. This is intended to be based on top of bug 742841 (note the dependency).

UnmarkGray will cross through compartments without a care in the world. A cross-compartment wrapper is just a Proxy object whose private pointer points into the other compartment. So JS_TraceChildren treats it like any other pointer.

At one point I added JS_ASSERT(!foundBlackGray) and I browsed around for a few minutes. I never hit it. However, that's not the most rigorous testing in the world.
Comment 12 Andrew McCreight [:mccr8] 2012-05-30 13:35:11 PDT
(In reply to Bill McCloskey (:billm) from comment #11)
> I'll fix the comment. This is intended to be based on top of bug 742841
> (note the dependency).

Great.  I figured you'd realized that, but I thought I'd mention it.  And I failed to notice the dependency.

> UnmarkGray will cross through compartments without a care in the world. A
> cross-compartment wrapper is just a Proxy object whose private pointer
> points into the other compartment. So JS_TraceChildren treats it like any
> other pointer.

Ah, great.  I figured it must work, but I had a moment of panic there.

> At one point I added JS_ASSERT(!foundBlackGray) and I browsed around for a
> few minutes. I never hit it. However, that's not the most rigorous testing
> in the world.

Sure.  It might be interesting to push to try with something that will dump to the log when you trip it, then look at logs.  I can do that if you'd like.
Comment 13 Bill McCloskey (:billm) 2012-06-10 16:14:08 PDT
Created attachment 631779 [details] [diff] [review]
patch v4

I fixed the comment. I also pushed a version of this to tryserver. It asserts if we ever find black/gray cross-compartment edges.

https://tbpl.mozilla.org/?tree=Try&rev=810d24073d57
Comment 15 Graeme McCutcheon [:graememcc] 2012-06-12 03:07:56 PDT
https://hg.mozilla.org/mozilla-central/rev/62d7c3276ceb
Comment 16 Bill McCloskey (:billm) 2012-06-15 16:24:34 PDT
*** Bug 759522 has been marked as a duplicate of this bug. ***

Note You need to log in before you can comment on or make changes to this bug.