March 2019 - hotspot-gc-dev

RFR: 8208636: Incresing bottom instead of decreasing top when an owner thread fails to take the last task
by Michihiro Horie 13 Apr '20

13 Apr '20

Dear all, Would you please review the following change? This is not a bug fix but refactoring. The taskqueue is implemented based on the ABP work-stealing algorithm as noted in comment. (It looks Chase-Lev work-stealing, which addressed the overflow problem of ABP work-stealing, but core algorithm is the same.) 231 // Arora, N. S., Blumofe, R. D., and Plaxton, C. G. 232 // Thread scheduling for multiprogrammed multiprocessors. 233 // Theory of Computing Systems 34, 2 (2001), 115-144. Correctness is presented with the following paper as also noted: 241 // Le, N. M., Pop, A., Cohen A., and Nardell, F. Z. 242 // Correct and efficient work-stealing for weak memory models 243 // Proceedings of the 18th ACM SIGPLAN symposium on Principles and 244 // practice of parallel programming (PPoPP 2013), 69-80 There is a slight difference between the current taskqueue and Chase-Lev's deque. When an owner thread and thieves try to take the last task in the deque and a thief succeeds in stealing the task, top is decreased by the owner thread in taskqueue, while bottom is increased in Chase-Lev's deque. Le et al. use the characteristics that top monotonically increases for the proofs in their paper. I think there is no reason to decrease top in taskqueue. One tricky thing in current taskqueue is that loading a task before the CAS trial in pop_global() avoids the side effect that owner thread can push (thus, overwrite with) a new task in the same slot at which a thief stole the last task. I think it is more straightforward that top is changeable only via CAS and monotonically increased. Releasing incremented bottom is not necessary because succeeding push will release the bottom. Please note there is no concern in concurrency because size() in pop_global () returns 0 for dirty_size()=N-1 and so there is no thief trying a steal. 180 return (sz == N - 1) ? 0 : sz; Bug: https://bugs.openjdk.java.net/browse/JDK-8208636 Webrev: http://cr.openjdk.java.net/~mhorie/8208636/webrev.00/ I ran JTREG and SPECjbb2015 to confirm there is no change in the test results and performance. Again this is just a refactoring and it's true current taskqueue works even if it is not aligned with the formal proofs partly. Best regards, -- Michihiro, IBM Research - Tokyo

2 2

[PATCH] Exploit Empty Regions in Young Gen to Enhance PS Full GC Performance
by Haoyu Li 18 Nov '19

18 Nov '19

Hi all， I have developed a patch to enhance the full GC performance of Parallel Scavenge on OpenJDK 11, may I have some reviews? The patch is described as follows and attached in this mail. *Problem* Parallel Scavenge(PS) implements a compacting algorithm to do the full GC, and we find that this algorithm leads in terrible GC thread utilization (like only 8% on Derby benchmark in SPECjvm2008 suite) since there are serious dependencies between heap regions, i.e., a region is available to receive live objects from its source regions only after it has been collected. The work stealing does not solve this problem, idle GC threads cannot steal anything because most regions are unavailable to collect. *Optimization* We propose *shadow region* to solve the above problem. The basic idea is to let GC threads collect unavailable regions in advance by copying their live data into newly allocated empty regions, i.e., shadow regions, to resolve the region dependencies. The contents of shadow regions will be copied back to the corresponding regions later. With our approach, GC threads can keep working at most of the time without suffering from any work stealing failure (except the work stealing failure happened in the end of a full GC). And we notice that the to space in young gen is always empty, so we exploit the empty regions in to space to play the role of shadow regoins (if the ScavengeBeforeFullGC option is on, regions in eden space may be used, too) and avoid allocating shadow regions from off heap memory. *Evaluation* We evaluate the full GC performance with our patch on DaCapo, SPECjvm2008, JOlden benchmark suits, and the results shows that shadow region optimization could improve full GC throughput by 2.1X on average, up to 3.2X. The patch and evaluation result are attached. Best Regrads, Haoyu Li, Institute of Parallel and Distributed Systems(IPADS), School of Software, Shanghai Jiao Tong University

5 31

