677431 - GC: MarkAndSweep cleanups

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

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

Attachment: 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]

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

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

Attachment: 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]]

I didn't look at the patch in much detail, but I like the reorganization!

Comment 4

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

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]

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

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

https://hg.mozilla.org/integration/mozilla-inbound/rev/8a77bab70ad5

Comment 7

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/8a77bab70ad5