1360526 - Parallelise more table sweeping

Status: RESOLVED FIXED

(Core :: JavaScript: GC, enhancement)

Description

[Jon Coppeard (:jonco)]

When we start to sweep a group of zones there are several tables that need to be swept and this is done non-incrementally so as to be atomic from the point of view of the mutator.

Most but not all of this work is performed in parallel using the JS helper threads.  We should parallelise as much as possible of the remaining work.

(The long term goal here is to do this work incrementally, but it seems like this would be an easy win)

Comment 1

[Jon Coppeard (:jonco)]

Attachment: 1 - refactor-queue-arenas

First of all, refactor part that queues arenas for sweeping to use a single loop to iterate the zones.

Comment 2

[Jon Coppeard (:jonco)]

Attachment: 2 - sweep-weak-maps-in-parallel

Create a new sweep task for sweeping weak maps in parallel.  The wrinkle here is that sweeping debuggers may update some debugger weak maps, so do this on the main thread before we do anything else.  Use of debuggers is rare so this is not a performance concern.

Comment 3

[Jon Coppeard (:jonco)]

Attachment: 3 - sweep-misc

Move some miscellaneous sweeping into the 'sweep misc' task.  We don't need to sweep all watchpoints in a zone GC since the cross-compartment closure pointer they hold is traced strongly during marking.  We do need to trace all watchpoints during a compacting GC since other zones may have watchpoints with closures that are in compacting zones.

Comment 4

[Jon Coppeard (:jonco)]

Attachment: 4 - sweep-weak-caches

Sweep runtime-wide weak caches as part of the sweep weak caches task.  I factored the iteration of per-zone and per-runtime caches out into a separate function.

Comment 5

[Jon Coppeard (:jonco)]

Attachment: 5 - sweep-unique-ids

Make a new task for sweeping unique IDs.

This conflicts with JSCompartment::sweepDebugEnvironments which looks up things in the unique ID table, so I factored this (and breakpoint sweeping) into a separate method that runs first before anything else.

Comment 6

[Jon Coppeard (:jonco)]

Attachment: 6 - refactor-jit-sweep

Everything left that happens on the main thread while parallel tasks are running is related to the JITs (even Zone::beginSweepTypes it seems) so I factored this out into a separate method.

Comment 7

[Jon Coppeard (:jonco)]

Attachment: 7 - refactor-start-join

This adds AutoRunGCSweepTask to factor out a load of boilerplate.

I had to make ~GCParallelTask with whether or not the helper thread lock was held.

Comment 8

[Jon Coppeard (:jonco)]

Attachment: 8 - fix-stats-phases

Finally, add separate stats phases for all these different things.

Comment 9

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

Comment on attachment 8863779 [details] [diff] [review]
4 - sweep-weak-caches

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

::: js/src/jsgc.cpp
@@ +5127,5 @@
> +    })) {
> +        return out;
> +    }
> +
> +    // If we ran out of memory, do all the work now and return and empty list.

*an empty

@@ +5132,5 @@
> +    IterateWeakCaches(rt, [&] (JS::WeakCache<void*>* cache) {
> +        SweepWeakCacheTask(rt, *cache).runFromActiveCooperatingThread(rt);
> +        return true;
> +    });
> +    return WeakCacheTaskVector();

I guess this is ok. All the lambda and functor vector and fallback stuff seems a little complicated, but I guess counting up the weak caches in the sweep group and then using reserve() isn't any better. It still ends up coding the iteration twice.

I tried to think of a way to make template-happy appended and chained iterators, but... never mind. I guess the alternative would be an explicit weak cache iterator that you give a runtime.

Oh well. This works.

Comment 10

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

Comment on attachment 8863826 [details] [diff] [review]
5 - sweep-unique-ids

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

::: js/src/jsgc.cpp
@@ +5108,5 @@
> +
> +    gcstats::AutoPhase ap(stats(), gcstats::PHASE_SWEEP_COMPARTMENTS);
> +
> +    // Sweep debug environment information. This performs lookups in the Zone's
> +    // unique IDs table and so must not happen in parallel with this.

s/this/sweeping that table/?

Comment 11

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

Comment on attachment 8863828 [details] [diff] [review]
7 - refactor-start-join

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

::: js/src/jsgc.cpp
@@ +4970,5 @@
> +MAKE_GC_SWEEP_TASK(SweepRegExpsTask,          gcstats::PHASE_SWEEP_REGEXP);
> +MAKE_GC_SWEEP_TASK(SweepMiscTask,             gcstats::PHASE_SWEEP_MISC);
> +MAKE_GC_SWEEP_TASK(SweepCompressionTasksTask, gcstats::PHASE_SWEEP_MISC);
> +MAKE_GC_SWEEP_TASK(SweepWeakMapsTask,         gcstats::PHASE_SWEEP_MISC);
> +MAKE_GC_SWEEP_TASK(SweepUniqueIdsTask,        gcstats::PHASE_SWEEP_BREAKPOINT);

This patch does kill a lot of boilerplate. Nice!

Though here, I'm starting to wonder what the advantage of this macro is over something like

template <void RunnerFunc(GCRuntime*), gcstats::Phase phase>
class SweepTask : public GCSweepTask {
  void run() override {
    AutoPhase ap(runtime()->gc.stats(), phase);
    RunnerFunc(runtime());
  }
}