RFR(M): 8220089: G1 wastes a significant amount of space in survivor regions
by sangheon.kim＠oracle.com 21 May '19

21 May '19

Hi all, Can I have some reviews for the patch that changes to reuse survivor region as mutator region? Currently survivor region is released at the end of gc and then added to the next collection set. This means there would be waste if the survivor region has enough free space. As the survivor regions are converted to eden at the start of gc, reusing the *last* survivor region as a mutator region would maximize the use of heap region. This patch suggests to reuse the survivor region as described if the remaining free space satisfies a minimum threshold(currently, larger than or equal to minimum TLAB). Some benchmarks(specjbb2015, specjbb2005, specjvm2008) that I ran didn't have noticeable improvement. However, I think this change would benefit more with NUMA implementation because the NUMA implementation manages survivor region per NUMA node. i.e. more wastes on multiple node system. CR: https://bugs.openjdk.java.net/browse/JDK-8220089 webrev: http://cr.openjdk.java.net/~sangheki/8220089/webrev.0 (based on JDK-8218668 webrev.1 which is under review now) Testing: hs-tier1 ~ 5 Thanks, Sangheon

3 6

RFR (XS): 8220388: Increase -inlinehint-threshold for Clang to avoid G1 pause time regression
by Man Cao 29 Apr '19

29 Apr '19

Hi all, I propose to increase the -inlinehint-threshold for Clang build: Webrev: https://cr.openjdk.java.net/~manc/8220388/webrev.00/ RFE: https://bugs.openjdk.java.net/browse/JDK-8220388 The background and performance impact is described in the RFE. In summary, increasing the -inlinehint-threshold could noticeably reduce GC pause time for G1 when building the JVM with Clang. I have a couple of questions though: (1) I agree that a better long-term solution might be to identify those performance-critical functions and mark them with "ALWAYSINLINE" instead of "inline". However, it might be difficult to identify all those functions, and similar issue might arise in collectors other than G1, or in other places in HotSpot. Currently ALWAYSINLINE is rarely used across HotSpot. I'm not sure if there is a convention to prefer "inline" over "ALWAYSINLINE". If we go down this path, we might end up changing many "inline" to "ALWAYSINLINE" all over the places, due to the cascading effect of inlining. (2) Since this change only affects Clang build, and Clang is only officially supported for Mac OS ( https://wiki.openjdk.java.net/display/Build/Supported+Build+Platforms) should I send this patch to broader mailing lists such as build-dev, hotspot-dev? Thanks, Man

4 7

RFR: JDK-8220813: update hotspot tier1_gc tests depending on GC to use @requires vm.gc.X
by Ao Qi 26 Apr '19

26 Apr '19

Hi, JBS: https://bugs.openjdk.java.net/browse/JDK-8220813 Webrev: http://cr.openjdk.java.net/~aoqi/8220813/webrev.00/ If a particular GC is not supported, some hotspot tier1_gc tests fails. The patch allows tests to be skipped on platforms where a GC is not supported. Tested: before (configure using --with-jvm-features): configure TOTAL PASS FAIL ERROR default 251 251 0 0 -cmsgc 243 222 21 0 -epsilongc 229 229 0 0 -g1gc,-jfr 181 157 24 0 -parallelgc 241 219 22 0 -shenandoahgc 206 206 0 0 -zgc 251 251 0 0 after (configure using --with-jvm-features): configure TOTAL PASS FAIL ERROR default 251 251 0 0 -cmsgc 222 222 0 0 -epsilongc 229 229 0 0 -g1gc,-jfr 157 157 0 0 -parallelgc 219 219 0 0 -shenandoahgc 206 206 0 0 -zgc 251 251 0 0 before: VM_OPTIONS TOTAL PASS FAIL ERROR default 251 251 0 0 -XX:+UseG1GC 129 129 0 0 -XX:+UseSerialGC 65 65 0 0 -XX:+UseParallelGC 69 69 0 0 -XX:+UseConcMarkSweepGC 67 67 0 0 after: VM_OPTIONS TOTAL PASS FAIL ERROR default 251 251 0 0 -XX:+UseG1GC 121 121 0 0 -XX:+UseSerialGC 51 51 0 0 -XX:+UseParallelGC 60 60 0 0 -XX:+UseConcMarkSweepGC 66 66 0 0 The difference in VM_OPTIONS tests mainly comes from two reasons: 1. This patch make this kind of change: @requires vm.gc=="null" & !vm.graal.enabled => @requires vm.gc.ConcMarkSweep & !vm.graal.enabled Take gc/arguments/TestAlignmentToUseLargePages.java#id1 for example. Before the patch, even if VM_OPTIONS=-XX:+UseConcMarkSweepGC is set, gc/arguments/TestAlignmentToUseLargePages.java#id1 will not be executed (but the test is intended to test -XX:+UseConcMarkSweepGC). After the patch, the test will be executed if VM_OPTIONS=-XX:+UseConcMarkSweepGC is set. 2. If a test use a particular GC in the code (not in the jtreg tag fields) and another GC is passed by -XX:UseXGC, the test will be skipped. For example, gc/startup_warnings/TestG1.java uses -XX:+UseG1GC in the main method. This test will be skipped, when VM_OPTIONS=-XX:+UseConcMarkSweepGC is used. Before this patch, it will be executed. I am doing more tests and checking test results above. Could someone give some advice on this change? Thanks, Ao Qi

