June 2020 - hotspot-gc-dev

RFR (S): 8248322: G1: Refactor full collection sizing code
by Thomas Schatzl 09 Jul '20

09 Jul '20

Hi all, can I have reviews for this small refactoring changes that moves some heap sizing policy related code into a the "right" place (into the heap sizing policy class), and refactors it a bit. CR: https://bugs.openjdk.java.net/browse/JDK-8248322 Webrev: http://cr.openjdk.java.net/~tschatzl/8248322/webrev/ Testing: tier1 Thanks, Thomas

3 3

RFR(S) [15] : 8208207 : Test nsk/stress/jni/gclocker/gcl001 fails after co-location
by Igor Ignatyev 30 Jun '20

30 Jun '20

http://cr.openjdk.java.net/~iignatyev//8208207/webrev.00 > 38 lines changed: 1 ins; 4 del; 33 mod; Hi all, could you please review the small patch which fixes gcl001 test and returns it back to work? the issue reported in the bug (assert(!JavaThread::current()->in_critical()) failed: Would deadlock) was caused by calls to JNI functions other than (Get|Release).*Critical within a critical region. after this get fixed by moving Get.*Length calls outside of critical regions, the test started to fail w/ "Data validation failure" message. this was b/c of returning 0 and w/o sorting arrays in case isCopy was changed to TRUE by Get.*Critical. as there is no way to guarantee that we won't get a copy of array, I decided to remove checks of isCopy which, although might slightly change that this test check, makes the test more robust. JBS: https://bugs.openjdk.java.net/browse/JDK-8208207 webrev: http://cr.openjdk.java.net/~iignatyev//8208207/webrev.00 testing: - run the test against {linux,windows,macosx-x64}-{product,fastdebug} w/ default GC - run the test against macosx-x64-slowdebug w/ Serial,Parallel,G1,ZGC Thanks, -- Igor

2 1

[11u] RFR: 8244214: Add paddings for TaskQueuSuper to reduce false-sharing cache contention
by Patrick Zhang OS 30 Jun '20

30 Jun '20

Hi Could I ask for a review of this simple patch which takes a tiny part from the original ticket JDK-8243326 [1]. The reason that I do not want a full backport is, the majority of the patch at jdk/jdk [2] is to clean up the volatile use and may be not very meaningful to 11u, furthermore the context (dependencies on atomic.hpp refactor) is too complicated to generate a clear backport (I tried, ~81 files need to be changed). The purpose of having this one-line change to 11u is, the two volatile variables in TaskQueuSuper: _bottom, _age and corresponding atomic operations upon, may cause severe cache contention inside GC with larger number of threads, i.e., specified by -XX:ParallelGCThreads=##, adding paddings (up to DEFAULT_CACHE_LINE_SIZE) in-between can reduce the possibility of false-sharing cache contention. I do not need the paddings before _bottom and after _age from the original patch [2], because the instances of TaskQueuSuper are usually (always) allocated in a set of queues, in which they are naturally separated. Please review, thanks. JBS: https://bugs.openjdk.java.net/browse/JDK-8248214 Webrev: http://cr.openjdk.java.net/~qpzhang/8248214/webrev.01/ Testing: tier1-2 pass with the patch, commercial benchmarks and small C++ test cases (to simulate the data struct and work-stealing algorithm atomics) validated the performance, no regression. By the way, I am going to request for 8u backport as well once 11u would have it. [1] https://bugs.openjdk.java.net/browse/JDK-8243326 Cleanup use of volatile in taskqueue code [2] https://hg.openjdk.java.net/jdk/jdk/rev/252a1602b4c6 Regards Patrick

4 11

RE: RFR(L): 8215624: add parallel heap inspection support for jmap histo(G1)(Internet mail)
by Hohensee, Paul 30 Jun '20

30 Jun '20

