1469640 - Use GCParallelTask for background sweeping

Status: RESOLVED FIXED

(Core :: JavaScript: GC, enhancement)

Description

[Jon Coppeard (:jonco)]

For some reason background sweeping has entirely separate infrastructure to our other off-thread work.  We should use the same infrastructure for both.

Comment 1

[Jon Coppeard (:jonco)]

Attachment: bug1469640-sweep-task

Patch to remove GCHelperState and make background sweeping a GCParallelTask.

There's some complication because a single background sweep task can sweep zones from more than one sweep group, so it loops until backgroundSweepZones is empty (in BackgroundSweepTask::run).  Checking whether it is still running on the main thread can't be done by checking the GCParallelTask state because this can still report it as running after we return from the method because we drop the helper thread lock at that time.  Instead we add a new state variable |done| and make sure we set this to false before starting the task.

The alternative was to make all GCParallelTask run() methods start with the helper thread lock held and release it themselves.  It would be annoying to make all clients do this though.

Comment 2

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

Comment on attachment 8986469 [details] [diff] [review]
bug1469640-sweep-task

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

r=me if that !isBackgroundSweeping() && !done problem isn't a problem.

::: js/src/gc/GCRuntime.h
@@ +331,5 @@
>      State state() const { return incrementalState; }
>      bool isHeapCompacting() const { return state() == State::Compact; }
>      bool isForegroundSweeping() const { return state() == State::Sweep; }
> +    bool isBackgroundSweeping() {
> +        return sweepTask.isRunning();

This could return false while !done, right? Is this sequence of events possible:

background sweep is running
main thread does queueZonesForBackgroundSweep
main thread gets to State::Finalize
background sweep empties its local copy of the queue, unlocks HelperThreadState
main thread checks isBackgroundSweeping()=false (with GC lock held, HelperThreadState lock not held)
main thread continues
background sweep re-locks HelperThreadState lock and works on the rest of the queue
main thread does sweepZones() concurrently with the background sweep -- I assume this is bad

@@ +480,5 @@
>      // Free certain LifoAlloc blocks when it is safe to do so.
>      void freeUnusedLifoBlocksAfterSweeping(LifoAlloc* lifo);
>      void freeAllLifoBlocksAfterSweeping(LifoAlloc* lifo);
>  
>      // Public here for ReleaseArenaLists and FinalizeTypedArenas.

Please document updateThreshold -- what it is and why it needs to be conditional.

Maybe use alreadyDecrementedArenaCount or something?

@@ -828,2 @@
>  
> -  private:

Why is all this stuff no longer private? Is it because you now need to friend a templatized class? I think that should work.

Comment 3

[Jon Coppeard (:jonco)]

(In reply to Steve Fink [:sfink] [:s:] (PTO June 31) from comment #2)
> This could return false while !done, right? 

No, this is not possible.  isRunning() ends up checking GCParallelTask::state_ which is set to Dispatched in GCParallelTask::startWithLockHeld() and remains that way until the run() method returns.

> Please document updateThreshold -- what it is and why it needs to be
> conditional.

I just realised I can move the update into the callers and get rid of this altogether, so I'll do that.

> Why is all this stuff no longer private? Is it because you now need to
> friend a templatized class? I think that should work.

This |private| was unnecessary because we're already in a private section at this point.

Comment 4

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/6867578ac52d
Make background sweeping a parallel task and remove GCHelperState r=sfink

Comment 5

[Noemi Erli[:noemi_erli]]

https://hg.mozilla.org/mozilla-central/rev/6867578ac52d