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

Thomas Schatzl thomas.schatzl at oracle.com
Wed Mar 7 08:48:41 UTC 2018

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.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

More information about the hotspot-gc-dev mailing list