For the interface, I'd use "parallel" instead of "parallelThreadNum". All the other options are lower case, and it's a lot easier to type "parallel". I took the liberty of updating the CSR. If you're ok with it, you might want to change variable names and such, plus of course JMap.usage. Thanks, Paul On 4/22/20, 2:29 AM, "serviceability-dev on behalf of linzang(臧琳)" <serviceability-dev-bounces(a)openjdk.java.net on behalf of linzang(a)tencent.com> wrote: Dear Stefan, Thanks a lot! I agree with you to decouple the heap inspection code with GC's. I will start from your POC code, may discuss with you later. BRs, Lin On 2020/4/22, 5:14 PM, "Stefan Karlsson" <stefan.karlsson(a)oracle.com> wrote: Hi Lin, I took a look at this earlier and saw that the heap inspection code is strongly coupled with the CollectedHeap and G1CollectedHeap. I'd prefer if we'd abstract this away, so that the GCs only provide a "parallel object iteration" interface, and the heap inspection code is kept elsewhere. I started experimenting with doing that, but other higher-priority (to me) tasks have had to take precedence. I've uploaded my work-in-progress / proof-of-concept: https://cr.openjdk.java.net/~stefank/8215624/webrev.01.delta/ https://cr.openjdk.java.net/~stefank/8215624/webrev.01/ The current code doesn't handle the lifecycle (deletion) of the ParallelObjectIterators. There's also code left unimplemented in around CollectedHeap::run_task. However, I think this could work as a basis to pull out the heap inspection code out of the GCs. Thanks, StefanK On 2020-04-22 02:21, linzang(臧琳) wrote: > Dear all, > May I ask you help to review? This RFR has been there for quite a while. > Thanks! > > BRs, > Lin > > > On 2020/3/16, 5:18 PM, "linzang(臧琳)" <linzang(a)tencent.com> wrote:> > >> Just update a new path, my preliminary measure show about 3.5x speedup of jmap histo on a nearly full 4GB G1 heap (8-core platform with parallel thread number set to 4). >> webrev: http://cr.openjdk.java.net/~lzang/jmap-8214535/8215624/webrev_02/ >> bug: https://bugs.openjdk.java.net/browse/JDK-8215624 >> CSR: https://bugs.openjdk.java.net/browse/JDK-8239290 >> BRs, >> Lin >> > On 2020/3/2, 9:56 PM, "linzang(臧琳)" <linzang(a)tencent.com> wrote: >> > >> > Dear all, >> > Let me try to ease the reviewing work by some explanation :P >> > The patch's target is to speed up jmap -histo for heap iteration, from my experience it is necessary for large heap investigation. E.g in bigData scenario I have tried to conduct jmap -histo against 180GB heap, it does take quite a while. >> > And if my understanding is corrent, even the jmap -histo without "live" option does heap inspection with heap lock acquired. so it is very likely to block mutator thread in allocation-sensitive scenario. I would say the faster the heap inspection does, the shorter the mutator be blocked. This is parallel iteration for jmap is necessary. >> > I think the parallel heap inspection should be applied to all kind of heap. However, consider the heap layout are different for GCs, much time is required to understand all kinds of the heap layout to make the whole change. IMO, It is not wise to have a huge patch for the whole solution at once, and it is even harder to review it. So I plan to implement it incrementally, the first patch (this one) is going to confirm the implemention detail of how jmap accept the new option, passes it to attachListener of the jvm process and then how to make the parallel inspection closure be generic enough to make it easy to extend to different heap layout. And also how to implement the heap inspection in specific gc's heap. This patch use G1's heap as the begining. >> > This patch actually do several things: >> > 1. Add an option "parallelThreadNum=<N>" to jmap -histo, the default behavior is to set N to 0, means let's JVM decide how many threads to use for heap inspection. Set this option to 1 will disable parallel heap inspection. (more details in CSR: https://bugs.openjdk.java.net/browse/JDK-8239290) >> > 2. Make a change in how Jmap passing arguments, changes in http://cr.openjdk.java.net/~lzang/jmap-8214535/8215624/webrev_01/src/jdk.jc…, originally it pass options as separate arguments to attachListener, this patch change to that all options be compose to a single string. So the arg_count_max in attachListener.hpp do not need to be changed, and hence avoid the compatibility issue, as disscussed at https://mail.openjdk.java.net/pipermail/serviceability-dev/2019-March/02733… >> > 3. Add an abstract class ParHeapInspectTask in heapInspection.hpp / heapInspection.cpp, It's work(uint worker_id) method prepares the data structure (KlassInfoTable) need for every parallel worker thread, and then call do_object_iterate_parallel() which is heap specific implementation. I also added some machenism in KlassInfoTable to support parallel iteration, such as merge(). >> > 4. In specific heap (G1 in this patch), create a subclass of ParHeapInspectTask, implement the do_object_iterate_parallel() for parallel heap inspection. For G1, it simply invoke g1CollectedHeap's object_iterate_parallel(). >> > 5. Add related test. >> > 6. it may be easy to extend this patch for other kinds of heap by creating subclass of ParHeapInspectTask and implement the do_object_iterate_parallel(). >> > >> > Hope these info could help on code review and initate the discussion :-) >> > Thanks! >> > >> > BRs, >> > Lin >> > >On 2020/2/19, 9:40 AM, "linzang(臧琳)" <linzang(a)tencent.com> wrote:. >> > > >> > > Re-post this RFR with correct enhancement number to make it trackable. >> > > please ignore the previous wrong post. sorry for troubles. >> > > >> > > webrev: http://cr.openjdk.java.net/~lzang/jmap-8214535/8215624/webrev_01/ >> > > Hi bug: https://bugs.openjdk.java.net/browse/JDK-8215624 >> > > CSR: https://bugs.openjdk.java.net/browse/JDK-8239290 >> > > -------------- >> > > Lin >> > > >Hi Lin, > > > > > >> > > >Could you, please, re-post your RFR with the right enhancement number in >> > > >the message subject? >> > > >It will be more trackable this way. >> > > > >> > > >Thanks, >> > > >Serguei >> > > > >> > > > >> > > >On 2/17/20 10:29 PM, linzang(臧琳) wrote: >> > > >> Dear David, >> > > >> Thanks a lot! >> > > >> I have updated the refined code to http://cr.openjdk.java.net/~lzang/jmap-8214535/8215264/webrev_01/. >> > > >> IMHO the parallel heap inspection can be extended to all kinds of heap as long as the heap layout can support parallel iteration. >> > > >> Maybe we can firstly use this webrev to discuss how to implement it, because I am not sure my current implementation is an appropriate way to communicate with collectedHeap, then we can extend the solution to other kinds of heap. >> > > >> >> > > >> Thanks, >> > > >> -------------- >> > > >> Lin >> > > >>> Hi Lin, >> > > >>> >> > > >>> Adding in hotspot-gc-dev as they need to see how this interacts with GC >> > > >>> worker threads, and whether it needs to be extended beyond G1. >> > > >>> >> > > >>> I happened to spot one nit when browsing: >> > > >>> >> > > >>> src/hotspot/share/gc/shared/collectedHeap.hpp >> > > >>> >> > > >>> + virtual bool run_par_heap_inspect_task(KlassInfoTable* cit, >> > > >>> + BoolObjectClosure* filter, >> > > >>> + size_t* missed_count, >> > > >>> + size_t thread_num) { >> > > >>> + return NULL; >> > > >>> >> > > >>> s/NULL/false/ >> > > >>> >> > > >>> Cheers, >> > > >>> David > > > > >>> >> > > >>> On 18/02/2020 2:15 pm, linzang(臧琳) wrote: >> > > >>>> Dear All, >> > > >>>> May I ask your help to review the follow changes: >> > > >>>> webrev: >> > > >>>> http://cr.openjdk.java.net/~lzang/jmap-8214535/8215264/webrev_00/ >> > > >>>> bug: https://bugs.openjdk.java.net/browse/JDK-8215624 >> > > >>>> related CSR: https://bugs.openjdk.java.net/browse/JDK-8239290 >> > > >>>> This patch enable parallel heap inspection of G1 for jmap histo. >> > > >>>> my simple test shown it can speed up 2x of jmap -histo with >> > > >>>> parallelThreadNum set to 2 for heap at ~500M on 4-core platform. >> > > >>>> >> > > >>>> ------------------------------------------------------------------------ >> > > >>>> BRs, >> > > >>>> Lin >> > > >> > >> > > > > > >

