RFR [L][3/7]: 8197850: Calculate liveness in regions during marking (original) (raw)

Stefan Johansson stefan.johansson at oracle.com
Tue Mar 6 12:31:31 UTC 2018

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

More information about the hotspot-gc-dev mailing list