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

[Stefan Johansson]

Hi Thomas,

On 2018-03-05 15:41, Thomas Schatzl wrote:
> Hi all,
>
>    can I have reviews for this change that calculates the liveness in
> regions (below nTAMS) during marking?
>
> This is required to be more clever about selecting the regions G1 is
> going to rebuild the remembered sets for (i.e. the well-known
> G1MixedGCLiveThresholdPercent threshold).
>
> This slows down the marking part of the concurrent phase a bit, but all
> alternatives were worse:
>
> - initially rebuild everything and then drop remembered sets for
> regions for too highly occupied regions at cleanup: wastes lots of
> space and memory maintaining these superfluous remembered sets.
>
> - choose regions using previous marking cycle data: this is generally
> not a good idea if your marking cycles are infrequent (which should be
> the normal case), as it tends to make too few regions available for
> evacuation (e.g. eager reclaim may already reclaim a lot). I.e. you
> need extra headroom, or be more aggressive with rebuilding.
>
> The main change, next to the calls that add the liveness to some
> internal data structure every time an object is marked, is some per-
> thread cache that (G1RegionMarkStatsCache). It helps decreasing the
> synchronization overhead the (atomic) increments in the global result
> cause.
>
> It is a very simple fixed size hashmap where increments to the same
> entries are collected, and on collision evicts the previous entry,
> doing the atomic operation on the global result.
>
> There is also some unfortunate moving of code that is required to flush
> these caches correctly during mark overflow handling.
>
> 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.
---
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.
---
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.
---

Thanks,
Stefan

> Testing:
> hs-tier 1-5; the next change will add an assert that verifies that this
> concurrently calculated value is correct at the end of rebuild, i.e.
> the results will always verified.
>
> Thanks,
>    Thomas
>