January 2020 - hotspot-gc-dev

RFR: 8236815: Shenandoah: Fix weak roots in final Traversal GC phase
by Roman Kennke 08 Jan '20

08 Jan '20

We're not fixing up all weak roots in final-traversal. But we have to, because weak roots are not scanned+evacuated at init-traversal, and may thus keep dangling pointers that would leak out to the next cycle. This can lead to heap corruption, crashes, etc. Bug: https://bugs.openjdk.java.net/browse/JDK-8236815 Webrev: http://cr.openjdk.java.net/~rkennke/JDK-8236815/webrev.00/ Testing: several runs of hotspot_gc_shenandoah, which *sometimes* exposed the bug. I couldn't reproduce it. I suggest to give it more spins in CI. Can I please get a review? Thanks, Roman

2 1

[14] RFR (S): 8235934: gc/g1/TestGCLogMessages.java fails with 'DerivedPointerTable Update' found
by Thomas Schatzl 08 Jan '20

08 Jan '20

Hi all, can I have reviews for this testbug where there is a mismatch between "C2 compiler is enabled" and "C2 compiler is compiled in" in verifying output messages. I.e. G1 prints some additional log messages if the C2 compiler is compiled in, but the test checks this message for (non-)existence if the C2 compiler is enabled. Since there are a few flags that can toggle compiler use even when compiled in (UseCompiler, TieredStopAtLevel<=3, ...) the GC prints that message but the test does not expect it. The fix is to add a whitebox method that specifically returns whether the C2 compiler is compiled in or not, to be used by the test. I would like to push this to 14 even if it is P4 because of the test bug exemption, returning unnecessary reproducable errors. CR: https://bugs.openjdk.java.net/browse/JDK-8235934 Webrev: http://cr.openjdk.java.net/~tschatzl/8235934/webrev/ Testing: hs-tier1-3, local runs of TestGCLogMessages.java Thanks, Thomas

3 6

RFR[14]: 8235751: Assertion when triggering concurrent cycle during shutdown
by Kim Barrett 07 Jan '20

07 Jan '20

Please review this change to G1's handling of requests to initiate concurrent marking. When such a request is made during shutdown processing, after the cm thread has been stopped, the request to initiate concurrent marking is ignored. This could lead to an assertion failure for user requested GCs (System.gc and via agent) by a thread that has not yet been brought to a halt, because the possibility of such a request being ignored was missed when the assertion was recently added by JDK-8232588. We now report to the GC-invoking thread when initiation of concurrent marking has been suppressed because termination of the cm thread has been requested. In that case the GC invocation is considered finished. CR: https://bugs.openjdk.java.net/browse/JDK-8235751 Webrev: https://cr.openjdk.java.net/~kbarrett/8235751/open.00/ Testing: mach5 tier1-5 Locally (linux-x64) reproduced fairly quickly the failure using the approach described in the CR; after applying the proposed chage, failed to reproduce.

3 4

RFR: 8236732: Shenandoah: Stricter placement for oom-evac scopes
by Roman Kennke 07 Jan '20

07 Jan '20

I'm currently looking at a deadlock with the derby benchmark which involves oom-scopes and new concurrent-class-unloading. Currently, we have sprinkled OOM-evac scopes all over the place: - In the main evac-loop (of course) - In the LRB (of course) - In various places The latter is very questionable and has repeatedly lead to problems in the past. The trouble was usually that some weird path would dive into evacuation with a GC worker, although the oom-scope was already held at an outer scope. It becomes really bad when locks are involved, e.g. the heap-lock, code-cache-lock and recently the per-nmethod locks. This is very deadlock-prone. The way out is to be very strict about where we place the oom-scopes. They should *only* be very close to SH::evacuate_object(), and they should *always* be the innermost scopes, inside any possible locks. Placement must be such that both conditions are rather obviously met. The biggest trouble here is Traversal GC: since it does *both* evacs and other stuff during traversal, it dives into LRB through various paths while GC threads holding the evac-scope. The solution is to only enter evac-scope very closely to SH::evacuate_object() at the expense of doing it quite often during traversal. I prefer to have a clear way to do it though, instead of the mess that we currently have. Bug: https://bugs.openjdk.java.net/browse/JDK-8236732 Webrev: http://cr.openjdk.java.net/~rkennke/JDK-8236732/webrev.00/ Testing: hotspot_gc_shenandoah, the specjvm/derby benchmark that troubled me with deadlock is now looking clean Ok? Roman

