The default bug view has changed. See this FAQ.

GC: Incremental sweeping of atoms table

RESOLVED FIXED in mozilla17

Status

()

Core
JavaScript Engine
--
enhancement
RESOLVED FIXED
5 years ago
5 years ago

People

(Reporter: jonco, Unassigned)

Tracking

unspecified
mozilla17
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [Snappy])

Attachments

(2 attachments, 2 obsolete attachments)

(Reporter)

Description

5 years ago
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.
(Reporter)

Updated

5 years ago
QA Contact: jcoppeard
(Reporter)

Comment 1

5 years ago
Created attachment 647576 [details] [diff] [review]
Current patch
(Reporter)

Comment 2

5 years ago
Created attachment 647990 [details] [diff] [review]
Proposed fix
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+
(Reporter)

Comment 4

5 years ago
(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.
(Reporter)

Comment 5

5 years ago
https://hg.mozilla.org/integration/mozilla-inbound/rev/0d0cf69c69a8
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.
(Reporter)

Comment 7

5 years ago
Created attachment 653415 [details] [diff] [review]
Add per-compartment mark/sweep state
Attachment #647990 - Attachment is obsolete: true
Attachment #653415 - Flags: review?(wmccloskey)
(Reporter)

Comment 8

5 years ago
Created attachment 653416 [details] [diff] [review]
Part 2 - sweep atoms compartment at the end
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?
(Reporter)

Comment 12

5 years ago
https://hg.mozilla.org/integration/mozilla-inbound/rev/271c3965015e
https://hg.mozilla.org/integration/mozilla-inbound/rev/a8785c8a603a
https://hg.mozilla.org/mozilla-central/rev/271c3965015e
https://hg.mozilla.org/mozilla-central/rev/a8785c8a603a
Status: NEW → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla17
You need to log in before you can comment on or make changes to this bug.