Last Comment Bug 779183 - GC: Incremental sweeping of atoms table
: GC: Incremental sweeping of atoms table
Status: RESOLVED FIXED
[Snappy]
:
Product: Core
Classification: Components
Component: JavaScript Engine (show other bugs)
: unspecified
: All All
: -- enhancement (vote)
: mozilla17
Assigned To: general
: Jon Coppeard (:jonco)
:
Mentors:
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2012-07-31 09:50 PDT by Jon Coppeard (:jonco)
Modified: 2012-08-23 03:51 PDT (History)
3 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
Current patch (17.92 KB, patch)
2012-07-31 09:53 PDT, Jon Coppeard (:jonco)
no flags Details | Diff | Splinter Review
Proposed fix (15.04 KB, patch)
2012-08-01 09:30 PDT, Jon Coppeard (:jonco)
wmccloskey: review+
Details | Diff | Splinter Review
Add per-compartment mark/sweep state (8.14 KB, patch)
2012-08-20 09:54 PDT, Jon Coppeard (:jonco)
wmccloskey: review+
Details | Diff | Splinter Review
Part 2 - sweep atoms compartment at the end (10.49 KB, patch)
2012-08-20 09:55 PDT, Jon Coppeard (:jonco)
wmccloskey: review+
Details | Diff | Splinter Review

Description Jon Coppeard (:jonco) 2012-07-31 09:50:55 PDT
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) 2012-07-31 09:53:53 PDT
Created attachment 647576 [details] [diff] [review]
Current patch
Comment 2 Jon Coppeard (:jonco) 2012-08-01 09:30:46 PDT
Created attachment 647990 [details] [diff] [review]
Proposed fix
Comment 3 Bill McCloskey (:billm) 2012-08-01 18:48:47 PDT
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) 2012-08-02 07:02:02 PDT
(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 7 Jon Coppeard (:jonco) 2012-08-20 09:54:12 PDT
Created attachment 653415 [details] [diff] [review]
Add per-compartment mark/sweep state
Comment 8 Jon Coppeard (:jonco) 2012-08-20 09:55:00 PDT
Created attachment 653416 [details] [diff] [review]
Part 2 - sweep atoms compartment at the end
Comment 9 Bill McCloskey (:billm) 2012-08-21 08:53:13 PDT
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 (:billm) 2012-08-21 09:03:31 PDT
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] 2012-08-21 09:04:54 PDT
Bill, in your GC^2 measurements, how long does this tend to take?

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