1417388 - Investigate simplifying sweep actions

Status: RESOLVED FIXED

(Core :: JavaScript: GC, enhancement, P3)

Description

[Jon Coppeard (:jonco)]

Incremental sweeping executes a tree of sweep actions which are defined in GCRuntime::initSweepActions().  The current setup is probably more complicated than required.  

For example we should be able to merge the loops that iterate over zones for example (ForEachZoneInSweepGroup), and we may be able to finalize all objects at the same time rather than in two separate loops for objects and non-object.  I haven't tried either though so there may be some constraints that require this.

Comment 1

[Chia-Hung Duan]

Attachment: Loop-fusion on ForEachZoneGroup and remove unnecessary data structures. [WIP]

This patch removes *savedObjectArenas_* which is redundant if we do
sweepTypeInformation first. Thus I reordered them and merge into the same
ForEachZonGroup. I think *savedEmptyObjectArenas_* can be removed also,
but leaving empty arena in sweepList may fire some assertions.
Merging ForEachZonGroup of *sweepShapeTree* also causes runtime abort. These
two issues will be solved in next patch if it possible to be addressed easily.

Comment 2

[Jon Coppeard (:jonco)]

Comment on attachment 8935305 [details] [diff] [review]
Loop-fusion on ForEachZoneGroup and remove unnecessary data structures. [WIP]

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

It would be great to do sweepTypeInformation first and remove savedObjectArenas, but is this safe?  My guess would that there is some dependency (i.e. some finalizers do things that depend on type information).  Do the test results indicate any problems in this area?

Otherwise, this looks good.

A way forward would be to split this change into several small changes and land the pieces as we go.

Comment 3

[Chia-Hung Duan]

Attachment: Loop-fusion on ForEachZoneGroup and remove unnecessary data structures

The patch fixes minor bugs in the last one and do minor reorder of the sweep
actions.

Test is ongoing.

Comment 4

[Jon Coppeard (:jonco)]

Comment on attachment 8940674 [details] [diff] [review]
Loop-fusion on ForEachZoneGroup and remove unnecessary data structures

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

Thanks for the patch.  This looks really good.  Just a few comments.

::: js/src/gc/ArenaList.h
@@ +265,5 @@
>      ZoneGroupData<Arena*> gcAccessorShapeArenasToUpdate;
>      ZoneGroupData<Arena*> gcScriptArenasToUpdate;
>      ZoneGroupData<Arena*> gcObjectGroupArenasToUpdate;
>  
> +    // The list of empty arenas which are collected during sweep phase.

This should probably also say that these are released at the end of sweeping every sweep group, if that's what we end up doing.

@@ -271,5 @@
> -    // happen at the beginning of the GC), so that type sweeping can determine
> -    // which of the object pointers are marked.
> -    ZoneGroupData<ObjectAllocKindArray<ArenaList>> savedObjectArenas_;
> -    ArenaList& savedObjectArenas(AllocKind i) { return savedObjectArenas_.ref()[i]; }
> -    ZoneGroupData<Arena*> savedEmptyObjectArenas;

It's great that we can remove this.

@@ +316,5 @@
>  
>      void queueForegroundObjectsForSweep(FreeOp* fop);
>      void queueForegroundThingsForSweep(FreeOp* fop);
>  
> +    void ReleaseForegroundSweptEmptyArenas();

nit: should start with lowercase, as above.

::: js/src/gc/GCRuntime.h
@@ +1123,5 @@
>      void sweepJitDataOnMainThread(FreeOp* fop);
>      IncrementalProgress endSweepingSweepGroup(FreeOp* fop, SliceBudget& budget);
>      IncrementalProgress performSweepActions(SliceBudget& sliceBudget, AutoLockForExclusiveAccess& lock);
>      IncrementalProgress sweepTypeInformation(FreeOp* fop, SliceBudget& budget, Zone* zone);
> +    IncrementalProgress ReleaseSweptEmptyArenas(FreeOp* fop, SliceBudget& budget, Zone* zone);

nit: coding style for the JS engine is to have methods start with lowercase letters, like releaseSweptEmptyArenas.

::: js/src/jsgc.cpp
@@ +3827,5 @@
>  
>  bool
>  ArenaLists::checkEmptyArenaList(AllocKind kind)
>  {
> +    bool is_empty = true;

nit: according to style this should be |isEmpty|.  num_live and max_cells below are wrong too, but don't feel you need to fix everything.

@@ +5738,5 @@
>      incrementalSweptArenas.ref().clear();
>  
> +    // During arena relocation, it makes an assumption that there is no empty arena in the
> +    // ArenaList. Thus we should extract the empty arena to meet the assumption.
> +    sweepList.extractEmpty(&savedEmptyArenas.ref());

What this code did previously was to delay releasing of object arenas until after the type information had been swept (presumably because type information sweeping needed access to mark bits for any objects it referred to).

The keepArenas argugument to FinalizeArenas is KEEP_ARENAS only for object alloc kinds.  This value means that it doesn't release arenas immediately but puts them into sweepList, where they can be accessed with extractEmpty.


This extractEmpty call is only needed (and only does anything) if IsObjectAllocKind(thingKind) is true, so it should be inside an if statement.

I'm not sure whether this is needed at all now that we sweep type information first, but it's probably best to leave it as if for this patch.

Also, I'm not sure why not doing this would cause there to be empty arenas left in the ArenaList (I would have expected them to be left in sweepList, and maybe leaked?  not sure).  But in general the reason for the invariant is that we free all completely empty arenas after GC.

@@ +6384,5 @@
> +                                         Call(&GCRuntime::finalizeAllocKind)),
> +                        Call(&GCRuntime::sweepShapeTree))),
> +                // After each zone sweeping, we may collect some empty arenas,
> +                // release them all at once.
> +                ForEachZoneInSweepGroup(rt, Call(&GCRuntime::ReleaseSweptEmptyArenas)),

