1351405 - Simplify the GC sweep phase implementation

Status: RESOLVED FIXED

(Core :: JavaScript: GC, enhancement)

Description

[Jon Coppeard (:jonco)]

At the moment GCRuntime::sweepPhase is very complicated and mixes up sweeping code with code that handles yielding to the mutator and resuming later on.

We can clean this up making it more data driven.  If we have a list of sweep actions we can split the driver code which handles yielding and resuming from the code that actually does the sweeping.

Comment 1

[Jon Coppeard (:jonco)]

Attachment: bug1351405-simplify-sweep-phase

As discussed, make incremental sweeping data driven.

Comment 2

[Steve Fink [:sfink] [:s:]]

Comment on attachment 8852416 [details] [diff] [review]
bug1351405-simplify-sweep-phase

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

Wow, that came out really nice. My initial reaction was that you could set all this up with a static initializer, but with the looping over AllocKinds and things, I agree with the way you have it here.

::: js/src/gc/GCRuntime.h
@@ +1216,5 @@
>       * Incremental sweep state.
>       */
>      ActiveThreadData<JS::Zone*> sweepGroups;
>      ActiveThreadOrGCTaskData<JS::Zone*> currentSweepGroup;
> +    ActiveThreadData<size_t> sweepPhase;

Having this be a size_t seemed weird to me until I realized that it's an index. I know it's long, but how about sweepPhaseIndex and sweepActionIndex?

Or go all out and do

  struct { phaseIndex, actionIndex } incrementalSweepState; // incrementalSweepProgress?

::: js/src/jsgc.cpp
@@ +393,5 @@
>          }
>      }
>  };
>  
> +// Incremental sweeping is controlled by a list of actions that descibe what

*describe

@@ +396,5 @@
>  
> +// Incremental sweeping is controlled by a list of actions that descibe what
> +// happens and in what order. Due to the incremental nature of sweeping an
> +// action does not necessarily run to completion so the current state is tracked
> +// in the GCRuntime by sweepPhase() method.

by *the

@@ +421,5 @@
> +
> +using SweepActionVector = Vector<SweepAction, 0, SystemAllocPolicy>;
> +using SweepPhaseVector = Vector<SweepActionVector, 0, SystemAllocPolicy>;
> +
> +SweepPhaseVector SweepPhases;

static

@@ +433,5 @@
> +void
> +js::gc::ShutDownStaticData()
> +{
> +    SweepPhases.clearAndFree();
> +}

Why is the shutdown needed? Won't ~Vector take care of this?

Comment 3

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/5df436b84953
Simplify incremental sweeping implementation r=sfink

Comment 4

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/5df436b84953