The default bug view has changed. See this FAQ.

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

RESOLVED FIXED in mozilla17

Status

()

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

People

(Reporter: jonco, Assigned: jonco)

Tracking

(Depends on: 1 bug)

Trunk
mozilla17
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(3 attachments, 1 obsolete attachment)

(Assignee)

Description

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

Updated

5 years ago
Assignee: general → jcoppeard
(Assignee)

Comment 1

5 years ago
Created attachment 652125 [details] [diff] [review]
Part 1 -  sweep background things at the end
Attachment #652125 - Flags: review?(wmccloskey)
(Assignee)

Comment 2

5 years ago
Created attachment 652126 [details] [diff] [review]
Part 2 - removed unused member from FreeOp
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.
(Assignee)

Comment 5

5 years ago
Created attachment 652784 [details] [diff] [review]
Part 1 - sweep background things at the end v2
Attachment #652125 - Attachment is obsolete: true
(Assignee)

Comment 6

5 years ago
Created attachment 652801 [details] [diff] [review]
Part 1 - sweep background things at the end v3
(Assignee)

Updated

5 years ago
Depends on: 779897
(Assignee)

Comment 7

5 years ago
https://hg.mozilla.org/integration/mozilla-inbound/rev/a1e67b8915a0
https://hg.mozilla.org/integration/mozilla-inbound/rev/e6fe39185cd4
https://hg.mozilla.org/mozilla-central/rev/a1e67b8915a0
https://hg.mozilla.org/mozilla-central/rev/e6fe39185cd4
Status: NEW → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla17

Updated

5 years ago
Depends on: 787879
You need to log in before you can comment on or make changes to this bug.