RFR [S][5/7]: 8197573: Remove concurrent cleanup and secondary free list handling

[Thomas Schatzl]

Hi,

On Thu, 2018-03-08 at 13:28 +0100, Stefan Johansson wrote:
> On 2018-03-08 12:59, Thomas Schatzl wrote:
> > Hi,
> > 
> > On Mon, 2018-03-05 at 16:16 +0100, Thomas Schatzl wrote:
> > > Hi all,
> > > 
> > >    can I have reviews for this change that removes the concurrent
> > > cleanup phase and the secondary free list handling entirely. It
> > > moves leftover work into the Cleanup pause.
> > > 
> > > In the previous change the remembered set scrubbing, which took
> > > in cases where it mattered like 95%+ of time, has been removed
> > > because it is not required any more.
> > > 
> > > The reasons for removal are:
> > > - due to JDK-8180415 *freeing* remembered sets (as opposed to
> > > calculating occupancy) is really fast, so this phase has never
> > > been very long for a long time.
> > > - it adds a lock in the LAB allocation path
> > > 
> > > It also allows us to remove the "mixed_gc_pending" flag in the
> > > CollectorState, as there is no further time to wait until G1 can
> > > schedule the "last young gc".
> > > 
> > > CR:
> > > https://bugs.openjdk.java.net/browse/JDK-8197573
> > > Webrev:
> > > http://cr.openjdk.java.net/~tschatzl/8197573/webrev/index.html
> > > Testing:
> > > hs-tier 1-3, ....
> > > 
> > > (I consider this an "S" changeset because it is mostly removal of
> > > code).
> > > 
> > 
> > Some ripple-through after latest changes to JDK-8180415 requires an
> > update of the webrevs:
> > http://cr.openjdk.java.net/~tschatzl/8197573/webrev.0_to_1 (diff)
> > http://cr.openjdk.java.net/~tschatzl/8197573/webrev.1 (full)
> 
> Looks lovely, just some minor things:
> src/hotspot/share/gc/g1/concurrentMarkThread.cpp
> 436           log_info(gc, marking)("Concurrent Mark Abort");
> 
> This line was removed, and it looks like will never log a concurrent 
> mark abort now. Looks like this could be added to 
> G1ConcurrentMark::concurrent_cycle_end() where we trace a potential
> cm  failure.

Nice catch!

> ---
> src/hotspot/share/gc/g1/g1FullGCPrepareTask.cpp
> 106   _g1h->free_humongous_region(hr, &dummy_free_list);
> 
> Should be safe to call free_humongous_region with locked = true
> here, since this will be the only thread working on that region.

Fixed.

> 
> 115   _g1h->card_table()->clear(MemRegion(hr->bottom(), hr->end()));
> 
> Pre-existing, but since you have added the clear_cardtable() function
> we could use it here as well, seems to be the only place where the
> old "scheme" is used.

Done.

> ---
> 
> > I also found some more opportunity to do minor cleanup after
> > Stefan's
> > last remark about log messages...
> 
> Nice, and you also restructured the code exactly like I planned to 
> comment :)

:)

New webrevs:
http://cr.openjdk.java.net/~tschatzl/8197573/webrev.1_to_2/ (diff)
http://cr.openjdk.java.net/~tschatzl/8197573/webrev.2/ (full)

Thanks,
  Thomas