January 2020 - hotspot-gc-dev

[RFC] ZGC proposal for aarch64 jdk11u
by Yangfei (Felix) 17 Jan '20

17 Jan '20

Hi, Currently, we only have zgc for the jdk11 x86 platform. Zgc for aarch64 platform was later added in jdk13 by Stuart from Linaro. So it’s an interesting question whether zgc for aarch64 platform should be backported to jdk11. Dozens of our arm-based cloud customers are switching to jdk11 and some of them have a strong demand for the zgc feature for their business. To satisfy that requirement, we took the action to backport this feature in our jdk11 release. But we think this work should not be kept private. Other jdk11 vendors may come to the same problem. It’s appreciated if this work could be incorporated in the upstream jdk11 repo and further improved. We have backported the following zgc related patches to jdk11u: https://bugs.openjdk.java.net/browse/JDK-8217745 https://bugs.openjdk.java.net/browse/JDK-8224187 https://bugs.openjdk.java.net/browse/JDK-8214527 https://bugs.openjdk.java.net/browse/JDK-8224675<javascript:;> Basic test such as jtreg, jcstress looks good. Specjbb2015 test with zgc gives us anticipated max & critical score. I can provide more details and propose a webrev for the backport. But before that I would like to hear your comments & suggestions. Thanks, Felix

4 7

[15] RFR 8236878: Use atomic instruction to update StringDedupTable's entries and entries_removed counters
by Zhengyu Gu 16 Jan '20

16 Jan '20

Hi, Please review this small change that uses atomic operations to update StringDedupTable's entries and entries_removed counter. This is *not* a correctness fix or performance enhancement, but for Shenandoah GC to move StringDedupTable cleanup task into concurrent phase, while holding StringDedupTable_lock. Bug: https://bugs.openjdk.java.net/browse/JDK-8236878 Webrev: http://cr.openjdk.java.net/~zgu/JDK-8236878/webrev.00/index.html Test: hotspot_gc (fastdebug and release) on x86_64 Linux Submit test in progress. Thanks, -Zhengyu

3 7

[14] RFR 8237392: Shenandoah: Remove unreliable assertion
by Zhengyu Gu 16 Jan '20

16 Jan '20

Offline discussion concluded that the assertion added by JDK-8237369 is not reliable. For piggyback reference updating cycle, has_forwarded_objects flag is carried into next GC cycle and Shenandoah resets marking bitmap concurrently just before new GC cycle. So, there is a short period without reliable marking bitmap, could trigger false assertion. Bug: https://bugs.openjdk.java.net/browse/JDK-8237392 Webrev: http://cr.openjdk.java.net/~zgu/JDK-8237392/webrev.00/ Test: hotspot_gc_shenandoah Thanks, -Zhengyu

2 1

RFR: 8237261: Concurrent refinement activation threshold not updated for card counts
by Kim Barrett 16 Jan '20

16 Jan '20

Please review this change to the activation threshold for the primary (first) concurrent refinement thread. The special calculation used for that thread's threshold wasn't updated to account for the change from using buffer counts to using counts of the cards in the buffers by JDK-8230109. Also fixed a parameter name that wasn't updated by that same change from buffer counts to card counts. CR: https://bugs.openjdk.java.net/browse/JDK-8237261 Webrev: https://cr.openjdk.java.net/~kbarrett/8237261/open.00/ Testing: mach5 tier1 by itself mach5 tier1-5 and some perf testing with in development change for JDK-8237143

3 4

[14] RFR 8237369: Shenandoah: failed vmTestbase/nsk/jvmti/AttachOnDemand/attach021/TestDescription.java test
by Zhengyu Gu 16 Jan '20

16 Jan '20

Please review this small patch. keep_alive is only applicable during marking phase. Bug: https://bugs.openjdk.java.net/browse/JDK-8237369 Webrev: http://cr.openjdk.java.net/~zgu/JDK-8237369/webrev.00/ Test: hotspot_gc_shenandoah vmTestbase/nsk/jvmti/AttachOnDemand/attach021/TestDescription.java with Shenandoah GC (normal and traversal mode) Thank, -Zhengyu

2 1

RFR: 8237198+8237199+8237200: ZGC: Share heap multi-mapping code across platforms
by Per Liden 16 Jan '20

