779183 - GC: Incremental sweeping of atoms table

Status: RESOLVED FIXED

(Core :: JavaScript Engine, enhancement)

Description

[Jon Coppeard (:jonco)]

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.

Comment 1

[Jon Coppeard (:jonco)]

Attachment: Current patch

Comment 2

[Jon Coppeard (:jonco)]

Attachment: Proposed fix

Comment 3

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

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).

Comment 4

[Jon Coppeard (:jonco)]

(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.

Comment 5

[Jon Coppeard (:jonco)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/0d0cf69c69a8

Comment 6

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

Sorry, it looks like this was causing Talos crashes. I had to back it out.

https://hg.mozilla.org/integration/mozilla-inbound/rev/2d5c8c401f28

Here are some of the logs:
https://tbpl.mozilla.org/php/getParsedLog.php?id=14071447&tree=Mozilla-Inbound&full=1
https://tbpl.mozilla.org/php/getParsedLog.php?id=14072368&tree=Mozilla-Inbound&full=1
https://tbpl.mozilla.org/php/getParsedLog.php?id=14074395&tree=Mozilla-Inbound&full=1
https://tbpl.mozilla.org/php/getParsedLog.php?id=14073592&tree=Mozilla-Inbound&full=1

Talos is a performance test, so we only run it on opt builds.

Comment 7

[Jon Coppeard (:jonco)]

Attachment: Add per-compartment mark/sweep state

Comment 8

[Jon Coppeard (:jonco)]

Attachment: Part 2 - sweep atoms compartment at the end

Comment 9

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

Comment on attachment 653415 [details] [diff] [review]
Add per-compartment mark/sweep state

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

Excellent!

Comment 10

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

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.

Comment 11

[Andrew McCreight [:mccr8]]

Bill, in your GC^2 measurements, how long does this tend to take?

Comment 12

[Jon Coppeard (:jonco)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/271c3965015e
https://hg.mozilla.org/integration/mozilla-inbound/rev/a8785c8a603a

Comment 13

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/271c3965015e
https://hg.mozilla.org/mozilla-central/rev/a8785c8a603a