Last Comment Bug 677431 - GC: MarkAndSweep cleanups
: GC: MarkAndSweep cleanups
Status: RESOLVED FIXED
:
Product: Core
Classification: Components
Component: JavaScript Engine (show other bugs)
: unspecified
: x86 Linux
: -- normal (vote)
: mozilla9
Assigned To: [PTO to Dec5] Bill McCloskey (:billm)
:
: Jason Orendorff [:jorendorff]
Mentors:
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2011-08-08 17:30 PDT by [PTO to Dec5] Bill McCloskey (:billm)
Modified: 2011-09-23 04:55 PDT (History)
4 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
patch (12.34 KB, patch)
2011-08-08 17:30 PDT, [PTO to Dec5] Bill McCloskey (:billm)
no flags Details | Diff | Splinter Review
patch v2 (16.92 KB, patch)
2011-08-09 12:38 PDT, [PTO to Dec5] Bill McCloskey (:billm)
igor: review+
Details | Diff | Splinter Review

Description [PTO to Dec5] Bill McCloskey (:billm) 2011-08-08 17:30:50 PDT
Created attachment 551637 [details] [diff] [review]
patch

This patch breaks up MarkAndSweep and simplifies some of the logic. Mostly I want this for incremental GC, where we run the different phases at different times.

The way that the probes are called changes a little bit. Steve Fink said this was okay (and he's making similar changes in bug 673631, which will probably land first).
Comment 1 Igor Bukanov 2011-08-09 01:20:54 PDT
Comment on attachment 551637 [details] [diff] [review]
patch

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

::: js/src/jsgc.cpp
@@ +2306,5 @@
>      /* Collect watch points associated with unreachable objects. */
>      WatchpointMap::sweepAll(rt);
>  
> +    Probes::GCStartSweepPhase(NULL);
> +    PER_COMPARTMENT_OP(rt, Probes::GCStartSweepPhase(c));

On few occasions I have to step through the sweep loops that patch turns into the macros in the debugger. This change brings inconvenience. 

So replace PER_COMPARTMENT_OP with an iterator class that iterates over one or all compartments and use it here. A possible debugger-friendly usage would be (any other iterator method names and interface details would work just fine): 

for (GCCompartmentIter c(rt); !c.done(); c.next())
    Probes::GCStartSweepPhase(c);

@@ +2315,4 @@
>      /*
> +     * Order of finalization is important. We need to ensure that a finalizer
> +     * doesn't touch a GC thing after it has been swept. Also, we should not
> +     * call IsAboutToBeFinalized on something that has already been swept.

The current comment at least explains why the order is import. In addition the shape finalization calls IsAboutToBeFinalized in the form of Cell::isMarked and that should work for already finalized shapes. So for now keep the current comment.

@@ +2385,5 @@
> +
> +    BeginMarkPhase(cx, &gcmarker, comp, gckind);
> +    MarkPhase(cx, &gcmarker, comp, gckind);
> +    EndMarkPhase(cx, &gcmarker, comp, gckind);
> +    SweepPhase(cx, &gcmarker, comp, gckind);

Do you plan to remove JSRuntime::gcCurrentCompartment? If not, remove the comp argument from the 4 calls above and from MarkAndSweep while moving the asserts at the start of MarkAndSweep to GCCycle.
Comment 2 [PTO to Dec5] Bill McCloskey (:billm) 2011-08-09 12:38:35 PDT
Created attachment 551859 [details] [diff] [review]
patch v2

Thanks for the suggestions. I like the iterator a lot better than the macro.

I still made some changes to the comment, but I left it closer to the original version. I took out the sentence about property trees, since all that code has been overhauled and I don't think the statement is true anymore. I moved the sentence about sweeping compartments down to the place where we actually sweep compartments.
Comment 3 Andrew McCreight [:mccr8] 2011-08-09 12:39:26 PDT
I didn't look at the patch in much detail, but I like the reorganization!
Comment 4 [PTO to Dec5] Bill McCloskey (:billm) 2011-08-09 12:43:13 PDT
I'm thinking it might be nice to add an AllCompartmentsIter to jscntxt.h. The way we iterate over compartments right now is a little awkward because of dealing with the JSCompartment**. That could be a separate bug, though.
Comment 5 Igor Bukanov 2011-08-09 13:03:54 PDT
Comment on attachment 551859 [details] [diff] [review]
patch v2

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

::: js/src/jsgc.cpp
@@ +1796,5 @@
>  
> +class GCCompartmentsIter {
> +  private:
> +    JSRuntime *runtime;
> +    JSCompartment **it;

The implementation can be simplified if the runtime field is replaced by JSCompartment **end. Then for the rt->gcCurrentCompartment case "it" and "end" are initialized to &rt->gcCurrentCompartment and &rt->gcCurrentCompartment + 1.

@@ +1855,1 @@
>  #endif

Nice!
Comment 6 [PTO to Dec5] Bill McCloskey (:billm) 2011-09-22 13:36:26 PDT
https://hg.mozilla.org/integration/mozilla-inbound/rev/8a77bab70ad5
Comment 7 Ed Morley [:emorley] 2011-09-23 04:55:14 PDT
https://hg.mozilla.org/mozilla-central/rev/8a77bab70ad5

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