1376096 - Split sweep actions into per-sweep-group and per-zone actions

Status: RESOLVED FIXED

(Core :: JavaScript: GC, enhancement)

Description

[Jon Coppeard (:jonco)]

Attachment: per-group-sweep-actions

Currently we have a list of per-zone actions that are grouped into phases and run for every zone in a sweep group.

For incremental table sweeping we'll need a list of per-sweep-group actions since we want to bet able to sweep all tables in a sweep group at the same if possible.

This patch adds this and uses it for sweeping the atoms table which didn't need to be run per-zone anyway.

Comment 1

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

Comment on attachment 8881031 [details] [diff] [review]
per-group-sweep-actions

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

::: js/src/jsgc.cpp
@@ +432,3 @@
>  //       for each zone in sweep group:
>  //         for each action in phase:
>  //           perform_action

I had to read through this several times, especially since it's not clear from the description where things are interruptible. I found myself wondering what the continuation has to store (ie, what state you have to track to be able to resume), since that would tell me what the resume points are.

Maybe annotate this pseudocode with at least the SweepState settings:

   [SweepState -> Init]
   for each sweep group:
     [SweepState -> RunPerSweepGroupActions]
     for each per-sweep-group action:
       perform_action
     [SweepState -> RunPerZoneActions]
     for each per-zone phase:
       for each zone in sweep group:	
         for each action in phase:	
           perform_action

and then maybe say that this can be interrupted anywhere, by any action returning NotFinished.

But really, SweepState is just the part of the continuation that gives the outermost state. I think it might be easier to follow if you combined the full continuation together. I'm not sure what to call it, since something need one more name, but maybe

  struct SweepContinuation {
    SweepState state;

    // valid for RunPerSweepGroupActions and RunPerZoneActions
    size_t actionIndex;

    // valid for RunPerZoneActions
    size_t phaseIndex;
    Zone* zone;
  }

"continuation" is kind of an awful name, I guess. Maybe the whole thing could be SweepState, and SweepState could become... uh... "mode"? "pass"? I don't love any of those. If it were "progress", you could rename IncrementSweepState to AdvanceSweepProgress or something, and I don't like the "increment" part much. AdvanceSweep{Mode,Pass} or NextSweep{Mode,Pass} would work too.

And I guess those all have to be ActiveThreadData<T>.

Comment 2

[Jon Coppeard (:jonco)]

(In reply to Steve Fink [:sfink] [:s:] from comment #1)
I'm going to replace SweepState with SweepActionList to indicate which list we are processing which should be more self-explanatory and update the comments.

Comment 3

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/8b1346172862
Add per-sweep-group actions in addition to per-zone actions r=sfink

Comment 4

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/8b1346172862