Closed Bug 779183 Opened 12 years ago Closed 12 years ago

GC: Incremental sweeping of atoms table

Categories

(Core :: JavaScript Engine, enhancement)

enhancement
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla17

People

(Reporter: jonco, Unassigned)

Details

(Whiteboard: [Snappy])

Attachments

(2 files, 2 obsolete files)

There's a couple of things we can do to improve collection of atoms.

1) Currently, if we don't want to collect atoms we mark them all so that they don't get thrown away when they are swept.  Instead, we can not mark or sweep them at all.

2) We can sweep the atoms table in a separate slice after the last mark slice if we take care not to return references to finalized atoms back to the called before this point.

Tracing shows that there can be > 150000 atoms in use by a fairly light browser use, and time spent sweeping the atoms table in this case can be nearly 10ms so separating this out into a separate slice is a sensible thing to do.
QA Contact: jcoppeard
Attached patch Current patch (obsolete) — Splinter Review
Attached patch Proposed fix (obsolete) — Splinter Review
Attachment #647576 - Attachment is obsolete: true
Attachment #647990 - Flags: review?(wmccloskey)
Whiteboard: [Snappy]
Comment on attachment 647990 [details] [diff] [review]
Proposed fix

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

This looks nice. Feel free to check in when the issues below are fixed.

::: js/src/jsatom.cpp
@@ +184,5 @@
>      FreeOp fop(rt, false, false);
> +    for (AtomSet::Range r = state->atoms.all(); !r.empty(); r.popFront()) {
> +        JSAtom *atom = r.front().asPtr();
> +        if (atom)
> +            atom->finalize(&fop);

This change shouldn't be needed. We'll never run this code when there's a GC running.

@@ +245,2 @@
>  
> +        if (!atom)

Could you maybe put a comment here that a NULL result from asPtr means that the atom was unmarked?

@@ +247,3 @@
>              e.removeFront();
>          else
>              e.rekeyFront(AtomHasher::Lookup(atom), AtomStateEntry(atom, entry.isTagged()));

There's no need for the rekey anymore, so you can take it out.

::: js/src/jsgc.cpp
@@ +3715,5 @@
> +     * atoms. Otherwise, the non-collected compartments could contain pointers
> +     * to atoms that we would miss.
> +     */
> +    if (rt->gcKeepAtoms || !all)
> +        rt->atomsCompartment->setCollecting(false);

Could you move this code to Collect? There's already some existing code there that decides which compartments to collect. I like to have this stuff as close to the start of the GC as possible so that flags like needsBarrier() and isGCScheduled() don't disagree.

Also, please do this only if gcIncrementalState is NO_INCREMENTAL (i.e., the first slice). If we start collecting the atoms compartment and then later on new compartments are created, it's still safe to sweep the atoms compartment. Any atoms used by the new compartment will have been marked by a barrier.

@@ +3950,5 @@
>  
>          EndMarkPhase(rt, isIncremental);
> +        BeginSweepPhase(rt);
> +
> +        rt->gcIncrementalState = rt->atomsCompartment->isCollecting() ? SWEEP_ATOMS : SWEEP;

If we don't yield, this will fall through to SWEEP_ATOMS in either case. It seems easiest to me to go to SWEEP_ATOMS always, but have it do nothing if we're not collecting the atoms compartment.

@@ +3976,5 @@
> +        /* fall through */
> +      }
> +
> +      case SWEEP_ATOMS: {
> +        gcstats::AutoPhase ap2(rt->gcStats, gcstats::PHASE_SWEEP_ATOMS);

This can be called ap. It only needs to be called ap2 if there's one nested inside another (yeah, gross).
Attachment #647990 - Flags: review?(wmccloskey) → review+
(In reply to Bill McCloskey (:billm) from comment #3)

Thanks for the comments.

> > +    if (rt->gcKeepAtoms || !all)
> > +        rt->atomsCompartment->setCollecting(false);
> 
> Could you move this code to Collect?

In the end I moved it to BeginMarkPhase() because I realised we need to check this again if we restart the GC due to call ResetIncrementalGC().

I agree we need to tidy up all the state.

> > +        rt->gcIncrementalState = rt->atomsCompartment->isCollecting() ? SWEEP_ATOMS : SWEEP;
> 
> If we don't yield, this will fall through to SWEEP_ATOMS in either case. 

Yeah this was wrong, I meant it to jump to the SWEEP state if atoms were not being collected.  Fixed as suggested.
Attachment #647990 - Attachment is obsolete: true
Attachment #653415 - Flags: review?(wmccloskey)
Attachment #653416 - Flags: review?(wmccloskey)
Comment on attachment 653415 [details] [diff] [review]
Add per-compartment mark/sweep state

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

Excellent!
Attachment #653415 - Flags: review?(wmccloskey) → review+
Comment on attachment 653416 [details] [diff] [review]
Part 2 - sweep atoms compartment at the end

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

::: js/src/jsgc.cpp
@@ +3822,5 @@
> +
> +    JS_ASSERT(c->isGCMarking());
> +    c->setGCState(JSCompartment::Sweep);
> +
> +    c->arenas.purge();

Can you not do the purge for the atomsCompartment in BeginSweepPhase.

@@ +3832,5 @@
> +
> +    FreeOp fop(rt, rt->gcSweepOnBackgroundThread);
> +
> +    PartitionCompartments partition(rt);
> +    partition.partition();

No need to include the SCC stuff here. I don't expect that this will count for much time, and the SCC numbering may be different than it was before, which could screw up the measurements.
Attachment #653416 - Flags: review?(wmccloskey) → review+
Bill, in your GC^2 measurements, how long does this tend to take?
https://hg.mozilla.org/mozilla-central/rev/271c3965015e
https://hg.mozilla.org/mozilla-central/rev/a8785c8a603a
Status: NEW → RESOLVED
Closed: 12 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla17
You need to log in before you can comment on or make changes to this bug.