1072564 - Incrementalize type information sweeping

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Brian Hackett [Laid off!]]

Type information sweeping is done non-incrementally, and is often the bottleneck when we do an incremental GC.  Some parts of this, such as sweeping some tables, are truly non-incremental, but these parts are a small portion of type sweeping.  Most of the time is spent walking the scripts and type objects in each zone and copying live data allocated for these into a new LifoAlloc.  These parts can be done incrementally, which should substantially lower the max pause time taken for this part of sweeping.

Comment 1

[Brian Hackett [Laid off!]]

Attachment: patch

Comment 2

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

Comment on attachment 8495995 [details] [diff] [review]
patch

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

Thanks for doing this! I'm having trouble remembering the details of RecompileInfo though. Can you explain what that is and how it relates to CompilerOutput?

Also, I want to make sure that Jon has seen this. I'm guessing it's likely to interact with compacting GC.

::: js/src/jsgc.cpp
@@ +4853,5 @@
> +            gcstats::AutoPhase ap1(stats, gcstats::PHASE_SWEEP_COMPARTMENTS);
> +            gcstats::AutoPhase ap2(stats, gcstats::PHASE_SWEEP_TYPES);
> +
> +            for (; sweepZone; sweepZone = sweepZone->nextNodeInGroup()) {
> +                Zone *zone = sweepZone;

Instead of saving zone in a variable, how about saving the ArenaLists&? That would shorten some of these long lines, which would make this code a little easier to read.

@@ +4854,5 @@
> +            gcstats::AutoPhase ap2(stats, gcstats::PHASE_SWEEP_TYPES);
> +
> +            for (; sweepZone; sweepZone = sweepZone->nextNodeInGroup()) {
> +                Zone *zone = sweepZone;
> +                bool oom = false;

Could you make an RAII class here? It could have a method to return a pointer to the actual oom boolean and its destructor would take care of calling OnTypeInferenceOOM. You would also need a way to explicitly do the destruction thing for the one right before endSweep.

::: js/src/jsgc.h
@@ +617,3 @@
>      ArenaHeader *gcShapeArenasToSweep;
> +    ArenaHeader *gcScriptArenasToSweep;
> +    ArenaHeader *gcTypeObjectArenasToSweep;

How about calling these new ones gcFooArenasToUpdate? The difference is that gcShapeArenasToSweep processes dead objects and in the other two we're processing live objects.

::: js/src/jsinferinlines.h
@@ +48,5 @@
>  inline CompilerOutput*
>  RecompileInfo::compilerOutput(TypeZone &types) const
>  {
> +    if (generation != types.outputGeneration) {
> +        if (!types.sweepCompilerOutputs || outputIndex >= types.sweepCompilerOutputs->length())

I see that this is present in the normal branch too, but how would it ever happen (in either case)?

Comment 3

[Brian Hackett [Laid off!]]

(In reply to Bill McCloskey (:billm) from comment #2)
> Thanks for doing this! I'm having trouble remembering the details of
> RecompileInfo though. Can you explain what that is and how it relates to
> CompilerOutput?

A RecompileInfo is just an index into the zone's compiler outputs.  It would be neater if it was a pointer but since CompilerOutputs are allocated inline with a vector's data we can't do that.  It would be cleaner if we just heap allocated CompilerOutputs and removed RecompileInfo.  The wrinkle with this patch is that since we sweep type information incrementally there are two vectors of CompilerOutputs and we need to be able to determine which one a RecompileInfo is indexing into.  This would also be a lot simpler if we heap allocated CompilerOutput.

> ::: js/src/jsinferinlines.h
> @@ +48,5 @@
> >  inline CompilerOutput*
> >  RecompileInfo::compilerOutput(TypeZone &types) const
> >  {
> > +    if (generation != types.outputGeneration) {
> > +        if (!types.sweepCompilerOutputs || outputIndex >= types.sweepCompilerOutputs->length())
> 
> I see that this is present in the normal branch too, but how would it ever
> happen (in either case)?

This is defensive code that was added a while back when we were seeing crashes due to apparently accessing invalid CompilerOutput indexes.  I don't think the underlying bug was identified and I don't know if this stuff is necessary anymore.

Comment 4

[Jon Coppeard (:jonco)]

Comment on attachment 8495995 [details] [diff] [review]
patch

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

Thanks for the heads-up - the GC changes look good to me and it's great to see the arena traversals incrementalized.

Comment 5

[Brian Hackett [Laid off!]]

Attachment: patch

This patch addresses some major design flags in the previous one, which showed up during testing:

- During incremental sweeping the contents of type sets that have not been swept yet are accessible to the VM, which can read out pointers that are about to be finalized and e.g. use them during Ion compilation.  This patch changes type sweeping so that we can sweep type objects and scripts either as part of the incremental sweep or at the point they are first used by the VM.  The TypeZone's generation is used to track whether this sweeping has occurred, as it already did for RecompileIndex sweeping.

- Type information sweeping needs to know whether objects are about to be finalized or not, and at the point the sweeping occurred foreground finalized objects had already been swept and their memory reclaimed for allocation, invalidating the mark bits.  This patch changes things so that after the foreground finalization the arenas are not used for allocation or released until after type sweeping finishes in the zone.

This patch is green on try:

https://tbpl.mozilla.org/?tree=Try&rev=c6f4a2cef3e7

Comment 6

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

Sorry I've been so slow on this. I should be finishing some stuff up tomorrow, so I'll start on it by Wednesday.

Comment 7

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

Comment on attachment 8501191 [details] [diff] [review]
patch

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

Sorry again for the slow review. I promise I'll be faster for the next one; I have an idea below for a change to mergeSweptArenas that I think would simplify the patch.

Also, I think it's a problem that you're converting some of the recompile vectors from ContextAllocPolicy to SystemAllocPolicy. The result is that we'll return false on OOM without setting an exception on the cx. You could fix this by explicitly flagging the OOM each time we return false when an append operation fails. There might be an easier way though. It sorta looks like all the places where you use the vector, there will be a cx. Maybe that's not true for AutoEnterAnalysis, but I'm guessing we don't use the pendingRecompiles if the non-cx constructor is used?

::: js/src/jsgc.cpp
@@ -566,5 @@
> -     * During parallel sections, we sometimes finalize the parallel arenas,
> -     * but in that case, we want to hold on to the memory in our arena
> -     * lists, not offer it up for reuse.
> -     */
> -    bool releaseArenas = !InParallelSection();

Can you do MOZ_ASSERT_IF(InParallelSection(), keepArenas)?

@@ +2677,5 @@
> +ArenaLists::mergeSweptArenas(AllocKind thingKind)
> +{
> +    // Merge the arenas actively being allocated from at arenaLists[thingKind]
> +    // with arenas that have been swept in arenaListsToSweep[thingKind], and
> +    // release any empty arenas in the latter list.

It seems like what you're doing is pretty similar to what we do for background finalization: combine a list of swept arenas with the arenas that we've been allocating into during GC. The latter list should consist entirely of full arenas (including the arena we're currently allocating into, which will be full by the time we need to look at the list). The only difference from background finalization is that you want to release all the empty arenas when done.

Based on that, I think it would be worth it to try a different (hopefully simpler) approach:
- queueObjectsForSweep would call some variant of finalizeNow that would separate out the completely empty arenas from the SortedArenaList. These empty lists would have to be stored in some new global field somewhere. Then we could call toArenaList() on the remaining SortedArenaList stuff to get a list of non-empty arenas.
- Then mergeSweptArenas code would release the empty arenas and call the existing insertListWithCursorAtEnd function for the remainder, as we do in bg finalization:
http://mxr.mozilla.org/mozilla-central/source/js/src/jsgc.cpp#2576

Comment 8

[Brian Hackett [Laid off!]]

Attachment: patch

Updated per comments.

Comment 9

[Brian Hackett [Laid off!]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/5a443ae5cf22

(I had to update a PJS jit-test that started failing on my machine because it was seeing GC activity earlier and the code bailed out.  I don't even know if that was related to this patch or not as the test works fine if there are more threads available to it.  These "just so story" jit-tests are pretty unfortunate.)

Comment 10

[Ryan VanderMeulen [:RyanVM]]

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