As discussed, it would be simpler if we could merge this into the previous loop.

Comment 5

[Chia-Hung Duan]

Attachment: Loop-fusion on ForEachZoneGroup and remove unnecessary data structures

Comment 6

[Chia-Hung Duan]

(In reply to Jon Coppeard (:jonco) from comment #4)
> Comment on attachment 8940674 [details] [diff] [review]
> Loop-fusion on ForEachZoneGroup and remove unnecessary data structures
> 
> Review of attachment 8940674 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Thanks for the patch.  This looks really good.  Just a few comments.
> 
> ::: js/src/gc/ArenaList.h
> @@ +265,5 @@
> >      ZoneGroupData<Arena*> gcAccessorShapeArenasToUpdate;
> >      ZoneGroupData<Arena*> gcScriptArenasToUpdate;
> >      ZoneGroupData<Arena*> gcObjectGroupArenasToUpdate;
> >  
> > +    // The list of empty arenas which are collected during sweep phase.
> 
> This should probably also say that these are released at the end of sweeping
> every sweep group, if that's what we end up doing.
> 

Done.

> @@ -271,5 @@
> > -    // happen at the beginning of the GC), so that type sweeping can determine
> > -    // which of the object pointers are marked.
> > -    ZoneGroupData<ObjectAllocKindArray<ArenaList>> savedObjectArenas_;
> > -    ArenaList& savedObjectArenas(AllocKind i) { return savedObjectArenas_.ref()[i]; }
> > -    ZoneGroupData<Arena*> savedEmptyObjectArenas;
> 
> It's great that we can remove this.
> 
> @@ +316,5 @@
> >  
> >      void queueForegroundObjectsForSweep(FreeOp* fop);
> >      void queueForegroundThingsForSweep(FreeOp* fop);
> >  
> > +    void ReleaseForegroundSweptEmptyArenas();
> 
> nit: should start with lowercase, as above.
> 

Done.

> ::: js/src/gc/GCRuntime.h
> @@ +1123,5 @@
> >      void sweepJitDataOnMainThread(FreeOp* fop);
> >      IncrementalProgress endSweepingSweepGroup(FreeOp* fop, SliceBudget& budget);
> >      IncrementalProgress performSweepActions(SliceBudget& sliceBudget, AutoLockForExclusiveAccess& lock);
> >      IncrementalProgress sweepTypeInformation(FreeOp* fop, SliceBudget& budget, Zone* zone);
> > +    IncrementalProgress ReleaseSweptEmptyArenas(FreeOp* fop, SliceBudget& budget, Zone* zone);
> 
> nit: coding style for the JS engine is to have methods start with lowercase
> letters, like releaseSweptEmptyArenas.
> 

Done.

> ::: js/src/jsgc.cpp
> @@ +3827,5 @@
> >  
> >  bool
> >  ArenaLists::checkEmptyArenaList(AllocKind kind)
> >  {
> > +    bool is_empty = true;
> 
> nit: according to style this should be |isEmpty|.  num_live and max_cells
> below are wrong too, but don't feel you need to fix everything.
> 

No problem. I've fixed them all.

> @@ +5738,5 @@
> >      incrementalSweptArenas.ref().clear();
> >  
> > +    // During arena relocation, it makes an assumption that there is no empty arena in the
> > +    // ArenaList. Thus we should extract the empty arena to meet the assumption.
> > +    sweepList.extractEmpty(&savedEmptyArenas.ref());
> 
> What this code did previously was to delay releasing of object arenas until
> after the type information had been swept (presumably because type
> information sweeping needed access to mark bits for any objects it referred
> to).
> 
> The keepArenas argugument to FinalizeArenas is KEEP_ARENAS only for object
> alloc kinds.  This value means that it doesn't release arenas immediately
> but puts them into sweepList, where they can be accessed with extractEmpty.
> 
> 
> This extractEmpty call is only needed (and only does anything) if
> IsObjectAllocKind(thingKind) is true, so it should be inside an if statement.
> 
> I'm not sure whether this is needed at all now that we sweep type
> information first, but it's probably best to leave it as if for this patch.
> 
> Also, I'm not sure why not doing this would cause there to be empty arenas
> left in the ArenaList (I would have expected them to be left in sweepList,
> and maybe leaked?  not sure).  But in general the reason for the invariant
> is that we free all completely empty arenas after GC.
> 

I forget why I remove the *IsObjectAllocKind*..., but it's fine, I've taken it back.
I'll go through this part again and maybe I can remember the reason for that then bring more comments about this part.

> @@ +6384,5 @@
> > +                                         Call(&GCRuntime::finalizeAllocKind)),
> > +                        Call(&GCRuntime::sweepShapeTree))),
> > +                // After each zone sweeping, we may collect some empty arenas,
> > +                // release them all at once.
> > +                ForEachZoneInSweepGroup(rt, Call(&GCRuntime::ReleaseSweptEmptyArenas)),
> 
> As discussed, it would be simpler if we could merge this into the previous
> loop.

Done.

Comment 7

[Jon Coppeard (:jonco)]

Comment on attachment 8942125 [details] [diff] [review]
Loop-fusion on ForEachZoneGroup and remove unnecessary data structures

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

This patch looks great, and is a nice simplification.  Thanks for working on this.

Comment 8

[Jon Coppeard (:jonco)]

Attachment: bug1417388-simplify-sweep-actions

Updated patch

Comment 9

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/c78c6e1797b1
Simplify incremental sweeping on the main thread r=jonco

Comment 10

[Natalia Csoregi [:nataliaCs]]

https://hg.mozilla.org/mozilla-central/rev/c78c6e1797b1