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

Stefan Johansson stefan.johansson at oracle.com
Wed Mar 7 10:12:10 UTC 2018

Thanks Thomas,

This looks good to me, reviewed.

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.html Look good, just some minor things. src/hotspot/share/gc/g1/g1ConcurrentMark.cpp 394 regionmarkstats(NEWCHEAPARRAY(G1RegionMarkStats, maxregions, mtGC)) maxregions is passed into the constructor but everywhere else in G1ConcurrentMark we use g1h->maxregions(). I would consider either calling g1h->maxregions() in the constructor too, or save the value in a member and use it everywhere. Done using option 1), use g1h->maxregions() everywhere. Thanks. --- src/hotspot/share/gc/g1/g1globals.hpp 262 experimental(sizet, G1RegionMarkStatsCacheSize, 1024, ... 265 range(128, (maxjuint / 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 regionidx 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.0to1/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

More information about the hotspot-gc-dev mailing list