December 2018 - hotspot-gc-dev

RFR(M): 8211424: Allocation of old generation of java heap on alternate memory devices - ParallelOld
by Kharbas, Kishor 21 Dec '18

21 Dec '18

Hi all, Requesting review of the patch for allocating old generation of parallelold gc on alternate memory devices such nv-dimm. The design of this implementation is explained on the bug page - https://bugs.openjdk.java.net/browse/JDK-8211424. This is subtask of https://bugs.openjdk.java.net/browse/JDK-8202286 which is implementation for parallel old gc. Please follow the parent issue here : https://bugs.openjdk.java.net/browse/JDK-8202286. (PS: this is continuation of old thread 'RFR(M): 8204908: NVDIMM for POGC and G1GC - ReserveSpace.cpp changes are mostly eliminated/no collector specific code.') Webrev: http://cr.openjdk.java.net/~kkharbas/8211424/webrev.04 http://cr.openjdk.java.net/~kkharbas/8211424/webrev.03_to_04 Thanks Kishor

3 30

RFR(M): 8211425: Allocation of old generation of java heap on alternate memory devices - G1 GC
by Kharbas, Kishor 21 Dec '18

21 Dec '18

Hi all, Requesting review of the patch for allocating old generation of g1 gc on alternate memory devices such nv-dimm. The design of this implementation is explained on the bug page - https://bugs.openjdk.java.net/browse/JDK-8211425 Please follow the parent issue here : https://bugs.openjdk.java.net/browse/JDK-8202286. Webrev: http://cr.openjdk.java.net/~kkharbas/8211425/webrev.00 Testing : Passed tier1_gc and tier1_runtime jtret tests. I added extra options "-XX:+UnlockExperimentalVMOptions -XX:AllocateOldGenAt=/home/Kishor" to each test. There are some tests which I had to exclude since they won't work with this flag. Example: tests in ConcMarkSweepGC which does not support this flag, tests requiring CompressedOops to be enabled, etc. Thanks Kishor

4 56

[11] RFR 8215637: client VM variant build error on zgc
by Boris Ulasevich 21 Dec '18

21 Dec '18

Hi all, Please review this fix for jdk-11. JDK-11 client VM variant build fails because zgc (enabled by default now) depends on opto headers. In jdk-12 opto header #includes are disabled by #ifdef COMPILER, but I see no simple change to backport it. Here is the minimal change for jdk-11. http://bugs.openjdk.java.net/browse/JDK-8215637 http://cr.openjdk.java.net/~bulasevich/8215637/webrev.01 thanks, Boris

3 3

[12] RFR (XS): 8215548: G1PeriodicGCSystemLoadThreshold needs to be a double
by Thomas Schatzl 20 Dec '18

20 Dec '18