16 Jan '20

Hi, Please review this cleanup of the ZPhysicalMemory/ZBackingFile layer, which aims to de-duplicate some of the multi-mapping code. I've split the change into three separate patches, the main patch followed by two patches doing some renaming. 1) The ZBackingFile code was designed to allow platforms to decide if they want to use heap multi-mapping or some other (possibly HW supported) scheme. As of today, all our supported platforms do heap multi-mapping, so there's some degree of code duplication in ZBackingFile for each platform. This patch moves common multi-mapping code into ZPhysicalMemoryManager. If we in the future find that we want to support a platform that doesn't do multi-mapping, then we can introduce an abstraction for this again. Bug: https://bugs.openjdk.java.net/browse/JDK-8237198 Webrev: http://cr.openjdk.java.net/~pliden/8237198/webrev.0 2) Rename ZBackingFile to ZPhysicalMemoryBacking, since "File" is somewhat misleading on platforms other than Linux. Bug: https://bugs.openjdk.java.net/browse/JDK-8237199 Webrev: http://cr.openjdk.java.net/~pliden/8237199/webrev.0 3) Rename ZBackingPath to ZMountPoint, as it's a better name in light of JDK-8237199. Bug: https://bugs.openjdk.java.net/browse/JDK-8237200 Webrev: http://cr.openjdk.java.net/~pliden/8237200/webrev.0 cheers, Per

4 6

RFR: 8237201: ZGC: Remove unused ZRelocationSetSelector::fragmentation()
by Per Liden 16 Jan '20

16 Jan '20

ZRelocationSetSelector::fragmentation() is not used and can be removed. Bug: https://bugs.openjdk.java.net/browse/JDK-8237201 Webrev: http://cr.openjdk.java.net/~pliden/8237201/webrev.0 /Per

2 2

Re: RFR: 8236073: G1: Use SoftMaxHeapSize to guide GC heuristics
by Thomas Schatzl 16 Jan '20

16 Jan '20

