Last Comment Bug 782318 - GC: Simplify compartment GC state
: GC: Simplify compartment GC state
Status: RESOLVED FIXED
:
Product: Core
Classification: Components
Component: JavaScript Engine (show other bugs)
: Trunk
: All All
: -- enhancement (vote)
: mozilla17
Assigned To: Jon Coppeard (:jonco)
:
: Jason Orendorff [:jorendorff]
Mentors:
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2012-08-13 10:05 PDT by Jon Coppeard (:jonco)
Modified: 2012-08-14 17:52 PDT (History)
1 user (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
Proposed fix (10.99 KB, patch)
2012-08-13 10:05 PDT, Jon Coppeard (:jonco)
wmccloskey: review+
Details | Diff | Splinter Review

Description Jon Coppeard (:jonco) 2012-08-13 10:05:08 PDT
In preparation incremental compartment sweeping, separate out the 'scheduled for GC' flag from 'incremental GC in progress' flag for compartments.
Comment 1 Jon Coppeard (:jonco) 2012-08-13 10:05:55 PDT
Created attachment 651441 [details] [diff] [review]
Proposed fix
Comment 2 [PTO to Dec5] Bill McCloskey (:billm) 2012-08-13 16:45:00 PDT
Comment on attachment 651441 [details] [diff] [review]
Proposed fix

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

::: js/src/jscompartment.h
@@ +164,5 @@
>  
>    private:
>      enum CompartmentGCState {
> +        NO_GC,
> +        COLLECTING

We're trying to transition to lowercase names for stuff like this. Can you use NoGC and Collecting?

::: js/src/jsgc.cpp
@@ +3190,5 @@
> +     */
> +    DebugOnly<bool> any = false;
> +    for (CompartmentsIter c(rt); !c.done(); c.next()) {
> +        JS_ASSERT(!c->isCollecting());
> +        for (unsigned i = 0 ; i < FINALIZE_LIMIT ; ++i)

No spaces before the semicolons.

@@ +3198,5 @@
> +    }
> +    JS_ASSERT(any);
> +
> +    /*
> +     * Set up which compartments will be collected.

Can just be one line: /* Set up which compartments will be collected. */

@@ +3201,5 @@
> +    /*
> +     * Set up which compartments will be collected.
> +     */
> +    rt->gcIsFull = true;
> +    for (CompartmentsIter c(rt); !c.done(); c.next()) {

Can this loop be combined with the previous one?

@@ +3670,5 @@
>      for (CompartmentsIter c(rt); !c.done(); c.next()) {
>          c->setGCLastBytes(c->gcBytes, c->gcMallocAndFreeBytes, gckind);
>          if (c->wasGCStarted())
> +            c->setCollecting(false);
> +

Extra space here.
Comment 4 Ryan VanderMeulen [:RyanVM] 2012-08-14 17:52:46 PDT
https://hg.mozilla.org/mozilla-central/rev/ac17d5c3d370

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