November 2014 - hotspot-gc-dev

RFR (L): 8065972: Remove support for ParNew+SerialOld and DefNew+CMS
by Bengt Rutisson 27 Nov '14

27 Nov '14

Hi everyone, Can I have a couple of reviews for this change to remove the support for ParNew+SerialOld and DefNew+CMS? This work is part of JEP 214 (http://openjdk.java.net/jeps/214) The main change is in the hotspot repo: http://cr.openjdk.java.net/~brutisso/8065972/hotspot-webrev.00/ I also removed the ParNew runs from the jprt configuration: http://cr.openjdk.java.net/~brutisso/8065972/root-webrev.00/ JBS bug: https://bugs.openjdk.java.net/browse/JDK-8065972 Some notes about the change: - UseParNewGC is now a redundant flag. It can only be used with CMS, so it will always have the same value as UseConcMarkSweepGC. Since UseParNewGC was not one of the flags that were deprecated in JDK 8, we decided to leave the flag in for JDK 9 but print a warning message that it is deprecated. We plan on removing UseParNewGC in JDK 10. Here's how the VM will behave with different values of UseParNewGC after my change: $ java -XX:+UseParNewGC -version It is not possible to combine the ParNew young collector with the Serial old collector. Error: Could not create the Java Virtual Machine. Error: A fatal exception has occurred. Program will exit. $ java -XX:-UseParNewGC -version Java HotSpot(TM) 64-Bit Server VM warning: The UseParNewGC flag is deprecated and will likely be removed in a future release java version "1.9.0-internal-debug" Java(TM) SE Runtime Environment (build 1.9.0-internal-debug-brutisso_2014_11_05_09_40-b00) Java HotSpot(TM) 64-Bit Server VM (build 1.9.0-internal-debug-brutisso_2014_11_05_09_40-b00, mixed mode) $ java -XX:+UseParNewGC -XX:+UseConcMarkSweepGC -version Java HotSpot(TM) 64-Bit Server VM warning: The UseParNewGC flag is deprecated and will likely be removed in a future release java version "1.9.0-internal-debug" Java(TM) SE Runtime Environment (build 1.9.0-internal-debug-brutisso_2014_11_05_09_40-b00) Java HotSpot(TM) 64-Bit Server VM (build 1.9.0-internal-debug-brutisso_2014_11_05_09_40-b00, mixed mode) $ java -XX:-UseParNewGC -XX:+UseConcMarkSweepGC -version It is not possible to combine the DefNew young collector with the CMS collector. Error: Could not create the Java Virtual Machine. Error: A fatal exception has occurred. Program will exit. - The NEW_C_HEAP_ARRAY and "new" calls in initialize_generations() methods will exit if they fail. No need to check for null after those calls. This simplified the two implementations significantly. - At a few places I've changed from "DefNewGeneration* dng = (DefNewGeneration*)_young_gen;" to "DefNewGeneration* dng = _young_gen->as_DefNewGeneration();". The as_DefNewGeneration() method does the asserts that were often placed before the original casting code. - In concurrentMarkSweepGeneration.cpp I left the casting to DefNewGeneration even though we could now use the more specific ParNewGeneration. I prefer to do such an update later (if at all) since that might come with its own complications regarding virtual calls etc. - The two methods must_be_youngest() and must_be_oldest() were unused. Removing them was not strictly needed for this change, but seemed like a nice thing to do anyway and I don't think it clutters the webrev much. - UseParNewGC is now always true when UseConcMarkSweepGC is true. And it is always false when UseConcMarkSweepGC is false. If statements that used to test against both flags have been simplified to only test against UseConcMarkSweepGC. This means that UseParNewGC is now only used in arguments.cpp - Similarly JTreg tests that used UseParNewGC have been changed to use UseConcMarkSweepGC instead. This should make it easy to remove in JDK 10. - The change to make/jprt.properties only affects the internal testing system we have. It will no longer test the ParNew+SerialOld combination (which is no longer supported). Note that DefNew+CMS was not tested at all by JPRT. Thanks, Bengt

3 9

RFR 8065579: WB method to start G1 concurrent mark cycle should be introduced
by Leonid Mesnik 27 Nov '14

27 Nov '14

Hi Here is the fix which introduce WB method which stars concurrent mark cycle explicitly. It is needed to make CM cycle related tests more stable. Changeset also includes test and very small fix of WB method g1InConcurrentMark . Fix pass JPRT with hotspot testset. JIRA: https://bugs.openjdk.java.net/browse/JDK-8065579 Webrev: http://cr.openjdk.java.net/~sjohanss/lmesnik/8065579/hotspot.00/

3 8

RFR(s): 8055239: assert(_thread == Thread::current()->osthread()) failed: The PromotionFailedInfo should be thread local.
by Sangheon Kim 27 Nov '14

27 Nov '14

Hi all, Please review this change to reset PromotionFailedInfo before use: https://bugs.openjdk.java.net/browse/JDK-8055239 Webrev: http://cr.openjdk.java.net/~sangheki/8055239/webrev.00/ Summary: This issue happens only with ParallelRefProcEnabled. And this assert happens as the current thread (when second promotion failure happened) is different from saved thread (when first promotion failure happened). From ParNewGeneration::collect(), there are two mt processing, for live object processing (by ParNewGenTask) and reference processing (by ParNewRefEnqueueTaskProxy). When first promotion failure happened from ParNewGenTask, thread pointer will be stored by PromotionFailedInfo::register_copy_failure(). And at second failure, assert happens as thread pointer is not guaranteed to be same. As the thread counts increase, its frequency become higher. Fix: Added ParScanThreadStateSet::trace_promotion_failed() to trace and reset promotion failure information instance. Test: - jprt Thanks, Sangheon

5 13

RFR: 8065227: Report allocation context stats at end of cleanup
by Stefan Johansson 26 Nov '14

26 Nov '14

Hi, Please review this forward port to fix this issue in jdk9: https://bugs.openjdk.java.net/browse/JDK-8065227 Webrev: jdk9: http://cr.openjdk.java.net/~sjohanss/8065227/jdk9-hotspot.00/ jdk8u40: http://cr.openjdk.java.net/~sjohanss/8065227/hotspot.00/ Old review thread: http://mail.openjdk.java.net/pipermail/hotspot-gc-dev/2014-November/011348.… Summary: The changeset didn't apply clean due to surrounding changes in concurrentMark.cpp, but the idea for the fix remains the same. Move the allocation_context_stats-update to the clean up phase instead of remark. Testing: * Built through JPRT Thanks, Stefan

3 2

RFR [1/2] 8065358: JDK-8065218 Move CMS-specific fields from Space to CompactibleFreeListSpace
by Mikael Gerdin 26 Nov '14

26 Nov '14

Hi all, This is the first patch of two to fix JDK-8065358. In this change I suggest that we move a field used only by CMS to CompactibleFreeListSpace and introduce a virtual getter in the Space base class. I estimate that the performance impact of the virtual call will be small since the getter path already does several virtual calls. Moving the field will allow me to add another field to G1's G1OffsetTableContigSpace in patch [2/2] without introducing a footprint regression. Webrev: http://cr.openjdk.java.net/~mgerdin/8065218/webrev.0/ Bug: https://bugs.openjdk.java.net/browse/JDK-8065218 Parent bug: https://bugs.openjdk.java.net/browse/JDK-8065358 Testing: JPRT Thanks /Mikael

4 5

RFR [2/2] 8065258: JDK-8065358 Refactor G1s usage of save_marks and reduce related races
by Mikael Gerdin 26 Nov '14

26 Nov '14

Hi all, This is the second patch of two to fix JDK-8065358 In this change I suggest that we introduce a new field _scan_top to G1OffsetTableContigSpace to keep track of the maximum address where a GC worker thread is allowed to scan objects in old regions which have been used as allocation regions during the current evacuation cycle. More background (from the bug): G1 currently uses the _saved_mark_word field in Space to detemine a point in a region above which a current GC cannot scan objects. This is needed to prevent card scanning threads from scanning in memory regions where other gc workers are concurrently allocating objects. This re-use of the _saved_mark_word field causes confusion to readers of the code since it's not used for the same purpose as the other Space classes. The setting and reading of this field, and the per region gc time stamp which accompanies it is also unnecessarily racy. Previously the _saved_mark_word field was set to the value of the _top pointer when a region was selected as a GC allocation region and the time stamp was set to indicate that the saved mark field should be used as a maximum address. This code had some problems with races in JDK-8058209 and could be redesigned in a less racy way. Suggested fix is to introduce a new field in HeapRegions to keep track of the maximum address where card scanning is allowed and set that field at the point of retaining an OldGCAllocRegion instead of when allocations are being started. That way we get rid of the store ordering requirement in the timestamp record path. There is still a race between the per region time stamps and the per region top pointer, where we must ensure that the time stamp store must be visible before any subsequent top stores. This store ordering is enforced by the fact that all stores to top are either ordered with #storestore (the initial allocation) or guarded by a Mutex. To ensure that the reader path sees a consistent view it must be exectued with the proper load ordering, where we must read top before the time stamp in order to ensure that we don't see a top value which has been updated by a concurrent gc worker if we see a time stamp from a previous gc cycle. Webrev: http://cr.openjdk.java.net/~mgerdin/8065358/webrev.0/ Bug: https://bugs.openjdk.java.net/browse/JDK-8065358 Testing: JPRT, Kitchensink (4hr+) Thanks /Mikael

3 8

another GC log funny
by Kirk Pepperdine 26 Nov '14

26 Nov '14

Hi all, I’ve seen this a few times in the past but just recently it seems to be showing up more frequently…. 2014-11-24T17:18:02.364-0100: 0,602: [GC2014-11-24T17:18:02.364-0100: 0,602: [ParNew Desired survivor size 2621440 bytes, new threshold 1 (max 1) - age 1: 5062272 bytes, 5062272 total - age 2: 16 bytes, 5062288 total This is a 1.7.0_51. I’ve also got a log file with new threshold 15 (max 64). This, I guess, is a less serious problem where the user hasn’t been warned that you might ask for 64 but you’re getting 15. Kind regards, Kirk Pepperdine

1 0

RFR: 8065865: gc/TestSoftReferencesBehaviorOnOOME.java: Error. Can't find source file: TestSoftReference.java
by Evgeniya Stepanova 26 Nov '14

26 Nov '14

Hi, Could you please review very small fix for https://bugs.openjdk.java.net/browse/JDK-8065865. Problem: while test renaming one string was not updated so the test fails to build. Solution: String has been updated bug: https://bugs.openjdk.java.net/browse/JDK-8065865 webrev: http://cr.openjdk.java.net/~eistepan/8065865/webrev.00/ Thanks, Evgeniya Stepanova -- /Evgeniya Stepanova/

2 1

RFR: 8059066: CardTableModRefBS might commit the same page twice
by Erik Helin 26 Nov '14

26 Nov '14

Hi all, this patch fixes an issue with overlapping commits in cardTableModRefBS. If the JVM is started with the options -Xmx8m -XX:-UseLargePages -version on a system with a smallest page size of 8kB, then cardTableModRefBS will commit the same page twice. Given how os::commit_memory works, the issue is benign: mmap will simply overwrite the page with zeroes again, and since this is during initialization, the card table is only filled with zeroes. However, the issue becomes annoying when running with -XX:NativeMemoryTracking=detail, since NMT will report the overlapping commit. The patch is a little tricky, since the surrounding code is a bit intertwined. I will try to explain the patch step-by-step, it is composed of two parts: 1. Update which committed regions we check when determining if `new_end_aligned` interferes with another region. 2. Update the criteria for checking if `new_end_aligned` interferes with another region. The patch for the first part is: --- a/src/share/vm/memory/cardTableModRefBS.cpp +++ b/src/share/vm/memory/cardTableModRefBS.cpp @@ -277,5 +277,5 @@ void CardTableModRefBS::resize_covered_r // space of another region. int ri = 0; - for (ri = 0; ri < _cur_covered_regions; ri++) { + for (ri = ind + 1; ri < _cur_covered_regions; ri++) { if (ri != ind) { if (_committed[ri].contains(new_end_aligned)) { We only need to check if `new_end_aligned` clashes with regions following `ind`, since the code a few lines above handles regions before `ind`: 265 HeapWord* const max_prev_end = largest_prev_committed_end(ind); 266 if (max_prev_end > cur_committed.end()) { 267 cur_committed.set_end(max_prev_end); 268 } We only commit more memory if `new_end_for_commit > cur_committed.end()` where `new_end_for_commit` is equal to new_end_for_aligned`. Hence, the case where a region before `ind` already have committed memory up to (or beyond) `new_end_aligned` is taken care of. Therefore, we only need to check regions after `ind`, which is the patch above. This is also confirmed by an existing comment and assert in the loop body: 285 // Any region containing the new end 286 // should start at or beyond the region found (ind) 287 // for the new end (committed regions are not expected to 288 // be proper subsets of other committed regions). 289 assert(_committed[ri].start() >= _committed[ind].start(), 290 "New end of committed region is inconsistent"); Since the committed regions should be sorted according their start address (see CardTableModRefBS::find_covering_region_by_base), the assert can only fail for regions where `ri < ind`. The second part of the patch then updates the criteria for determining if `new_end_aligned` interferes with a region after `ind`: --- a/src/share/vm/memory/cardTableModRefBS.cpp +++ b/src/share/vm/memory/cardTableModRefBS.cpp @@ -279,5 +279,6 @@ void CardTableModRefBS::resize_covered_r for (ri = ind + 1; ri < _cur_covered_regions; ri++) { if (ri != ind) { - if (_committed[ri].contains(new_end_aligned)) { + if (new_end_aligned > _committed[ri].start() && + new_end_aligned <= _committed[ri].end()) { // The prior check included in the assert // (new_end_aligned >= _committed[ri].start()) `_committed[ri].contains(new_end_aligned)` only checks: bool contains(const void* addr) const { return addr >= (void*)_start && addr < (void*)end(); } and therefore misses the case when `new_end_aligned` completely covers the region _committed[ri], that is, `new_end_aligned == _committed[ri].end()`. This is fixed by the above patch. Given the above two patches, the `if (ri != ind`)` check can be removed from the loop body and the loop body can therefore remove two spaces of indentation. I also removed an outdated comment. Webrev: http://cr.openjdk.java.net/~ehelin/8059066/webrev.00/ Bug: https://bugs.openjdk.java.net/browse/JDK-8059066 Testing: - JPRT - Ad-hoc (Window 32/64, Linux 32/64, OS X 64, Solaris x86-64/SPARC): - Dacapo, Kitchensink, Weblogic+medrec, runThese - nsk.regression, vm.gc, vm.oom, vm.parallel_class_loading, vm.regression, vm.runtime - nashorn regression tests - JTReg: - HotSpot - jdk_core, jdk_svc - Running -Xmx8m -XX:-UseLargePages -version on a machine with 8kB pages Thanks, Erik

4 12

RFR: 8065749: [TESTBUG]: gc/arguments/TestG1HeapRegionSize.java fails at nightly
by Evgeniya Stepanova 25 Nov '14

25 Nov '14

Hi, Could you please review changes for 8065749 ? Test TestG1HeapRegionSize.java is G1-specific and could not work without -XX:+UseG1GC passed directly. Option added to the run. bug: https://bugs.openjdk.java.net/browse/JDK-8065749 fix: http://cr.openjdk.java.net/~eistepan/8065749/webrev.00/ Thanks, Evgeniya Stepanova -- /Evgeniya Stepanova/

4 7