Hi Liang, thanks for your contribution! I looked through the change a bit and have a few comments. What I noticed quickly after initial browsing through it is that this change implements three different features: 1) moving the memory uncommit into the concurrent phase 2) uncommit at the end of (almost) every GC 3) SoftMaxHeapSize These should be split across three separate changes (I already filed JDK-8236926 last week). No particular order I think, but the concurrent uncommit changes are probably hardest and will probably take most time. Some additional initial comments: - in G1Collectedheap::check_soft_max_heap_size_changed(), instead of the check for "AllocateOldGenAt != NULL" we probably want to ask the HeapRegionManager directly about whether it supports this. Also print some log message that it failed. Even on success, print a log message :) - in that same method, I recommend first doing the alignment adjustment (which probably needs to be done for that suggested soft_max_capacity() method below too) and then check if it changed. That saves the repeated != _prev_soft_max_heap_size check. Actually, just using the suggested soft_max_heap_size() method should be fine. - changes in G1CollectedHeap::resize_heap_if_necessary: please make the method to always use the soft max heap size as I do not understand why you would not do that. I recommend adding a "soft_max_capacity()" method in G1CollectedHeap, and let that return MIN2(align_up(SoftMaxHeapSize, heap_alignment), max_capacity()). There are a few places that check SoftMaxHeapSize validity (e.g. SoftMaxHeapSize <= capacity()), they could probably all be removed then. - doing timing: you might have noticed, we are currently transitioning to use Ticks/Tickspan for points in time and durations at least for the calculation, so in any new code please avoid using os::elapsedTime(). Use: Ticks start = Ticks::now(); // timed code phase_times()->record....((Ticks::now() - start).seconds() * 1000.0); instead. - in G1CollectedHeap::shrink_heap_after_young_collection() I would prefer a structure like in expand_heap_after_collection, i.e.: size_t shrink_bytes = _heap_sizing_policy->shrink_amount(); if (shrink_bytes > 0) { // do actual preparation for shrinking } and put all that logic determining the amount to shrink in that shrink_amount() method. - concurrent uncommit: - as mentioned, please split the related changes out from the other changes. This change is hard enough to get right as is. - I would really prefer if we did not need to introduce another helper thread for uncommitting memory. Did you try using the G1YoungRemSetSamplingThread? I understand that uncommit might then delay young gen sampling, but I do not expect these events to occur all the time (but I have no reference here). In the first implementation we could have another thread if others do not object, but every additional thread takes some time to startup and teardown, and memory for at least one stack page. - please move the change in G1CollectedHeap::abort_concurrent_cycle() into a separate method - waiting for completion of the concurrent uncommit and the concurrent marking are completely different concerns. - I admit I haven't looked at all cases in detail, but the split in is_available() and is_unavailable_for_allocation() in HeapRegionManager seems incomplete and unncessary. Particularly because of bad naming, as the documentation for is_available() says it's actually is_available[_for_allocation]. Disregarding the negation, these two look equivalent with the problem that !is_available() != is_unavailable..., which is really bad style. I have not found a case where it is harmful to not consider the _concurrent_resizing_map in is_available(). The split the state of a region between two bitmaps in HeapRegionManager (the available_map and the _concurrent_resizing_map) may be susceptible to tricky races. Please consider changing this to a real "state" as in "Available -> Removing -> Unavailable". This would make the code easier to read too. (And in the future "Adding" if required). - it should be possible to disable concurrent uncommit/resize via an experimental flag. Also there should be no concurrent resize thread if the Heapregionmanager does not support it. G1 couold immediately do the heap change in that case. The reason for this flag is to allow users too disable this if they experience problems. - not sure about why some methods in HeapRegionManager have "resizing" in their method name. As far as I can tell, the change only allows concurrent uncommit. Maybe use the above "remove" for regions instead of "uncommit" regions. Background: The code and comments in HeapRegionManager are aware and fairly consistent (I hope) to not use the wording commit/uncommit for operations on HeapRegions. Only operations on memory pages should use committed/uncommit. The naming in the added methods does not respect that. - some of the methods (e.g. to find free regions) should inform the caller that there are to-be-removed regions to maybe retry after waiting for completion of that thread to avoid unexpected OOM. - I have a feeling that if the concurrent uncommit thread worked on pages, not regions, the code would be easier to understand. It would also solve the issue you asked about with the G1RegionsSmallerThanCommitSizeMapper. You may still need to pass region numbers anyway for logging, but otoh the logging could be done asynchroniusly. - s/parallely/concurrently a few times - there is a bug in the synchronization of the concurrent uncommit thread: it seems possible that the uncommit thread is still working (iterating over the list of regions to uncommit) while a completed (young) GC may add new regions to that list as young gcs do not wait for completion of the uncommit thread. - the concurrently uncommitted regions only become available for commit at the next gc, which seems very long. Why not make them available for commit "immediately"? Related to that is the use of par_set/clear_bit in e.g. the available bitmap: since all par/clear actions are asserted to be in the vm thread at a safepoint, there does not seem to be a need for using the parallel variants of set/clear bit (if keeping the current mechanism). - please document the supposed interactions and assumptions like in the above two paragraphs between the "resize" thread and the other threads and safepoints. - please use the existing HeapRegionManager::shrink_by() method+infrastructure for passing a shrink request to the HRM, either immediately shrinking the heap or deferring for later shrinking (probably controlled by a flag) instead of adding new methods for the same purpose (with mostly the same contents). E.g. there is a lot of code duplication in the new code in HeapRegionManager, particularly the combination of HeapRegionManager::concurrent_uncommit_regions_memory, HRM::synchronize_uncommit_regions_memory and HRM::uncommit_regions could probably be cut to almost 1/3rd. On 13.01.20 12:45, Thomas Schatzl wrote: > Hi Liang, > > On 07.01.20 17:33, Liang Mao wrote: >> Hi Thomas, >> >> As we previously discussed, I use the concurrent heap uncommit/commit >> mechanism to implement the SoftMaxHeapSize for G1. It is also for >> thfurther implementation of >> G1ElasticHeap for ergonomic >> change of heap size. In the previous 8u implementation, we had some >> limitations which are all >> removed now in this patch. The concurrent uncommit/commit can also >> work with some senarios for >> immediate heap expansion. >> >> Here is the webrev link: >> http://cr.openjdk.java.net/~luchsh/8236073.webrev/ >> >> We still have some questions. >> 1. Does the SoftMaxHeapSize limitation need to consider the GC time >> ratio as in >> expand_heap_after_young_collection? Now we haven't put the logic in yet. I am not completely clear what you are asking about, but the gc time ratio only affects the current "optimal" heap size which is bounded by SoftMaxHeapsize/max_capacity. >> 2. The concurrent uncommit/commit can only work for >> G1RegionsLargerThanCommitSizeMapper but not >> G1RegionsSmallerThanCommitSizeMapper which might need some locks to >> ensure the multi-thread >> synchronization issue( heap may expand immediately). I think bringing >> the lock synchronization >> may not be worthy for the little gain. Another idea is can we just not >> uncommit the pages of >> auxiliary data if in G1RegionsSmallerThanCommitSizeMapper? Heap >> regions should not be >> G1RegionsSmallerThanCommitSizeMapper most of time I guess... >> >> Looking forward to your advice:) Thanks, Thomas

