989390 - Start background sweeping as soon as arenas are available to sweep

Status: RESOLVED FIXED

(Core :: JavaScript: GC, defect)

Description

[Jon Coppeard (:jonco)]

When we finish marking of a zone group, arenas for thing kinds that are swept incrementally (in the foreground or background) are removed and placed on a list waiting to be swept.  Foreground sweeping happens interleaved with mutator execution, and background sweeping happens on a separate thread which is only started at the very end of the collection.

The result is that when the mutator allocates an object during the sweep phase, it may find the arena list for the thing kind empty and have to allocate new arenas.  When sweeping finishes, we will probably find there are now non-full arenas available for that thing kind anyway.

If we can reduce the time until swept arenas are available, then we can decrease wasted space in non-full arenas.  It should be possible to start background sweeping for a zone group immediately we start sweeping that group, rather than waiting until the end of the GC.  The background sweeping would then run mainly in parallel with the main thread GC.

Comment 1

[Jon Coppeard (:jonco)]

It turned out when this was measured that this accounted for a only a small proportion of wasted space in arenas.

However this is needed for compacting GC, where we have to wait for background sweeping to finish before we can start compacting.  With this change this will happen much sooner if we start the background sweeping as soon as possible rather than waiting for the end of the GC.

Comment 2

[Jon Coppeard (:jonco)]

Attachment: bug989390-eager-background-sweep

Patch to kick off background sweeping after sweeping every zone group.

Comment 3

[Terrence Cole [:terrence]]

Comment on attachment 8526744 [details] [diff] [review]
bug989390-eager-background-sweep

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

I like the design, in particular how this doesn't even need to interlock all that much with gc slices. However, passing around raw head/tail and the embedded list manipulation really cries out for some more abstraction. I think it would be far better to start out with something like ZoneList for this set of zones, rather than having to tack it on later, as we did for chunkAvailableListHead. The change should be fairly simple, but make things much cleaner. I'd like to see it like that, unless you run into serious problems with that approach.

::: js/src/gc/GCInternals.h
@@ +146,5 @@
>      }
>  };
>  #endif
>  
> +class BackgroundSweepZonesIter

This becomes ZoneList::Iter.

::: js/src/gc/GCRuntime.h
@@ +729,5 @@
>      bool foundBlackGrayEdges;
>  
> +    /* Singly linekd list of zones to be swept in the background. */
> +    JS::Zone *backgroundSweepZones;
> +    JS::Zone *backgroundSweepZonesTail;

JS::ZoneList backgroundSweepZones;

::: js/src/gc/Zone.h
@@ +44,5 @@
>  };
>  
>  namespace gc {
>  
> +class BackgroundSweepZonesIter;

class ZonesList
{
    Zone *head;
    Zone *tail;

  public:
    void append(Zone *zone);
    Zone *removeHead();

    class Iter {
        ...
    };
};

@@ +254,5 @@
>  
> +    bool isQueuedForBackgroundSweep();
> +    void addToBackgroundSweepList(Zone *&listHead, Zone *&listTail);
> +    static void appendBackgroundSweepLists(Zone *&listHead, Zone *&listTail,
> +                                           Zone *list2Head, Zone *list2Tail);

This move to ZoneList.

@@ +321,5 @@
>      bool gcPreserveCode_;
>      bool jitUsingBarriers_;
>  
> +    static Zone * const BackgroundSweepEnd;
> +    Zone *backgroundSweepNext_;

friend class ZoneList;
Zone *next;

::: js/src/jsgc.cpp
@@ +559,5 @@
>                      ArenaLists::KeepArenasEnum keepArenas)
>  {
>      // When operating in the foreground, take the lock at the top.
>      Maybe<AutoLockGC> maybeLock;
> +    if (!fop->onBackgroundThread())

\o/

@@ +3733,4 @@
>          sweepFlag = false;
> +        Zone *zonesToSweep = rt->gc.backgroundSweepZones;
> +        rt->gc.backgroundSweepZones = nullptr;
> +        rt->gc.backgroundSweepZonesTail = nullptr;

rt->gc.backgroundSweepZones = ZoneList();

@@ +5241,5 @@
> +    if (sweepOnBackgroundThread) {
> +        Zone *sweepHead = nullptr;
> +        Zone *sweepTail = nullptr;
> +        for (GCZoneGroupIter zone(rt); !zone.done(); zone.next())
> +            zone->addToBackgroundSweepList(sweepHead, sweepTail);

This can become:
ZoneList toSweep = addZonesToSweepList<GCZoneGroupIter>();
queueZonesForBackgroundSweep(toSweep);

@@ +5561,5 @@
> +        Zone *sweepHead = nullptr;
> +        Zone *sweepTail = nullptr;
> +        for (GCZonesIter zone(rt); !zone.done(); zone.next())
> +            zone->addToBackgroundSweepList(sweepHead, sweepTail);
> +        sweepBackgroundThings(sweepHead, MainThread);

And:
ZoneList toSweep = addZonesToSweepList<GCZonesIter>();
sweepBackgroundThings(toSweep);

Comment 4

[Jon Coppeard (:jonco)]

Attachment: bug989390-eager-background-sweep v2

Yes, that's much nicer.

Comment 5

[Terrence Cole [:terrence]]

Comment on attachment 8527793 [details] [diff] [review]
bug989390-eager-background-sweep v2

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

Yeah, that's nice.

Comment 6

[Jon Coppeard (:jonco)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/00c8108767c3

Comment 7

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/00c8108767c3