RFR [L][3/7]: 8197850: Calculate liveness in regions during marking

[Thomas Schatzl]

Hi,

On Tue, 2018-03-06 at 15:03 +0100, Stefan Johansson wrote:
> 
> On 2018-03-06 14:33, Thomas Schatzl wrote:
> > On Tue, 2018-03-06 at 13:31 +0100, Stefan Johansson wrote:
> > > Hi Thomas,
> > > 
> > > On 2018-03-05 15:41, Thomas Schatzl wrote:
> > > > Hi all,
> > > > 
> > > > ...
> > > > 
> > > > CR:
> > > > https://bugs.openjdk.java.net/browse/JDK-8197850
> > > > Webrev:
> > > > http://cr.openjdk.java.net/~tschatzl/8197850/webrev/index.html
> > > 
> > > Look good, just some minor things.
> > > 
> > > src/hotspot/share/gc/g1/g1ConcurrentMark.cpp
> > > 394   _region_mark_stats(NEW_C_HEAP_ARRAY(G1RegionMarkStats,
> > > max_regions, mtGC))
> > > 
> > > max_regions is passed into the constructor but everywhere else in
> > > G1ConcurrentMark we use g1h->max_regions(). I would consider
> > > either
> > > calling g1h->max_regions() in the constructor too, or save the
> > > value
> > > in a member and use it everywhere.
> > 
> > Done using option 1), use g1h->max_regions() everywhere.
> 
> Thanks.
> > > ---
> > > src/hotspot/share/gc/g1/g1_globals.hpp
> > >    262   experimental(size_t, G1RegionMarkStatsCacheSize, 1024,
> > >    ...
> > >    265           range(128, (max_juint / 2) + 1)
> > > 
> > > Do we need this flag or should we scale it against the number of
> > > regions? If we want to keep it I think we should have a somewhat
> > > more restrictive range.
> > 
> > I would not want this to scale according to the number of heap
> > regions,
> > because that is going to have a significant impact on pause times
> > when
> > all of the thread's caches are evicted (The O(#regions) part).
> 
> True, just a thought since 1024 is roughly half the number regions
> we aim for maybe this could be used together with a reasonable upper
> bound.
>
> > The flag has only been added (rather late, admittedly) to fix
> > potential issues with marking time with really large heaps with
> > tens of thousands of regions.
> > Let me do some more measurements, I will get back to you with them
> > before giving you a new webrev.
> 
> Sound good.

I did some measurements with 100k regions, and I think we can just
remove the flag. There is always logging available (gc+mark+stats) that
shows cache hit ratio to diagnose issues.

> > > ---
> > > src/hotspot/share/gc/g1/g1RegionMarkStatsCache.hpp
> > > 
> > > In G1RegionMarkStatsCacheEntry _region_idx is set to 0 when
> > > clearing. I don't see a problem with it right now, but it feels a
> > > bit wrong since index 0 is a valid region index. Maybe we could
> > > use another cleared marker.
> > 
> > Unfortunately there is none (one could use something like -1, but
> > that one is valid too), and it seems a waste of memory to reserve
> > more space for it.
> > 
> > I see your point that this is a bit ugly, but there should never be
> > any issue because of the initialization of the value part of an
> > entry with zero. It simply has no impact on totals if you add zero.
> > 
> > The code could initialize the cache with region indices from 0 to
> > cache size - 1, which are valid values given the hash function,
> > what do you think?
> > 
> > However that would slow down clearing a little (the compiler will
> > just compile it to a memset() in the loop right now).
> 
> You are correct that -1 is a valid index, but we seldom see heaps
> with so many regions. I'm fine with leaving it as is, if you dislike
> using -1 and if it will slow down clearing.

Just a bit. It seems as bad to use -1 or another random value, so I
kept it.

Webrevs:
http://cr.openjdk.java.net/~tschatzl/8197850/webrev.0_to_1/index.html
(diff)
http://cr.openjdk.java.net/~tschatzl/8197850/webrev.1/index.html (full)

Testing:
This change ran, with some changes to 8180415 due to internal comments
(I will post in a few minutes) through hs-tier 1-5 over night.

Thanks,
  Thomas