2 9

RFR (XS) 8237223: Shenandoah: important flags should not be ergonomic for concurrent class unloading
by Aleksey Shipilev 15 Jan '20

15 Jan '20

Bug: https://bugs.openjdk.java.net/browse/JDK-8237223 Fix: diff -r 53b6aad22933 src/hotspot/share/gc/shenandoah/shenandoahNormalMode.cpp --- a/src/hotspot/share/gc/shenandoah/shenandoahNormalMode.cpp Wed Jan 15 20:04:51 2020 +0100 +++ b/src/hotspot/share/gc/shenandoah/shenandoahNormalMode.cpp Wed Jan 15 20:49:30 2020 +0100 @@ -34,10 +34,11 @@ void ShenandoahNormalMode::initialize_flags() const { + if (ShenandoahConcurrentRoots::can_do_concurrent_class_unloading()) { + FLAG_SET_DEFAULT(ShenandoahSuspendibleWorkers, true); + FLAG_SET_DEFAULT(VerifyBeforeExit, false); + } + SHENANDOAH_ERGO_ENABLE_FLAG(ExplicitGCInvokesConcurrent); SHENANDOAH_ERGO_ENABLE_FLAG(ShenandoahImplicitGCInvokesConcurrent); - if (ShenandoahConcurrentRoots::can_do_concurrent_class_unloading()) { - SHENANDOAH_ERGO_ENABLE_FLAG(ShenandoahSuspendibleWorkers); - SHENANDOAH_ERGO_DISABLE_FLAG(VerifyBeforeExit); - } Testing: hotspot_gc_shenandoah -- Thanks, -Aleksey

2 1

RFR (T) 8237217: Incorrect G1StringDedupEntry type used in StringDedupTable destructor
by Aleksey Shipilev 15 Jan '20

15 Jan '20

Bug: https://bugs.openjdk.java.net/browse/JDK-8237217 Spotted this when reading the strdedup code. This is a trivial leftover from JDK-8203641. G1StringDedupEntry symbol does not even exist, and the whole thing works because FREE_C_HEAP_ARRAY ignores that parameter. But it should be consistent anyway with constructor anyway. I would not bother with jdk-submit testing, as it looks pretty trivial. Fix: diff -r f7edb9ca045c src/hotspot/share/gc/shared/stringdedup/stringDedupTable.cpp --- a/src/hotspot/share/gc/shared/stringdedup/stringDedupTable.cpp Fri Jan 10 15:38:25 2020 +0100 +++ b/src/hotspot/share/gc/shared/stringdedup/stringDedupTable.cpp Wed Jan 15 19:47:47 2020 +0100 @@ -234,11 +234,11 @@ _buckets = NEW_C_HEAP_ARRAY(StringDedupEntry*, _size, mtGC); memset(_buckets, 0, _size * sizeof(StringDedupEntry*)); } StringDedupTable::~StringDedupTable() { - FREE_C_HEAP_ARRAY(G1StringDedupEntry*, _buckets); + FREE_C_HEAP_ARRAY(StringDedupEntry*, _buckets); } Testing: x86_64 fastdebug build; hotspot_gc_shenandoah -- Thanks, -Aleksey

3 3