Last Comment Bug 782993 - GC: Always sweep background finalized things at the same point in collection
: GC: Always sweep background finalized things at the same point in collection
Status: RESOLVED FIXED
:
Product: Core
Classification: Components
Component: JavaScript Engine (show other bugs)
: Trunk
: All All
: -- enhancement (vote)
: mozilla17
Assigned To: Jon Coppeard (:jonco)
:
: Jason Orendorff [:jorendorff]
Mentors:
Depends on: 787879 779897
Blocks:
  Show dependency treegraph
 
Reported: 2012-08-15 09:34 PDT by Jon Coppeard (:jonco)
Modified: 2012-09-03 00:22 PDT (History)
1 user (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
Part 1 - sweep background things at the end (10.33 KB, patch)
2012-08-15 09:35 PDT, Jon Coppeard (:jonco)
wmccloskey: review+
Details | Diff | Splinter Review
Part 2 - removed unused member from FreeOp (7.30 KB, patch)
2012-08-15 09:36 PDT, Jon Coppeard (:jonco)
wmccloskey: review+
Details | Diff | Splinter Review
Part 1 - sweep background things at the end v2 (12.99 KB, patch)
2012-08-17 08:53 PDT, Jon Coppeard (:jonco)
no flags Details | Diff | Splinter Review
Part 1 - sweep background things at the end v3 (16.64 KB, patch)
2012-08-17 09:56 PDT, Jon Coppeard (:jonco)
no flags Details | Diff | Splinter Review

Description Jon Coppeard (:jonco) 2012-08-15 09:34:33 PDT
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) 2012-08-15 09:35:34 PDT
Created attachment 652125 [details] [diff] [review]
Part 1 -  sweep background things at the end
Comment 2 Jon Coppeard (:jonco) 2012-08-15 09:36:53 PDT
Created attachment 652126 [details] [diff] [review]
Part 2 - removed unused member from FreeOp
Comment 3 Bill McCloskey (:billm) 2012-08-16 22:39:42 PDT
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 (:billm) 2012-08-16 22:41:31 PDT
Never mind about that onBackgroundThread assert. I guess you're taking it out.
Comment 5 Jon Coppeard (:jonco) 2012-08-17 08:53:51 PDT
Created attachment 652784 [details] [diff] [review]
Part 1 - sweep background things at the end v2
Comment 6 Jon Coppeard (:jonco) 2012-08-17 09:56:28 PDT
Created attachment 652801 [details] [diff] [review]
Part 1 - sweep background things at the end v3

Note You need to log in before you can comment on or make changes to this bug.