Closed Bug 677431 Opened 11 years ago Closed 11 years ago

GC: MarkAndSweep cleanups

Categories

(Core :: JavaScript Engine, defect)

x86
Linux
defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla9

People

(Reporter: billm, Assigned: billm)

Details

Attachments

(1 file, 1 obsolete file)

Attached patch patch (obsolete) — Splinter Review
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).
Attachment #551637 - Flags: review?(igor)
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.
Attachment #551637 - Flags: review?(igor)
Attached patch patch v2Splinter Review
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.
Attachment #551637 - Attachment is obsolete: true
Attachment #551859 - Flags: review?(igor)
I didn't look at the patch in much detail, but I like the reorganization!
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 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!
Attachment #551859 - Flags: review?(igor) → review+
https://hg.mozilla.org/mozilla-central/rev/8a77bab70ad5
Status: NEW → RESOLVED
Closed: 11 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.