Closed Bug 782993 Opened 8 years ago Closed 8 years ago

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

Categories

(Core :: JavaScript Engine, enhancement)

enhancement
Not set

Tracking

()

RESOLVED FIXED
mozilla17

People

(Reporter: jonco, Assigned: jonco)

References

(Depends on 1 open bug)

Details

Attachments

(3 files, 1 obsolete file)

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.
Assignee: general → jcoppeard
Attachment #652125 - Flags: review?(wmccloskey)
Attachment #652126 - Flags: review?(wmccloskey)
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.
Attachment #652125 - Flags: review?(wmccloskey) → review+
Attachment #652126 - Flags: review?(wmccloskey) → review+
Never mind about that onBackgroundThread assert. I guess you're taking it out.
Attachment #652125 - Attachment is obsolete: true
Depends on: 779897
https://hg.mozilla.org/mozilla-central/rev/a1e67b8915a0
https://hg.mozilla.org/mozilla-central/rev/e6fe39185cd4
Status: NEW → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla17
Depends on: 787879
You need to log in before you can comment on or make changes to this bug.