Closed Bug 1376096 Opened 3 years ago Closed 3 years ago

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

Categories

(Core :: JavaScript: GC, enhancement)

55 Branch
enhancement
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla56
Tracking Status
firefox56 --- fixed

People

(Reporter: jonco, Assigned: jonco)

References

Details

Attachments

(1 file)

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.
Attachment #8881031 - Flags: review?(sphink)
Assignee: nobody → jcoppeard
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>.
Attachment #8881031 - Flags: review?(sphink) → review+
(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.
Pushed by jcoppeard@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/8b1346172862
Add per-sweep-group actions in addition to per-zone actions r=sfink
https://hg.mozilla.org/mozilla-central/rev/8b1346172862
Status: NEW → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla56
You need to log in before you can comment on or make changes to this bug.