Last Comment Bug 716142 - GC: Allow multi-compartment GCs
: GC: Allow multi-compartment GCs
Status: RESOLVED FIXED
:
Product: Core
Classification: Components
Component: JavaScript Engine (show other bugs)
: unspecified
: All All
: -- normal with 1 vote (vote)
: mozilla14
Assigned To: Bill McCloskey (:billm)
:
:
Mentors:
: 720843 (view as bug list)
Depends on: 716619 742003 747243
Blocks: 756311
  Show dependency treegraph
 
Reported: 2012-01-06 17:00 PST by Bill McCloskey (:billm)
Modified: 2012-05-17 16:46 PDT (History)
12 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
patch (57.88 KB, patch)
2012-01-06 17:00 PST, Bill McCloskey (:billm)
no flags Details | Diff | Splinter Review
patch v2 (59.74 KB, patch)
2012-01-08 14:52 PST, Bill McCloskey (:billm)
no flags Details | Diff | Splinter Review
patch v3 (48.26 KB, patch)
2012-01-09 11:45 PST, Bill McCloskey (:billm)
igor: review+
Details | Diff | Splinter Review
debugger patch (11.93 KB, patch)
2012-01-09 11:48 PST, Bill McCloskey (:billm)
no flags Details | Diff | Splinter Review
debugger patch v2 (15.06 KB, patch)
2012-01-13 14:01 PST, Bill McCloskey (:billm)
jorendorff: review+
Details | Diff | Splinter Review

Description Bill McCloskey (:billm) 2012-01-06 17:00:02 PST
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.
Comment 1 Igor Bukanov 2012-01-06 18:41:33 PST
(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 (:billm) 2012-01-06 18:53:49 PST
(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 (:billm) 2012-01-08 14:52:11 PST
Created attachment 586846 [details] [diff] [review]
patch v2

Updated patch.
Comment 4 Igor Bukanov 2012-01-09 00:51:08 PST
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 (:billm) 2012-01-09 11:45:04 PST
Created attachment 587061 [details] [diff] [review]
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 (:billm) 2012-01-09 11:48:04 PST
Created attachment 587062 [details] [diff] [review]
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 2012-01-09 11:54:41 PST
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] 2012-01-13 13:51:21 PST
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 (:billm) 2012-01-13 14:01:57 PST
Created attachment 588521 [details] [diff] [review]
debugger patch v2

Thanks. Here's the revised patch.
Comment 10 Jason Orendorff [:jorendorff] 2012-01-16 13:43:39 PST
Comment on attachment 588521 [details] [diff] [review]
debugger patch v2

Yep, even better. Nice work.
Comment 11 Terrence Cole [:terrence] 2012-01-24 15:27:22 PST
*** Bug 720843 has been marked as a duplicate of this bug. ***
Comment 12 Olli Pettay [:smaug] (reviewing overload) 2012-02-07 04:20:36 PST
The patches don't apply cleanly to m-c anymore.
Comment 13 Bill McCloskey (:billm) 2012-02-07 08:02:06 PST
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 15 :Ms2ger (⌚ UTC+1/+2) 2012-04-03 02:08:25 PDT
https://hg.mozilla.org/mozilla-central/rev/b1a9e8a536bf

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