RFR [S][5/7]: 8197573: Remove concurrent cleanup and secondary free list handling (original) (raw)

sangheon.kim sangheon.kim at oracle.com
Mon Mar 26 22:38:16 UTC 2018

Hi Thomas,

On 03/08/2018 05:31 AM, Thomas Schatzl wrote:

Hi,

On Thu, 2018-03-08 at 13:28 +0100, Stefan Johansson wrote: On 2018-03-08 12:59, Thomas Schatzl wrote:

Hi,

On Mon, 2018-03-05 at 16:16 +0100, Thomas Schatzl wrote: Hi all,

can I have reviews for this change that removes the concurrent cleanup phase and the secondary free list handling entirely. It moves leftover work into the Cleanup pause. In the previous change the remembered set scrubbing, which took in cases where it mattered like 95%+ of time, has been removed because it is not required any more. The reasons for removal are: - due to JDK-8180415 freeing remembered sets (as opposed to calculating occupancy) is really fast, so this phase has never been very long for a long time. - it adds a lock in the LAB allocation path It also allows us to remove the "mixedgcpending" flag in the CollectorState, as there is no further time to wait until G1 can schedule the "last young gc". CR: https://bugs.openjdk.java.net/browse/JDK-8197573 Webrev: http://cr.openjdk.java.net/~tschatzl/8197573/webrev/index.html Testing: hs-tier 1-3, .... (I consider this an "S" changeset because it is mostly removal of code). Some ripple-through after latest changes to JDK-8180415 requires an update of the webrevs: http://cr.openjdk.java.net/~tschatzl/8197573/webrev.0to1 (diff) http://cr.openjdk.java.net/~tschatzl/8197573/webrev.1 (full) Looks lovely, just some minor things: src/hotspot/share/gc/g1/concurrentMarkThread.cpp 436 loginfo(gc, marking)("Concurrent Mark Abort"); This line was removed, and it looks like will never log a concurrent mark abort now. Looks like this could be added to G1ConcurrentMark::concurrentcycleend() where we trace a potential cm failure. Nice catch! --- src/hotspot/share/gc/g1/g1FullGCPrepareTask.cpp 106 g1h->freehumongousregion(hr, &dummyfreelist); Should be safe to call freehumongousregion with locked = true here, since this will be the only thread working on that region. Fixed. 115 g1h->cardtable()->clear(MemRegion(hr->bottom(), hr->end())); Pre-existing, but since you have added the clearcardtable() function we could use it here as well, seems to be the only place where the old "scheme" is used. Done. --- I also found some more opportunity to do minor cleanup after Stefan's last remark about log messages... Nice, and you also restructured the code exactly like I planned to comment :) :) New webrevs: http://cr.openjdk.java.net/~tschatzl/8197573/webrev.1to2/ (diff) http://cr.openjdk.java.net/~tschatzl/8197573/webrev.2/ (full) webrev.2 looks good. I only have minor nits.

=============================== src/hotspot/share/gc/g1/concurrentMarkThread.cpp  362       if (!cm()->has_aborted()) { ...  364       }  365  366       if (!cm()->has_aborted()) {

=============================== src/hotspot/share/gc/g1/g1CollectedHeap.cpp (from original file) 1030   // following two methods.

=============================== src/hotspot/share/gc/g1/heapRegionSet.cpp

Thanks, Sangheon

Thanks, Thomas

More information about the hotspot-gc-dev mailing list