782993 - GC: Always sweep background finalized things at the same point in collection

Status: RESOLVED FIXED

(Core :: JavaScript Engine, enhancement)

Description

[Jon Coppeard (:jonco)]

At present, finalization of things that can be finalized on the background thread may happen at a different time relative to other parts of sweeping depending on whether they are in fact finalized in the background.

Specifically, if they are finalized on the main thread then this will happen at the start of the sweep phase, whereas otherwise they are finalized after the end of the sweep phase.

Most of the time such objects will be finalized in the background, but there are situations where this does not happen:
 - engine shutdown
 - engine is built without thread support

This is undesirable because the sweeping code must handle both possibilites, but one is only rarely exercised.

The proposed solution is to finalize these objects at the same point in collection regardless of which thread they are being finalized from, and to unify the code path as much as possible.

Comment 1

[Jon Coppeard (:jonco)]

Attachment: Part 1 - sweep background things at the end

Comment 2

[Jon Coppeard (:jonco)]

Attachment: Part 2 - removed unused member from FreeOp

Comment 3

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

Comment on attachment 652125 [details] [diff] [review]
Part 1 -  sweep background things at the end

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

This looks nice!

::: js/src/jsgc.cpp
@@ -1681,5 @@
> -ArenaLists::backgroundFinalize(FreeOp *fop, ArenaHeader *listHead)
> -{
> -#ifdef JS_THREADSAFE
> -    JS_ASSERT(fop->onBackgroundThread());
> -#endif /* JS_THREADSAFE */

Rather than removing this, could you make it an ASSERT_IF(onBackgroundThread, ...)?

@@ +2803,5 @@
>          DecommitArenas(rt);
>  }
>  
> +static void
> +SweepBackgroundThings(JSRuntime* rt, bool onBackgroundThread)

JSRuntime *rt

@@ +2810,5 @@
> +     * We must finalize in the insert order, see comments in
> +     * finalizeObjects.
> +     */
> +    FreeOp fop(rt, false, false);
> +    for (int phase = 0 ; phase < BackgroundPhaseCount ; ++phase) {

No space before semicolons.

@@ +2812,5 @@
> +     */
> +    FreeOp fop(rt, false, false);
> +    for (int phase = 0 ; phase < BackgroundPhaseCount ; ++phase) {
> +        for (GCCompartmentsIter c(rt); !c.done(); c.next()) {
> +            for (int index = 0 ; index < BackgroundPhaseLength[phase] ; ++index) {

Same.

@@ +2815,5 @@
> +        for (GCCompartmentsIter c(rt); !c.done(); c.next()) {
> +            for (int index = 0 ; index < BackgroundPhaseLength[phase] ; ++index) {
> +                AllocKind kind = BackgroundPhases[phase][index];
> +                ArenaHeader *arenas = c->arenas.arenaListsToSweep[kind];
> +                if (arenas) {

No brace needed.

@@ +4048,5 @@
>  
>          if (rt->gcSweepOnBackgroundThread)
>              rt->gcHelperThread.startBackgroundSweep(gckind == GC_SHRINK);
>          else
>              FinishCollection(rt);

Here's an idea. Instead of this,
1. call SweepPhase
2. call EndSweepPhase
3. Call SweepForegroundOrBackground (name kinda sucks, maybe you can think of a better one)

SweepForegroundOrBackground would start background sweeping if rt->gcSweepOnBackgroundThread. Otherwise it would call SweepBackgroundThings followed by ExpireChunksAndArenas and rt->freeLifoAlloc.freeAll() (both moved from EndSweepPhase). The idea is that this function should have a roughly parallel structure to GCHelperThread::doSweep.

Comment 4

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

Never mind about that onBackgroundThread assert. I guess you're taking it out.

Comment 5

[Jon Coppeard (:jonco)]

Attachment: Part 1 - sweep background things at the end v2

Comment 6

[Jon Coppeard (:jonco)]

Attachment: Part 1 - sweep background things at the end v3

Comment 7

[Jon Coppeard (:jonco)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/a1e67b8915a0
https://hg.mozilla.org/integration/mozilla-inbound/rev/e6fe39185cd4

Comment 8

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/a1e67b8915a0
https://hg.mozilla.org/mozilla-central/rev/e6fe39185cd4