3 6

RE: [11u] RFR: 8244241: Add paddings for TaskQueueSuper to reduce false-sharing cache contention
by Lindenmaier, Goetz 29 Jun '20

29 Jun '20

Hi, Christoph and I sent several replies to this mail. For some reason they are not delivered to the archives. I get them back with security scan issues ... So I write a clean answer here, maybe this works: We saw build problems on mac. It does not like sizeof(_bottom). If I use sizeof(uint) it works on mac. Uint is also the term jdk/jdk uses here. I put it into our CI again to make sure all platforms build. I'll update you tomorrow if it worked. Also I think we should move the line up, just below the declaration of _bottom, as it now depends on the type used there. Patrick, the bug id in the subject of this mail is wrong. I kept the wrong one in this answer so the mail thread does not break. Also, please remove the "Summary:" line in webrev.02. It only repeats the bug title, and thus is redundant. You can use the Summary line if you want to add more than the bug title. You might add Summary: This is a downport of a part of JDK-8243326 Best regards, Goetz.

1 0

RE: [11u] RFR: 8244214: Add paddings for TaskQueuSuper to reduce false-sharing cache contention
by Lindenmaier, Goetz 29 Jun '20

29 Jun '20

Hi, Christoph and I sent several replies to this mail. For some reason they are not delivered to the archives. I get them back with security scan issues ... So I write a clean answer here, maybe this works: We saw build problems on mac. It does not like sizeof(_bottom). If I use sizeof(uint) it works on mac. Uint is also the term jdk/jdk uses here. I put it into our CI again to make sure all platforms build. I'll update you tomorrow if it worked. Also I think we should move the line up, just below the declaration of _bottom, as it now depends on the type used there. Patrick, the bug id in the subject of this mail is wrong. I kept the wrong one in this answer so the mail thread does not break. Also, please remove the "Summary:" line in webrev.02. It only repeats the bug title, and thus is redundant. You can use the Summary line if you want to add more than the bug title. You might add "Summary: This is a downport of a part of JDK-8243326" Best regards, Goetz. Best regards, Goetz.