static void
SweepAtoms(GCRuntime* rt)
{
   ...
}

.
.
.
AutoRunGCSweepTask<SweepTask<SweepAtoms, gcstats::PHASE_SWEEP_ATOMS>>> sweepAtoms(this, helperLock);

I think I'm mixing up JSRuntime and GCRuntime and probably missing various other bits and pieces. And I'm not sure there's really much of a benefit here. It's just a pretty funky macro, I guess.

Comment 12

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

Comment on attachment 8863829 [details] [diff] [review]
8 - fix-stats-phases

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

::: js/src/gc/Statistics.cpp
@@ +221,2 @@
>  
>      // Current number of telemetryBuckets is 61. If you insert new phases

Change the 61. Or make it refer to the PHASE_LIMIT one so we don't have to keep changing both.

::: js/src/jsgc.cpp
@@ +4970,5 @@
>  MAKE_GC_SWEEP_TASK(SweepRegExpsTask,          gcstats::PHASE_SWEEP_REGEXP);
>  MAKE_GC_SWEEP_TASK(SweepMiscTask,             gcstats::PHASE_SWEEP_MISC);
> +MAKE_GC_SWEEP_TASK(SweepCompressionTasksTask, gcstats::PHASE_SWEEP_COMPRESSION);
> +MAKE_GC_SWEEP_TASK(SweepWeakMapsTask,         gcstats::PHASE_SWEEP_WEAKMAPS);
> +MAKE_GC_SWEEP_TASK(SweepUniqueIdsTask,        gcstats::PHASE_SWEEP_UNIQUEIDS);

Heh. With all of these unique, we could even

template<> void
SweepTask<PHASE_SWEEP_UNIQUEIDS>::run() { ... }

but... nah.

Comment 13

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/02fb52a5071f
Refactor queing arenas for sweeping to use a single loop r=sfink
https://hg.mozilla.org/integration/mozilla-inbound/rev/3d0f582a516e
Sweep weakmaps in parallel with other sweeping r=sfink
https://hg.mozilla.org/integration/mozilla-inbound/rev/1ac60eace76b
Move more miscellaneous sweeping off the main thread r=sfink
https://hg.mozilla.org/integration/mozilla-inbound/rev/a755dec3c1a5
Sweep runtime-wide weak caches as part of the weak cache sweeping task r=sfink
https://hg.mozilla.org/integration/mozilla-inbound/rev/6139e5baee96
Sweep unique IDs in parallel with other sweeping r=sfink
https://hg.mozilla.org/integration/mozilla-inbound/rev/8c42621d4720
Move sweeping JIT-related data into its own method r=sfink
https://hg.mozilla.org/integration/mozilla-inbound/rev/276c2459d415
Add RAII class to start/join a GC sweep task r=sfink
https://hg.mozilla.org/integration/mozilla-inbound/rev/aaa215039794
Add separate stats phases for all the sweep tasks r=sfink

Comment 14

[Jon Coppeard (:jonco)]

Attachment: 9 - improve-weak-cache-iteration

(In reply to Steve Fink [:sfink] [:s:] from comment #9)
> All the lambda and functor vector and fallback stuff
> seems a little complicated, but I guess counting up the weak caches in the
> sweep group and then using reserve() isn't any better.

I rewrote this to use an iterator class like we do for Zones etc, but the result was longer and it wasn't as clear that it was correct as having a iterator function containing a few for loops.

Instead I refactored the current code a little to hopefully make it a bit clearer.

Comment 15

[Jon Coppeard (:jonco)]

Attachment: 10 - remove-gc-sweep-task

(In reply to Steve Fink [:sfink] [:s:] from comment #11)
> Though here, I'm starting to wonder what the advantage of this macro is over
> something like
> 
> template <void RunnerFunc(GCRuntime*), gcstats::Phase phase>
> class SweepTask : public GCSweepTask {

This is a great idea.  In fact we can kill GCSweepTask too and have our auto class inherit from GCParallelTask.

Comment 16

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

Comment on attachment 8864178 [details] [diff] [review]
9 - improve-weak-cache-iteration

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

That does read a little better, thanks.

Comment 17

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/02fb52a5071f
https://hg.mozilla.org/mozilla-central/rev/3d0f582a516e
https://hg.mozilla.org/mozilla-central/rev/1ac60eace76b
https://hg.mozilla.org/mozilla-central/rev/a755dec3c1a5
https://hg.mozilla.org/mozilla-central/rev/6139e5baee96
https://hg.mozilla.org/mozilla-central/rev/8c42621d4720
https://hg.mozilla.org/mozilla-central/rev/276c2459d415
https://hg.mozilla.org/mozilla-central/rev/aaa215039794

Comment 18

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

Comment on attachment 8864201 [details] [diff] [review]
10 - remove-gc-sweep-task

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

r=me since these are heavyweight enough that going through a function pointer is ok. I was originally thinking that you'd templatize on the exact function to eliminate the indirect call, but this is probably better. (I think it's probably simpler, and the perf overhead doesn't matter.)

Comment 19

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/2c14238a5a52
Make weak cache iteration more readable r=sfink
https://hg.mozilla.org/integration/mozilla-inbound/rev/b388bdf5bf38
Refactor to remove GCSweepTask and associated macros r=sfink

Comment 20

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/2c14238a5a52
https://hg.mozilla.org/mozilla-central/rev/b388bdf5bf38