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

[Thomas Schatzl]

Hi Stefan,

On Wed, 2018-03-07 at 11:12 +0100, Stefan Johansson wrote:
> Thanks Thomas,
> 
> This looks good to me, reviewed.

  thanks for your review; unfortunately rebasing the changes to latest
caused some additional work.

In this particular case, contents of g1RegionMarkStatsCache had to be
distributed to .cpp and .inline.hpp files (recent .inline.hpp include
fixes), and I preemptively removed the use of one VALUE_OBJ_CLASS_SPEC.

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

Testing:
hs-tier 1-5

Thanks,
  Thomas



> 
> Stefan
> 
> On 2018-03-07 09:48, Thomas Schatzl wrote:
> > 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.h
> > > > > > tml
> > > > > 
> > > > > 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.ht
> > ml
> > (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
> 
>