Last Comment Bug 739899 - GC: Compartment creation during incremental GC shouldn't reset the GC
: GC: Compartment creation during incremental GC shouldn't reset the GC
Product: Core
Classification: Components
Component: JavaScript Engine (show other bugs)
: unspecified
: All All
-- normal with 1 vote (vote)
: mozilla14
Assigned To: Bill McCloskey (:billm)
: Jason Orendorff [:jorendorff]
: 742003 (view as bug list)
Depends on: 749698
  Show dependency treegraph
Reported: 2012-03-27 22:31 PDT by Bill McCloskey (:billm)
Modified: 2012-04-27 11:41 PDT (History)
10 users (show)
See Also:
Crash Signature:
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---

patch (40.67 KB, patch)
2012-04-04 15:45 PDT, Bill McCloskey (:billm)
igor: review+
Details | Diff | Splinter Review

Description User image Bill McCloskey (:billm) 2012-03-27 22:31:07 PDT
Right now, if a compartment is created during an incremental GC, we reset the current GC and finish with a non-incremental GC. This seems to happen pretty frequently during normal browsing.

In theory, we should be able to carry on with the GC as normal without touching the new compartment (i.e., don't mark it and don't collect any of its objects). In practice, this could be a little difficult since it will be hard to avoid marking into the new compartment. Bug 716142 may simplify the problem: maybe we could switch the GC to a multi-compartment GC of every compartment except the newly-created one.
Comment 1 User image Bill McCloskey (:billm) 2012-04-04 15:45:03 PDT
Created attachment 612375 [details] [diff] [review]

Now that multi-compartment GC has landed, we can make headway on this. Unfortunately, the multi-compartment GC patch preserved a difference between "full" GCs and compartment GCs (even if the compartment GC collected every compartment). This patch removes that distinction.

The main changes in behavior from this patch are:

1. If a compartment is created during an incremental GC, we don't reset the GC or switch to non-incremental GC. In fact, we don't do anything. The new compartment won't be collected.

2. We now collect atoms only if every compartment that existed at the beginning of the GC is being collected.

3. Now any GC can delete a compartment if it has no live objects. Previously only "full" GCs could do this.

Note that most of the time we'll still collect all compartments. It's just that collecting all compartments is no longer a special case.
Comment 2 User image Igor Bukanov 2012-04-04 16:29:32 PDT
Comment on attachment 612375 [details] [diff] [review]

Review of attachment 612375 [details] [diff] [review]:

nice cleanup!

::: js/src/jsatom.cpp
@@ +241,5 @@
>      return true;
>  }
>  void
> +js::FinishCommonAtoms(JSContext *cx)

As you are changing this, replace the cx argument with the rt here

::: js/src/jsgc.cpp
@@ +2277,5 @@
> +            if (!c->isCollecting())
> +                isFullGC = false;
> +        }
> +    }
> +    MarkAtomState(trc, rt->gcKeepAtoms || !isFullGC);

We need a bug to stop marking atoms when the atom compartment is not under the GC itself.
Comment 4 User image Bill McCloskey (:billm) 2012-04-05 18:31:12 PDT
*** Bug 742003 has been marked as a duplicate of this bug. ***
Comment 5 User image Matt Brubeck (:mbrubeck) 2012-04-06 11:43:00 PDT

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