Hi all, can I have reviews for this small change that changes the G1PeriodicGCSystemLoadThreshold value type from an uintx to a double? (Manual) Testing by Ruslan from jElastic showed that integers are too coarse for this value, as typical load values you want to compare with are ~0.5. Unfortunately this has not been discovered during review; also testing against a particular load value provided by the OS is hard, so there has not been a regression test for this particular flag where this probably would have been found out. :( CR: https://bugs.openjdk.java.net/browse/JDK-8215548 Webrev: http://cr.openjdk.java.net/~tschatzl/8215548/webrev/ Testing: Manual testing with a hacked gc/g1/TestPeriodicCollection.java test, and the command line range checking tests. hs-tier1-5 Thanks, Thomas

3 6

RFR (M): 8213229: Treat StringTable as weak in G1 young collections
by Thomas Schatzl 19 Dec '18

19 Dec '18

Hi all, can I have reviews that treat the string table as weak during young gc (too)? The change itself is pretty straightforward, i.e. not processing the string table as roots (if ClassUnloading is true - this seems to be the commonly used flag to turn this off), and at the end in some parallel phase purge the string table itself. Unfortunately this incurred some changes to the parallel cleaning tasks to allow doing this parallel phase in conjunction with string deduplication to avoid the additional worker thread startup, and share some code, which rippled through the code a bit. However this also makes it a bit easier imho to join other parallel phases like the weak handle processing during the post evacuation phase into this "partial cleanup" in follow-ups. CR: https://bugs.openjdk.java.net/browse/JDK-8213229 Webrev: http://cr.openjdk.java.net/~tschatzl/8213229/webrev/ Testing: hs-tier1-5, gc/shenandoah for shenandoah Thanks, Thomas

1 1

Re: RFR (M): 8212657: Implementation of JDK-8204089 Timely Reduce Unused Committed Memory
by Ruslan Synytsky 19 Dec '18

19 Dec '18

Hi guys, some new findings related to this topic. Previously we got a great question from Stefan Johansson: >> Another question, when >> running in the cloud, what load is the user expecting us to compare >> against, the overall system or the local container. I'm actually not >> entirely sure what the getloadavg() call return in case of running in a >> container. > Good question! It depends on the used container technology. In short, if it’s a system > container then it shows the load of the container, if it’s an application container then the > load of the host machine. There is an article on a related topic > https://jelastic.com/blog/java-and-memory-limits-in-containers-lxc-docker-a… I found more details / info that will be useful for end users, there is a quick summary: - VMs - the problem does not exist as JVM gets loadavg of the parent virtual machine it’s running in. - Containers - LXC - the problem does not exist, because LXCFS <https://github.com/lxc/lxcfs> is making Linux containers feel more like a virtual machine. - OpenVZ - the problem does not exist as every container has a virtualized view of /proc pseudo-filesystem - Docker and runC - by default loadavg will be provided from the host machine, which is a kind of problem for determining the real load inside a container. However, the recent improvements in runC engine are solving this issue: libcontainer: add /proc/loadavg to the white list of bind mount <https://github.com/opencontainers/runc/pull/1882>. Also there is a useful related article - LXCFS for Docker and K8S <https://medium.com/@Alibaba_Cloud/kubernetes-demystified-using-lxcfs-to-imp…>. So, we can assume that this solution will be available by default in the near future. Also, some kind of a quick summary / "a state of JVM elasticity": - G1 - the new options will be introduced soon: *G1PeriodicGCInterval, **G1PeriodicGCSystemLoadThreshold, G1PeriodicGCInvokesConcurrent, *the work is in progress. - Shenandoah - the leading GC in terms of elasticity at the moment (my personal opinion). Available options: *ShenandoahUncommitDelay, ShenandoahGuaranteedGCInterval.* - OpenJ9 - introduced special options: *IdleTuningGcOnIdle, IdleTuningCompactOnIdle, IdleTuningMinIdleWaitTime. *However, the uncommitment is not fully transparent for end users, as a result it's harder to track the real usage. The only way to measure the effect is to monitor resident memory size *RES* using *top*. Also it implements different approach for checking idle sate - based on *samplerThreadStateLogic.* For more details please check to this conversation <https://github.com/eclipse/openj9/issues/2312#issuecomment-431453020>. - ZGC - the work is in the progress, there is a quick patch <http://cr.openjdk.java.net/~pliden/zgc/zrelease_unused_heap/webrev.0> to support releasing of memory back to the OS in ZGC. If you want to try it out, the patch should apply to the latest jdk/jdk tree. Use the new option *-XX:+ZReleaseUnusedHeap*. A more refined version of the patch will most likely be upstreamed at some point in the future. - Azul C4 - seems like it's scalable vertically too. I sent clarification request to Deputy CTO, but have not received any feedback so far, so it's unknown how exactly it works. If anyone can share his/her personal experience than will be useful. Thanks everybody involved for moving this topic forward. Regards -- Ruslan CEO @ Jelastic <https://jelastic.com/>

4 5

RFR: 8215487: ZGC: ZRuntimeWorkers incorrectly identify themselves as ZWorkers
by Per Liden 17 Dec '18

17 Dec '18

ZThread::is_worker() incorrectly returns true when executed from a ZRuntimeWorkers context. This function should only return true when executed from a ZWorker context. This issue can lead to data corruption as a ZRuntimeWorker might read and write to data structures that are thread local to a ZWorker. With this patch, the ZRuntimeWorkers will execute an initialize task (similar to what ZWorkers already do) to initialize the "worker type" data. Bug: https://bugs.openjdk.java.net/browse/JDK-8215487 Webrev: http://cr.openjdk.java.net/~pliden/8215487/webrev.0 /Per

2 2

Re: RFR (M) 8201655: Add thread-enabled support for the Heap Sampling
by serguei.spitsyn＠oracle.com 12 Dec '18

12 Dec '18

Hi Alex, Nice catch! It is about the following fragment: 726 for (i = 0; i < thread_stats.number_threads; i++) { 727 if (strcmp(expected_name, thread_stats.threads[i])) { 728 return FALSE; 729 } else { 730 found_thread = TRUE; 731 } 732 } 733 return found_thread; 734 } Also, I'd also use 'count' in place of 'number'. It is to avoid association with thread identification number. Thanks, Serguei On 12/11/18 4:42 PM, Alex Menkov wrote: > Hi Jc, > > The fix looks good. > The only note is checkThreadSamplesOnlyFrom native function > implementation - the cycle looks confusing. > As far as I got the function should check that thread_stats contains > only 1 thread and name of the thread is the same as name of the > specified thread. > And for error analysis it would be great to provide good error > description. > So I'd make it like > > if (thread_stats.number_threads != 1) { > fprintf(stderr, "Wrong thread number: %d (expected 1)\n", > thread_stats.number_threads); > return FALSE; > } > if (strcmp(expected_name, thread_stats.threads[i]) != 0) { > fprintf(stderr, "Unexpected thread name: '%s' (expected '%s')\n", > thread_stats.threads[i], expected_name); > return FALSE; > } > return TRUE; > > --alex > > On 12/11/2018 15:11, serguei.spitsyn(a)oracle.com wrote: >> Hi Jc, >> >> Alex will take a look at the test update. >> >> Thanks, >> Serguei >> >> On 11/12/18 9:53 AM, JC Beyler wrote: >>> Hi Serguei, >>> >>> Thanks for the update and thanks for testing mach5. Serguei sent me >>> that the testing passed mach5 testing, could I get another review to >>> be able to push it? >>> >>> Thanks! >>> Jc >>> >>> On Tue, Nov 6, 2018 at 10:41 PM serguei.spitsyn(a)oracle.com >>> <mailto:serguei.spitsyn@oracle.com> <serguei.spitsyn(a)oracle.com >>> <mailto:serguei.spitsyn@oracle.com>> wrote: >>> >>> Hi Jc, >>> >>> Thank you for the update! >>> It looks good. >>> It is great that testing on your side is Okay. >>> >>> I'll submit a mach5 job soon (today or tomorrow). >>> >>> Thanks, >>> Serguei >>> >>> >>> On 11/6/18 20:03, JC Beyler wrote: >>>> Hi Serguei, >>>> >>>> You are right, I should have reverted the memAllocator.cpp file, >>>> sorry about that. >>>> >>>> Here is the new webrev: >>>> http://cr.openjdk.java.net/~jcbeyler/8201655/webrev.04/ >>>> <http://cr.openjdk.java.net/%7Ejcbeyler/8201655/webrev.04/> >>>> >>>> I think we are good by testing standards, like I >>>> said HeapMonitorThreadTest.java tests multiple threads. I did >>>> test an example with a thousand threads and I get the samples >>>> from 1000 threads so it seems to work there too. >>>> >>>> Per thread is tested via the new >>>> HeapMonitorThreadDisabledTest.java so I think we are good there >>>> too. >>>> >>>> I would recommend a mach-5 testing just in case for this one if >>>> you can, it will be better to have that reinsurance. >>>> >>>> Thanks for your help, >>>> Jc >>>> >>>> On Tue, Nov 6, 2018 at 4:29 PM <serguei.spitsyn(a)oracle.com >>>> <mailto:serguei.spitsyn@oracle.com>> wrote: >>>> >>>> Hi Jc, >>>> >>>> Not sure, I understand a motivation for this change: >>>> >>>> - if (JvmtiExport::should_post_sampled_object_alloc()) { >>>> + { >>>> >>>> Also, I'm not sure this is still needed: >>>> >>>> +#include "prims/jvmtiEventController.inline.hpp" >>>> +#include "prims/jvmtiThreadState.inline.hpp" >>>> >>>> I expected you'd just revert all the changes in the >>>> memAllocator.cpp. >>>> >>>> Also, it is up to you to make a decision if these >>>> performance-related fix is needed or not. >>>> >>>> But it needs to be well tested so that both global+thread >>>> event management works correctly. >>>> >>>> Thanks, >>>> Serguei >>>> >>>> >>>> On 11/6/18 9:42 AM, JC Beyler wrote: >>>>> Hi Serguei, >>>>> >>>>> Yes exactly it was an optimization. When using a 512k >>>>> sampling rate, I don't see a no real difference (the >>>>> overhead is anyway low for that sampling rate), I imagine >>>>> there would be a difference if trying to sample every >>>>> allocation or with a low sampling interval. But because you >>>>> are right and it is an optimization of the system and not a >>>>> functional need, I've reverted it and now the webrev is >>>>> updated here: >>>>> >>>>> Webrev: >>>>> http://cr.openjdk.java.net/~jcbeyler/8201655/webrev.03/ >>>>> <http://cr.openjdk.java.net/%7Ejcbeyler/8201655/webrev.03/> >>>>> Bug: https://bugs.openjdk.java.net/browse/JDK-8201655 >>>>> >>>>> The incremental webrev is here: >>>>> http://cr.openjdk.java.net/~jcbeyler/8201655/webrev.02_03/ >>>>> <http://cr.openjdk.java.net/%7Ejcbeyler/8201655/webrev.02_03/> >>>>> >>>>> Let me know what you think, >>>>> Jc >>>>> >>>>> On Mon, Nov 5, 2018 at 6:51 PM serguei.spitsyn(a)oracle.com >>>>> <mailto:serguei.spitsyn@oracle.com> >>>>> <serguei.spitsyn(a)oracle.com >>>>> <mailto:serguei.spitsyn@oracle.com>> wrote: >>>>> >>>>> Hi Jc, >>>>> >>>>> Okay, I see your point: the change in memAllocator.cpp >>>>> is for performance. >>>>> Do you have any measurements showing a performance >>>>> difference? >>>>> Also, do you need me to submit a mach5 test run? >>>>> >>>>> Thanks, >>>>> Serguei >>>>> >>>>> >>>>> On 11/5/18 15:14, JC Beyler wrote: >>>>>> Hi Serguei, >>>>>> >>>>>> First off, thanks as always for looking at this :-) Let >>>>>> me inline my answers: >>>>>> >>>>>> I actually "struggled" with this part of the change. My >>>>>> change is correct semantically and if you care about >>>>>> performance for when sampling a given thread. >>>>>> Your change will work semantically but the performance >>>>>> is the same as the global sampling. >>>>>> >>>>>> What I mean by working semantically is that that the >>>>>> tests and the code will work. However, this means that >>>>>> all threads will be doing the sampling work but when >>>>>> the code will post the event here: >>>>>> -> >>>>>> http://cr.openjdk.java.net/~jcbeyler/8201655/webrev.02/src/hotspot/share/pr… >>>>>> <http://cr.openjdk.java.net/%7Ejcbeyler/8201655/webrev.02/src/hotspot/share/…> >>>>>> >>>>>> (which is why your suggestion works, the change in >>>>>> jvmtiExport basically ensures only the threads >>>>>> requested are posting events) >>>>>> >>>>>> The code will check that we actually only post for >>>>>> threads we care about. The code above ensures that only >>>>>> threads that were requested to be sampling are the ones >>>>>> that are sampling internally. >>>>>> >>>>>> Note: I REALLY prefer your suggestion for two reasons: >>>>>> - We do not change the runtime/GC code at all, it >>>>>> remains "simple" >>>>>> - The overhead in the general case goes away and this >>>>>> is a NOP for my actual use-case from a performance >>>>>> point of view (sampling every thread) >>>>>> >>>>>> But: >>>>>> - Then sampling per thread really is just telling the >>>>>> system don't pollute the callbacks, though internally >>>>>> you are doing all the work anyway. >>>>>> >>>>>> Let me know which you prefer :) >>>>>> >>>>>> >>>>>> Also, do you see that enabling the sampling >>>>>> events globally still works? >>>>>> >>>>>> >>>>>> Yes, otherwise HeapMonitorThreadTest.java would fail >>>>>> since it checks that. >>>>>> >>>>>> http://cr.openjdk.java.net/%7Ejcbeyler/8201655/webrev.02/test/hotspot/jtreg… >>>>>> >>>>>> A couple of places where err is declared as int >>>>>> instead of jvmtiError: >>>>>> 714 int err; >>>>>> 742 int err; Should not be silent in a case of >>>>>> JVMTI error: 744 err = >>>>>> (*jvmti)->GetThreadInfo(jvmti, thread, &info); >>>>>> 745 if (err != JVMTI_ERROR_NONE) { >>>>>> 746 return; >>>>>> >>>>>> >>>>>> >>>>>> Done and done, I added a fprintf on stderr saying the >>>>>> GetThreadInfo failed and the test is ignoring the add >>>>>> count. >>>>>> >>>>>> Thanks again for looking and let me know what you think, >>>>>> Jc >>>>>> >>>>>> On Mon, Nov 5, 2018 at 2:25 PM >>>>>> serguei.spitsyn(a)oracle.com >>>>>> <mailto:serguei.spitsyn@oracle.com> >>>>>> <serguei.spitsyn(a)oracle.com >>>>>> <mailto:serguei.spitsyn@oracle.com>> wrote: >>>>>> >>>>>> Hi Jc, >>>>>> >>>>>> It looks good in general but I have some comments >>>>>> below. >>>>>> >>>>>> >>>>>> http://cr.openjdk.java.net/%7Ejcbeyler/8201655/webrev.02/src/hotspot/share/… >>>>>> >>>>>> +static bool thread_enabled_for_one_jvmti_env() { >>>>>> + JavaThread *thread = JavaThread::current(); >>>>>> + JvmtiThreadState *state = >>>>>> thread->jvmti_thread_state(); >>>>>> + if (state == NULL) { >>>>>> + return false; >>>>>> + } >>>>>> + >>>>>> + JvmtiEnvThreadStateIterator it(state); >>>>>> + for (JvmtiEnvThreadState* ets = it.first(); ets >>>>>> != NULL; ets = it.next(ets)) { >>>>>> + if >>>>>> (ets->is_enabled(JVMTI_EVENT_SAMPLED_OBJECT_ALLOC)) { >>>>>> + return true; >>>>>> + } >>>>>> + } >>>>>> + >>>>>> + return false; >>>>>> +} >>>>>> + >>>>>> void >>>>>> MemAllocator::Allocation::notify_allocation_jvmti_sampler() { >>>>>> // support for JVMTI VMObjectAlloc event >>>>>> (no-op if not enabled) >>>>>> JvmtiExport::vm_object_alloc_event_collector(obj()); >>>>>> if >>>>>> (!JvmtiExport::should_post_sampled_object_alloc()) { >>>>>> // Sampling disabled >>>>>> return; >>>>>> } >>>>>> + // Sampling is enabled for at >>>>>> least one thread, >>>>>> is it this one? >>>>>> + if (!thread_enabled_for_one_jvmti_env()) { >>>>>> + return; >>>>>> + } >>>>>> + I don't think you need this change as this >>>>>> condition already does it: if >>>>>> (!JvmtiExport::should_post_sampled_object_alloc()) { >>>>>> // Sampling disabled >>>>>> return; >>>>>> } >>>>>> >>>>>> Please, look at the following line in the >>>>>> jvmtiEventController.cpp: >>>>>> JvmtiExport::set_should_post_sampled_object_alloc((any_env_thread_enabled >>>>>> & SAMPLED_OBJECT_ALLOC_BIT) != 0); >>>>>> >>>>>> I hope, testing will prove my suggestion is >>>>>> correct. >>>>>> Also, do you see that enabling the sampling >>>>>> events globally still works? >>>>>> >>>>>> >>>>>> http://cr.openjdk.java.net/%7Ejcbeyler/8201655/webrev.02/test/hotspot/jtreg… >>>>>> >>>>>> A couple of places where err is declared as int >>>>>> instead of jvmtiError: >>>>>> 714 int err; >>>>>> 742 int err; Should not be silent in a case of >>>>>> JVMTI error: 744 err = >>>>>> (*jvmti)->GetThreadInfo(jvmti, thread, &info); >>>>>> 745 if (err != JVMTI_ERROR_NONE) { >>>>>> 746 return; >>>>>> >>>>>> >>>>>> Thanks, >>>>>> Serguei >>>>>> >>>>>> >>>>>> On 10/26/18 10:48, JC Beyler wrote: >>>>>>> Hi all, >>>>>>> >>>>>>> When working on the heap sampling, I had promised >>>>>>> to do the per thread event so here it is! >>>>>>> >>>>>>> Could I get a review for this: >>>>>>> Webrev: >>>>>>> http://cr.openjdk.java.net/~jcbeyler/8201655/webrev.02/ >>>>>>> <http://cr.openjdk.java.net/%7Ejcbeyler/8201655/webrev.02/> >>>>>>> Bug: >>>>>>> https://bugs.openjdk.java.net/browse/JDK-8201655 >>>>>>> >>>>>>> I was thinking of adding GC-dev for the >>>>>>> memAllocator change once I get favorable reviews >>>>>>> for the rest of the change. >>>>>>> >>>>>>> I've done a bit of performance testing and on the >>>>>>> Dacapo benchmark I see no change in performance >>>>>>> when turned off (logical, any code change is >>>>>>> behind a flag check already in place) and when >>>>>>> turned on it is comparable to the current >>>>>>> performance. >>>>>>> >>>>>>> (More information is: I see a very slight >>>>>>> degradation if we are doing 512k sampling but no >>>>>>> degradation at 2MB). >>>>>>> >>>>>>> Thanks, >>>>>>> Jc >>>>>> >>>>>> >>>>>> >>>>>> -- >>>>>> Thanks, >>>>>> Jc >>>>> >>>>> >>>>> >>>>> -- >>>>> Thanks, >>>>> Jc >>>> >>>> >>>> >>>> -- >>>> Thanks, >>>> Jc >>> >>> >>> >>> -- >>> >>> Thanks, >>> Jc >>