4 30

RFR: 8219718: ZGC: Make nmethod entry barriers and nmethod::is_unloading use ZNMethodDataOops
by Erik Österlund 16 Apr '19

16 Apr '19

Hi, Accessing ZNMethodDataOops outside of concurrent code cache iteration used to be unsafe. It is now safe. Therefore, we should utilize this in the nmethod entry barriers and nmethod::is_unloading. Webrev: http://cr.openjdk.java.net/~eosterlund/8219718/webrev.00/ Bug: https://bugs.openjdk.java.net/browse/JDK-8219718 Thanks, /Erik

3 3

RFR (XL): 8218668: Clean up evacuation of optional collection set
by Thomas Schatzl 05 Apr '19

05 Apr '19

Hi all, please have a look at the following change that fixes a few (overall minor) problems with the Abortable Mixed GC implementation introduced in JDK12. Mainly, the collection of the optional collection set has been kind-of tacked on as a completely separate thing with its own collection set, its own iteration loop etc. This change integrates the optional evacuation much better imo into the existing program flow. Automatically it fixes a few issues with the existing implementation that were mostly already found and decided to be benign enough to move to another (this) stage (from the CR): - the optional collection set evacuation duplicates quite a bit of "first pass" evacuation code while conceptually very similar. This should be unified and cleaned up as much as possible. - with the optional collection set the code creates copies of parts of the candidate collection set, first popping it from it and then pushing back leftovers. This is not necessary, an index (cursor) within the candidate collection set is sufficient. - if there is an evacuation failure during evacuating an optional part, we try to fix up self-looped oops for the entire collection set which is not necessary. Obviously only regions part of the current collection pass need to be looked at. - the work distribution when evacuating an optional collection set is bad: every thread processes the same regions in the same sequence - calculation of total number of cards scanned and total scan RS time does not take optional evacuation into account - optional collection set log does not show code roots time (from the heap region's remembered sets) and termination time completely - optional collection also does not add "code root fixup time" - optional collection does not add to the "par time" (which is actually the time for the parallel evacuation), which means that the calculation for the "Other" time is wrong (result too high) impacting prediction (being too conservative) - the opt_index_in_cset of a heap region is not reset for all regions every time after GC. Since it is properly overwritten as needed this seems benign. Conceptually, nothing really changed: G1 young gc performs evacuation of the "initial collection set", i.e. young gen + an initial set of old gen regions (mixed gc only), and after that an incremental collection of optional regions as before (mixed gc only). However I tried to improve naming to work that out a bit better. Originally I wanted to make the code even more integrated, particularly merge the two gangtasks to evacuate the initial collection set and the optional collection set; however I could not find a good way due to the necessary MarkScopes. Previous recent changes were basically preparations for this change that were split out, but I strongly recommend looking at "JDK-8219100: Improve do_collection_pause_at_safepoint" also out for review right now before this change as it moves around quite a bit of code, and you might feel a bit lost with the baseline code otherwise. CR: https://bugs.openjdk.java.net/browse/JDK-8218668 Webrev: http://cr.openjdk.java.net/~tschatzl/8218668/webrev/ Testing: hs-tier1-8; no performance differences Thanks, Thomas

3 7

RFR: JDK-8220671: Initialization race for non-JavaThread PtrQueues
by Roman Kennke 01 Apr '19

01 Apr '19

In Shenandoah testing we discovered an initialization race: A non-Java GC thread (we have observed it on the StringDedupThread) may be initialized concurrently while Java and GC are already up an running, but not (yet) participate in safepointing. BS::on_thread_attach() usually does propagate global GC state to thread-local GC state, in this case the SATB active flag. When doing this concurrently, while not participating in safepointing, this may propagate the wrong state, and subsequently lead to heap corruption (e.g. because we missed some SATB updates). This is related to JDK-8219613 because before that change, non-Java-threads would simply use a shared SATB queue instead. The bug appeared in Shenandoah testing, but I don't see why it wouldn't affect G1 too. It's probably not run with aggressive enough tests to make it happen. (We run Shenandoah in aggressive mode, which starts continuous GCing right at the start.) Bug: https://bugs.openjdk.java.net/browse/JDK-8220671 Webrev: http://cr.openjdk.java.net/~rkennke/JDK-8220671/webrev.03/ The problem seems specific to the StringDedupThread (for now), and so is the solution: in the StringDedupThread's pre_run() and post_run(), take the STSJoiner. This ensures that the thread doesn't accidentally crosses safepoints while initializing or exiting, and thus loosing SATB updates. I tried a couple of other approaches like: http://cr.openjdk.java.net/~rkennke/JDK-8220671/webrev.02/ But we also need to protect the addition/removal of the thread to the global NonJavaThread list. Testing: Running the offending test (TestStringDedupStress.java) 20x in a row. It used to fail ~1 of 5 runs before. Now it all passes. Also, hotspot_gc_shenandoah passes. tier1 is fine too. Will push it through jdk-submit next. Can I please get reviews? Thanks, Roman

3 52

RFR(xs): 8221517: G1: Reserved page size for heap can be wrong
by sangheon.kim＠oracle.com 01 Apr '19

01 Apr '19

Hi all, Can I have some reviews for the patch which fixes wrong G1 heap page size issue? The page size which is used to decide G1 heap mapper comes from preferred page size or heap alignment. However, if it came from heap alignment, we have to limit to actual large page size if it is larger than large page size. I added a new jtreg test to check this problem. It relies on the error message when printed if reserving memory with large page fails. Unfortunately, Solaris has a bug that the error message is printed even in success. To address this problem I filed 'JDK-8221526: Solaris: large page reservation failure message is printed even in success.' and the test is excluded on Solaris. CR: https://bugs.openjdk.java.net/browse/JDK-8221517 webrev: http://cr.openjdk.java.net/~sangheki/8221517/webrev.0 Testing: hs-tier1 ~ 5 Thanks, Sangheon

3 6

RFR: 8221648: Remove CollectedHeap::is_in_closed_subset()
by Per Liden 01 Apr '19

01 Apr '19

I propose that we remove CollectedHeap::is_in_closed_subset() and its unused sibling CollectedHeap::is_in_closed_subset_or_null(). We can replace all calls to is_in_closed_subset(), most of which is in verification code, with calls to is_in(). For Serial and ZGC, is_in_closed_subset() is already equivalent to is_in(). For G1, is_in_closed_subset() is very close to equivalent to is_in(). For CMS, Parallel, Epsilon, and Shenandoah, is_in_closed_subset() is equivalent to is_in_reserved(). However, their is_in() is arguably not a lot more expensive, at least not to the degree that is motivates keeping the is_in_closed_subset() API around. G1 is the only GC that calls is_in_closed_subset() outside of verification code (in RegisterHumongousWithInCSetFastTestClosure). Bug: https://bugs.openjdk.java.net/browse/JDK-8221648 Webrev: http://cr.openjdk.java.net/~pliden/8221648/webrev.0 /Per

3 4