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

sangheon.kim sangheon.kim at oracle.com
Tue Mar 13 21:39:21 UTC 2018

Hi Thomas,

On 03/13/2018 06:52 AM, Thomas Schatzl wrote:

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 VALUEOBJCLASSSPEC. Webrevs: http://cr.openjdk.java.net/~tschatzl/8197850/webrev.1to2/index.html (diff) http://cr.openjdk.java.net/~tschatzl/8197850/webrev.2/index.html (full) Looks good, I have several minor comments.

=============================== src/hotspot/share/gc/g1/g1CardLiveData.cpp

292 assert(!hr->is_old() || marked_bytes == (_cm->liveness(hr->hrm_index()) * HeapWordSize),

Don't we need to print assert message for hr->is_old()?

=============================== src/hotspot/share/gc/g1/g1CollectedHeap.cpp

4215 // Any worker id is fine here as we are in the VM thread and single-threaded.

Would be better to add 'valid'? // Any valid worker id ...

=============================== src/hotspot/share/gc/g1/g1ConcurrentMark.cpp

1840 log_debug(gc, stats)("Mark stats cache hits " SIZE_FORMAT " misses " SIZE_FORMAT " ratio %1.3lf",

gc+stats is not registered at logPrefix.hpp, so currently GC ID is not printed. I guess this is not intensional?

=============================== src/hotspot/share/gc/g1/g1RegionMarkStatsCache.hpp

25 #ifndef SHARE_VM_GC_G1_G1REGIONMARKSTATSCACHE_HPP

I'm okay leaving as is but we don't have 'VM' directory anymore. At some point we would want to clean up these stuffs at once?

61 // logical and.

This seems incomplete sentence?

Thanks, Sangheon

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

-------------- next part -------------- An HTML attachment was scrubbed... URL: <https://mail.openjdk.org/pipermail/hotspot-gc-dev/attachments/20180313/3a5b34b3/attachment.htm>

More information about the hotspot-gc-dev mailing list