1 0

[16] RFR 8248227: Shenandoah: Refactor Shenandoah::heap() to match other GCs
by Zhengyu Gu 29 Jun '20

29 Jun '20

Please review this small patch that refactors Shenandoah::heap() to match other GCs. Bug: https://bugs.openjdk.java.net/browse/JDK-8248227 Webrev: http://cr.openjdk.java.net/~zgu/JDK-8248227/webev.00/index.html Test: hotspot_gc_shenandoah Thanks, -Zhengyu

2 3

RFR: 8232782: Shenandoah: streamline post-LRB CAS barrier (aarch64)
by Nilsen, Kelvin 27 Jun '20

27 Jun '20

See http://cr.openjdk.java.net/~kdnilsen/JDK-8232782/webrev.00/ This patch addresses the problem described in https://bugs.openjdk.java.net/browse/JDK-8232782 The implementation mimics the behavior of the recently revised x86 implementation of cmpxchg_oop with slight refinements: X86 version: Step 1: Try CAS Step 2: if CAS fails, check if original memory holds equivalent from-space pointer Step 3: Use CAS to overwrite memory with equivalent to-space pointer Step 4: Try CAS again Step 5: Return boolean result to indicate success or failure AARCH64 version: Step 1: Try CAS Step 2: if CAS fails, check if original memory holds equivalent from-space pointer Step 3 (differs): Do not overwrite memory with equivalent to-space pointer, Instead, run the original CAS request with from-space pointer as the "expected" value. If this succeeds, we're done. If this fails, go back to step 1 and try that again. Step 5: Return boolean result to indicate success or failure This patch satisfies tier1, tier2, and hotspot_gc_shenandoah regression tests on Ubuntu 18.04.4 LTS (GNU/Linux 5.3.0-1023-aws aarch64). I have also run an "extreme" garbage collection workload for 20 minutes without problem. Is this ok to merge? (This is a repost of a message that was posted and bounced yesterday.)

3 9

RFR (S): 8165501: Serial, Parallel and G1 should track the time since last gc for millis_since_last_gc() with full precision
by Thomas Schatzl 26 Jun '20

26 Jun '20

Hi all, can I get reviews for this small change that unifies code a bit between Serial, Parallel and G1 with regards to tracking the time stamp for CollectedHeap::millis_since_last_gc? In particular, it names the members uniformly, and removes an unnecessary division by internally storing raw os::JavaTimeNanos instead of some intermediate result. Other collectors are not affected because they calculate millis_since_last_gc from an internal clock source. There is no functionality change CR: https://bugs.openjdk.java.net/browse/JDK-8165501 Webrev: http://cr.openjdk.java.net/~tschatzl/8165501 Testing: tier1-5 with 8243974, 8248221; see also 8248221 for manual testing info Thanks, Thomas

3 5

RFR (S): 8248221: G1: millis_since_last_gc updated at wrong time
by Thomas Schatzl 26 Jun '20

26 Jun '20

Hi all, can I have reviews for this change that fixes the location at which the millis_since_last_gc timestamp is updated. The spec of the only user in sun.rmi.transport.GC.maxObjectInspectionAge says: * Returns the maximum <em>object-inspection age</em>, which is the number * of real-time milliseconds that have elapsed since the * least-recently-inspected heap object was last inspected by the garbage * collector. Currently we do that update only at young gc, which is wrong. We should do that if/when a complete liveness analysis cycle has finished instead, i.e. end of marking/end of full gc. I did some testing using the TestMillisSinceLastGC.java jtreg test in http://cr.openjdk.java.net/~tschatzl/8248221/webrev.test/. As expected, it failed for G1 but all other GCs tested there pass (Epsilon will probably also fail, because it never does any gc so I did not even try). However the test intrinsically depends on timing i.e. the duration between some GC and checking the CollectedHeap::millis_since_last_gc() result, so I did not add it to this webrev. While in my ci testing it did not fail, I deemed it too risky to add. I also eyeballed that the output is still reasonable (i.e. no unit problem introduced by 8165501) using its log output for all affected collectors. Depends on 8243974. CR: https://bugs.openjdk.java.net/browse/JDK-8248221 Webrev: http://cr.openjdk.java.net/~tschatzl/8248221/webrev/ Testing: tier1-5 with 8243974 and 8165501, manual testing as described above. Thanks, Thomas

3 5