2 2

RFR: 8236073: G1: Use SoftMaxHeapSize to guide GC heuristics
by Liang Mao 07 Jan '20

07 Jan '20

Hi Thomas, As we previously discussed, I use the concurrent heap uncommit/commit mechanism to implement the SoftMaxHeapSize for G1. It is also for the further 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. 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, Liang ------------------------------------------------------------------ From:MAO, Liang <maoliang.ml(a)alibaba-inc.com> Send Time:2019 Oct. 14 (Mon.) 11:52 To:Thomas Schatzl <thomas.schatzl(a)oracle.com>; hotspot-gc-dev <hotspot-gc-dev(a)openjdk.java.net> Subject:Re: G1 patch of elastic Java heap Hi Thomas, Thank you for the recognition:) Since we both agree on some clear specific points, I will try to extract them from current implementation and create a patch in OpenJDK upstream branch so we can continue discussion on the code level. Thanks, Liang ------------------------------------------------------------------ From:Thomas Schatzl <thomas.schatzl(a)oracle.com> Send Time:2019 Oct. 12 (Sat.) 23:00 To:"MAO, Liang" <maoliang.ml(a)alibaba-inc.com>; hotspot-gc-dev <hotspot-gc-dev(a)openjdk.java.net> Subject:Re: G1 patch of elastic Java heap Hi, On Sat, 2019-10-12 at 19:51 +0800, Liang Mao wrote: > Hi Thomas, > > The manual generation limit can be put aside currently since we know > it might not be so general for a GC. We can focus on how to change > heap size and return memory in runtime first. > > GCTimeRatio is a good metric to measure the health of a Java > application and I have considered to use that. But finally I chose > a simple way just like the periodic old GC. Guarantee a long > enough young GC interval is an alternative way to make sure the > GCTimeRatio at a heathy state. > I'm absolutely ok to use GCTimeRatio instead of the fixed young GC > interval. This part is same to ZGC or Shenandoah for how to balance > the desired memory size and GC frequency. I'm open to any good > solution and we are already in the same page for this issue > I think:) +1 > A big difference of our implementation is evaluating heap resizing in > any young GC instead of a concurrent gc cycle which I think is > swifter and more immmediate. The concurrent map/unmap > mechanism gets rid of the additional pause time. My thought is the > heap shrink/expand can be all determined in young GC pause and > performed in concurrent thread which could exclude the > considerable time cost by OS interface. Most of our Java users are > intolerant to those pause pikes caused by page fault which can be up > to seconds. And we also found the issue of time cost by map/unmap in > ZGC. > > A direct advantage of the young GC resizing and concurrent memory > free machanism is for implementing SoftMaxHeapSize. The heap size can > be changed after last mixed GC. The young GC won't have longer > pause and the memory can be freed concurrently without side effect. Agree and agree. Both evaluating and giving back memory at any gc sounds nice, and doing that without incurring the costs in the pause is even better :) Thanks, Thomas

1 0

[15] RFR(XS) 8236681: Shenandoah: Disable concurrent class unloading flag if no class unloading for the GC cycle
by Zhengyu Gu 07 Jan '20

07 Jan '20

Please review this small patch that disables concurrent class unloading if there is no class unloading for the particular GC cycle. This is not a fatal error, but can confuse verifier. Bug: https://bugs.openjdk.java.net/browse/JDK-8236681 Webrev: http://cr.openjdk.java.net/~zgu/JDK-8236681/webrev.00/ Test: hotspot_gc_shenandoah (fastdebug and release) Thanks, -Zhengyu

2 1