3 3

RFR(S): 8215114: Fix indent and dead code in GCPolicyCounters
by Man Cao 12 Dec '18

12 Dec '18

Hi all, Can I have reviews for this small cleanup? Webrev: https://cr.openjdk.java.net/~manc/8215114/webrev.00/ Bug: https://bugs.openjdk.java.net/browse/JDK-8215114 Thanks, Man

3 4

Re: RFR (S): 6490394: G1: Allow heap shrinking / memory unmapping after reclaiming regions during Remark
by Ruslan Synytsky 12 Dec '18

12 Dec '18

Dear Thomas and Sangheon, hope you are doing great. I just noticed an important improvement related to vertical scaling and would like to clarify details mentioned in the release notes <https://bugs.openjdk.java.net/browse/JDK-8211756> *> By default, G1 may now give back Java heap memory to the operating system during any concurrent mark cycle. G1 will respect default Java heap sizing policies at that time. * Does it mean that G1 is smart enough now by default and users do not need to deal with *loadaverage* and *G1PeriodicGCInterval *that we introduced in JEP 346 <https://bugs.openjdk.java.net/browse/JDK-8204089>? *> This behavior may be disabled by changing minimum Java heap size via the -Xms option.* At the same time if a user adjusts Xms option then he/she does not get this "smart behaviour" at all. Are these rephrased statements correct? Thank you -- Ruslan CEO @ Jelastic <https://jelastic.com/>

2 5