716142 - GC: Allow multi-compartment GCs

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

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

Attachment: patch

Currently we can GC all compartments or a single compartment. It would be good if we could GC an arbitrary subset of compartments. Then, for example, we could GC all the compartments that have run scripts in the last 5 seconds. This would be useful for bug 716014.

Comment 1

[Igor Bukanov]

(In reply to Bill McCloskey (:billm) from comment #0)
> Created attachment 586616 [details] [diff] [review]
> patch
> 
> Currently we can GC all compartments or a single compartment. It would be
> good if we could GC an arbitrary subset of compartments. Then, for example,
> we could GC all the compartments that have run scripts in the last 5
> seconds. This would be useful for bug 716014.

This is a nice idea. 

From a quick look to the patch:

1. Why it removes scheduling of the full GC in TriggerCompartmentGC when

rt->gcBytes > 8192 && rt->gcBytes >= 3 * (rt->gcTriggerBytes / 2) ?

2. Explicit isGCing flag query shoild be replaced with a predicate like JSCompartment::underGC(). That predicate can assert, for example, that the GC is running to prevent wrongful access to the flag. Also it seems gcIsNeeded and isGCing in JSCompartment() can be merged into explicit single 3-state enum. 

3. the patch should comment why it is not possible to have a single 3-state enum in place of JSRuntime::gcIsNeeded/gcFullIsNeeded. 

4. Debugger::markAllIteratively contains:

         /*
          * If this is a single-compartment GC, no compartment can debug itself, so skip
          * |comp|. If it's a global GC, then search every compartment.
          */
-        if (comp && dc == comp)
+        if (!rt->gcIsFull && c->isGCing)
             continue;

Just based on the comment the change cannot be right as it sounds like an optimization that is no longer possible as we can have non-global GC and yet both the debugger and the compartment it debug can be under the GC.

Comment 2

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

(In reply to Igor Bukanov from comment #1)
> 1. Why it removes scheduling of the full GC in TriggerCompartmentGC when
> 
> rt->gcBytes > 8192 && rt->gcBytes >= 3 * (rt->gcTriggerBytes / 2) ?

I added this trigger in the original compartment GC patch. I don't think it's ever actually been useful to us, so I think we should take it out. However, maybe that should be a different bug.

> 2. Explicit isGCing flag query shoild be replaced with a predicate like
> JSCompartment::underGC(). That predicate can assert, for example, that the
> GC is running to prevent wrongful access to the flag. Also it seems
> gcIsNeeded and isGCing in JSCompartment() can be merged into explicit single
> 3-state enum. 

Sounds good.

> 3. the patch should comment why it is not possible to have a single 3-state
> enum in place of JSRuntime::gcIsNeeded/gcFullIsNeeded. 

I hadn't thought of that, but you're right :-).

> 4. Debugger::markAllIteratively contains:
> 
>          /*
>           * If this is a single-compartment GC, no compartment can debug
> itself, so skip
>           * |comp|. If it's a global GC, then search every compartment.
>           */
> -        if (comp && dc == comp)
> +        if (!rt->gcIsFull && c->isGCing)
>              continue;
> 
> Just based on the comment the change cannot be right as it sounds like an
> optimization that is no longer possible as we can have non-global GC and yet
> both the debugger and the compartment it debug can be under the GC.

Good catch. I'll fix that.

Comment 3

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

Attachment: patch v2

Updated patch.

Comment 4

[Igor Bukanov]

Comment on attachment 586846 [details] [diff] [review]
patch v2

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

The patch needs more work - see comments below. Also ask Jason to review the debugger changes, I do not know the implementation details there.

::: dom/base/nsJSEnvironment.cpp
@@ +3506,5 @@
>      // a full GC to happen. Compartment GCs usually happen as a
>      // result of last-ditch or MaybeGC. In both cases its
>      // probably a time of heavy activity and we want to delay
>      // the full GC, but we do want it to happen eventually.
> +    if (!isFull) {

The comment rises a question - what is difference between full GC and a compartment GC that covers all compartments? I guess we need to file a bug of using a different criteria here, like distinguishing last-ditch GC from other kinds of the GC.

::: js/src/gc/Statistics.cpp
@@ +87,1 @@
>  

Fix pre-existing bug and initialize all Statistics fields in the the constructor

@@ +188,5 @@
>  
>      triggerReason = reason;
>  
>      beginPhase(PHASE_GC);
> +    Probes::GCStart(NULL);

File a bug about updating Probes to support the multi-compartment GC.

::: js/src/jscntxt.h
@@ +461,2 @@
>       */
> +    bool                gcStrictCompartmentChecking;

Put #ifdef DEBUG around  gcStrictCompartmentChecking. Note that DebugOnly<bool> does not work here as it that still leave the field in the class.

::: js/src/jscompartment.h
@@ +188,5 @@
> +    enum CompartmentGCState {
> +        NoGCScheduled,
> +        GCScheduled,
> +        GCRunning
> +    };

Nice names!

::: js/src/jsgc.cpp
@@ +1660,5 @@
>  
>      bool runGC = !!rt->gcIsNeeded;
>      for (;;) {
>          if (JS_UNLIKELY(runGC)) {
> +            RunLastDitchGC(cx, true);

Without the patch we run a compartment GC if that is requested here, but the patch changes that to always run the full GC. This would have a clear affect on that GC finished callback in DOM. So restore the logic here unless you have a strong reason not to. But in that case the DOM callback has to be updated as well.

::: js/src/jsgcinlines.h
@@ +375,5 @@
> +    GCCompartmentsIter(JSRuntime *rt) {
> +        JS_ASSERT(rt->gcRunning);
> +        it = rt->compartments.begin();
> +        end = rt->compartments.end();
> +        if (!(*it)->isCollecting())

Assert that we it < end here and *it == rt->atomsCompartment

::: js/src/jsgcmark.cpp
@@ +110,5 @@
>  CheckMarkedThing(JSTracer *trc, T *thing)
>  {
>      JS_ASSERT(thing);
>      JS_ASSERT(trc->debugPrinter || trc->debugPrintArg);
> +    JS_ASSERT_IF(trc->runtime->gcIsFull, IS_GC_MARKING_TRACER(trc));

This should be  !trc->runtime->gcIsFull

Comment 5

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

Attachment: patch v3

(In reply to Igor Bukanov from comment #4)
> ::: dom/base/nsJSEnvironment.cpp
> @@ +3506,5 @@
> >      // a full GC to happen. Compartment GCs usually happen as a
> >      // result of last-ditch or MaybeGC. In both cases its
> >      // probably a time of heavy activity and we want to delay
> >      // the full GC, but we do want it to happen eventually.
> > +    if (!isFull) {
> 
> The comment rises a question - what is difference between full GC and a
> compartment GC that covers all compartments? I guess we need to file a bug
> of using a different criteria here, like distinguishing last-ditch GC from
> other kinds of the GC.

I expect that by the time we start doing real GCs of multiple compartments, we'll have changed the nsJSEnvironment code anyway. So I'd rather leave this code alone right now.

> ::: js/src/jsgcinlines.h
> @@ +375,5 @@
> > +    GCCompartmentsIter(JSRuntime *rt) {
> > +        JS_ASSERT(rt->gcRunning);
> > +        it = rt->compartments.begin();
> > +        end = rt->compartments.end();
> > +        if (!(*it)->isCollecting())
> 
> Assert that we it < end here and *it == rt->atomsCompartment

You want to assert that compartments->begin() is the atoms compartment? Does that matter here? I think the only correctness requirement we have is that at least one compartment is being collected, and the it < end assertion should cover that.

> ::: js/src/jsgcmark.cpp
> @@ +110,5 @@
> >  CheckMarkedThing(JSTracer *trc, T *thing)
> >  {
> >      JS_ASSERT(thing);
> >      JS_ASSERT(trc->debugPrinter || trc->debugPrintArg);
> > +    JS_ASSERT_IF(trc->runtime->gcIsFull, IS_GC_MARKING_TRACER(trc));
> 
> This should be  !trc->runtime->gcIsFull

Well, we can't assert that because gcIsFull is false when we come through here via TraceRuntime. But I've added an assertion that if thing->compartment()->gcState == GCRunning, then we have a marking tracer.

Comment 6

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

Attachment: debugger patch

Jason, this patch changes the debugger code so that we can collect an arbitrary subset of compartments in a single GC.

The previous patch also adds MarkCrossCompartmentX functions so that we can safely mark edges that cross compartments.

Comment 7

[Igor Bukanov]

Comment on attachment 587061 [details] [diff] [review]
patch v3

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

::: js/src/jsgc.cpp
@@ +1072,5 @@
>       * scanning during drainMarkStack (as is done for generators) can break
>       * this invariant. So we disable the compartment assertions in this
>       * situation.
>       */
>      struct AutoSkipChecking {

I suppose #ifdef DEBUG is missing somewhere here.

Comment 8

[Jason Orendorff [:jorendorff]]

Comment on attachment 587062 [details] [diff] [review]
debugger patch

Great simplification!

Please search for comments mentioning GC in this file and fix them up, especially ones that refer to "single-compartment" or "per-compartment" GC, as that concept should be going away.

Debugger::sweepAll needs to be called always now, since the logic described in that comment doesn't work any more (as we discussed on IRC).

Clearing review flag.

Comment 9

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

Attachment: debugger patch v2

Thanks. Here's the revised patch.

Comment 10

[Jason Orendorff [:jorendorff]]

Comment on attachment 588521 [details] [diff] [review]
debugger patch v2

Yep, even better. Nice work.

Comment 12

[Olli Pettay [:smaug][[e-mail]]]

The patches don't apply cleanly to m-c anymore.

Comment 13

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

I pushed this patch to try a while back, and it had some problems. I'll try to get around to rebasing and fixing it soon.

Comment 14

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

https://hg.mozilla.org/integration/mozilla-inbound/rev/b1a9e8a536bf

Comment 15

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

https://hg.mozilla.org/mozilla-central/rev/b1a9e8a536bf