936514 - GC: In source documentation on the collector needs updating

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Jon Coppeard (:jonco)]

The documentation comment at the start of jsgc.cpp is pretty out of date, and could use expanding to cover full/zone GCs, incremental sweeping and incremental resets.

Comment 1

[Tom S [:evilpie]]

Also see bug 928378.

Comment 2

[Jon Coppeard (:jonco)]

Attachment: gc-doc

Here's some documentation for the file.

Comment 3

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

Comment on attachment 8337706 [details] [diff] [review]
gc-doc

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

Thanks, this is great!

::: js/src/jsgc.cpp
@@ +32,5 @@
> + *  - the collection must be run by calling js::GCSlice() rather than js::GC()
> + *  - the GC mode must have been set to JSGC_MODE_INCREMENTAL with
> + *    JS_SetGCParameter()
> + *  - no thread may have an AutoKeepAtoms instance on the stack
> + *  - there are no exclusive threads using the runtime

I think we are able to do incremental GC if exclusive threads are present.

@@ +34,5 @@
> + *    JS_SetGCParameter()
> + *  - no thread may have an AutoKeepAtoms instance on the stack
> + *  - there are no exclusive threads using the runtime
> + *  - incremental GC must not have been disabled by calling
> + *    JS::DisableIncrementalGC()

Looks like this function doesn't have any callers. Maybe you should mention the  JSCLASS_IMPLEMENTS_BARRIERS thing though. That's internal to the engine.

@@ +51,5 @@
> + *
> + * The collector proceeds through the following states, the current state being
> + * held in JSRuntime::gcIncrementalState:
> + *
> + *  - MARK_ROOTS - marks all roots

Maybe "marks the stack and other roots"

@@ +75,5 @@
> + *
> + *          ... JS code runs ...
> + *
> + * Slice n:   MARK:       Mark stack is completely drained.
> + *            SWEEP:      Sweeping is started and some sweeping done.

How about "Select first group of zones to sweep and sweep them."

@@ +79,5 @@
> + *            SWEEP:      Sweeping is started and some sweeping done.
> + *
> + *          ... JS code runs ...
> + *
> + * Slice n+1: SWEEP:      Sweep zone groups and continue marking unswept zones.

"Mark objects in unswept zones that were newly identified as alive (see below). Then sweep more zone groups."

@@ +100,3 @@
>   *
> + * Incremental collection requires close collaboration with the mutator (i.e.,
> + * JS code) to garantee correctness. The problem is that between slices the

garantee

@@ +107,5 @@
> + * We use a snapshot-at-the-beginning algorithm to do this. This means that we
> + * promise to mark at least everything that is reachable at the beginning of
> + * collection. To implement it we mark the old contents of every non-root memory
> + * location written to by the mutator while the collection is in progress, using
> + * write barriers. This is described in gc/Barrier.h.

The old comment had more detail on write barriers. Can you include more of that, or merge them somehow?

@@ +119,5 @@
> + * mutator code was allowed to run after the start of sweeping, it could observe
> + * the state of the cache and create a new reference to an object that was just
> + * about to be destroyed.
> + *
> + * Sweeping all finializable objects in one go would introduce long pauses, so

finializable

@@ +121,5 @@
> + * about to be destroyed.
> + *
> + * Sweeping all finializable objects in one go would introduce long pauses, so
> + * instead sweeping broken up into groups of zones. Zones which are not yet
> + * being swept are still marked, so the issue above does not apply. The order of

It also might be easier to explain this with an example. Say that object |a| from zone A points to object |b| in zone B and neither object was marked when we transitioned to the SWEEP phase. Imagine we sweep B first and then return to the mutator. It's possible that the mutator could cause |a| to become alive through a read barrier (perhaps it was a shape that was accessed via a shape table). Then we would need to mark |b|, which |a| points to, but |b| has already been swept.

@@ +141,5 @@
> + * that incremental collection is no longer possible. In this case, the collection
> + * is 'reset' by ResetIncrementalGC(). If we are in the mark state, this just
> + * stops marking, but if we have started sweeping already, we continue until we
> + * have swept the current zone group. Following a reset, a new non-incremental
> + * collection is started.

Nowadays I think we reset only in extreme cases. Most of the time we just finish the GC in one last slice. Can you mention that?

Comment 4

[Luke Wagner [:luke]]

Comment on attachment 8337706 [details] [diff] [review]
gc-doc

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

Great!  Naive questions that perhaps the comment could answer:

::: js/src/jsgc.cpp
@@ +21,5 @@
> + * the runtime.
> + *
> + * It is possible for an incremental collection that started out as a full GC to
> + * become a zone GC if new zones are created during the course of the
> + * collection.

Can a zone GC become a full GC (I thought maybe this happened sometimes on memory pressure...)?

@@ +141,5 @@
> + * that incremental collection is no longer possible. In this case, the collection
> + * is 'reset' by ResetIncrementalGC(). If we are in the mark state, this just
> + * stops marking, but if we have started sweeping already, we continue until we
> + * have swept the current zone group. Following a reset, a new non-incremental
> + * collection is started.

I'd be curious to hear what the extreme cases are.

I'd also be curious to hear what the cases that cause us to finish the GC in one last slice are.

Comment 5

[Jon Coppeard (:jonco)]

Thanks for the comments!

https://hg.mozilla.org/integration/mozilla-inbound/rev/9a11d5c81bbd

Comment 6

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/9a11d5c81bbd