RFR [L][4/7]: 8180415: Rebuild remembered sets during the concurrent cycle (original) (raw)
Thomas Schatzl thomas.schatzl at oracle.com
Thu Mar 22 10:37:29 UTC 2018
- Previous message (by thread): RFR [L][4/7]: 8180415: Rebuild remembered sets during the concurrent cycle
- Next message (by thread): RFR [L][4/7]: 8180415: Rebuild remembered sets during the concurrent cycle
- Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]
Hi Sangheon,
thanks for your thorough review.
On Wed, 2018-03-21 at 21:47 -0700, sangheon.kim wrote:
Hi Thomas,
On 03/13/2018 07:54 AM, Thomas Schatzl wrote: > Hi all, > [...] > > New webrevs: > http://cr.openjdk.java.net/~tschatzl/8180415/webrev.2to3 (diff) > http://cr.openjdk.java.net/~tschatzl/8180415/webrev.3 (full) webrev.3 looks good to me. I have some minor comments. =========================================================== src/hotspot/share/gc/g1/g1AllocRegion.cpp - Need to revert copyright update as there's no change. =========================================================== src/hotspot/share/gc/g1/g1ConcurrentMark.cpp 1045 class G1UpdateRemSetTrackingAfterRebuild : public HeapRegionClosure { - Couldn't have a constructor to pass G1CollectedHeap same as G1UpdateRemSetTrackingBeforeRebuild and use it from doheapregion()?
Fixed. I will do a cleanup pass to make every method use _g1h in g1ConcurrentMark.cpp soon.
=========================================================== src/hotspot/share/gc/g1/g1ConcurrentMark.hpp 143 static const sizet RegionMarkStatsCacheSize = 1024; 643 static const uint RegionMarkStatsCacheSize = 1024; - There are 2 RegionMarkStatsCacheSize and only the latter one seems to be used. Or am I missing something?
623 // Rebuilds the remembered sets for chosen regions concurrently and in parallel to the application. - Please ignore if I'm wrong. :) I guess you meant, processes with MT and concurrently with the application. Something like '... chosen regions in parallel and concurrently with the application.'?
Fixed, thanks!
=========================================================== src/hotspot/share/gc/g1/g1FullCollector.cpp - Copyright year update =========================================================== /src/hotspot/share/gc/g1/g1OopClosures.inline.hpp 145 HeapRegion* to = g1->heapregioncontaining(obj); - Getting HeapRegionRemSet and using it seems better for 147,148,149 lines. =========================================================== src/hotspot/share/gc/g1/g1RemSet.cpp 764 logdebug(gc,remset)("Humongous obj region %u marked %d start " PTRFORMAT " region start " PTRFORMAT " tams " PTRFORMAT " tars " PTRFORMAT, 842 "bot " PTRFORMAT " ntams " PTRFORMAT " TARS " PTRFORMAT, - Uppercase for tams and tars used in log message? 888 uint numworkers = workers->activeworkers(); - Need one more space before 'uint'. =========================================================== src/hotspot/share/gc/g1/heapRegionRemSet.hpp 135 bool addreference(OopOrNarrowOopStar from, uint tid); - It would be helpful to mention when it returns true/false. I guess returns true when we add a reference. - I couldn't see returning true in addreference(). Am I missing something? I know we are not using the return value so no problem so far. Or heapRegionRemSet.cpp:line 432 should return true? 432 return false; (heapRegionRemSet.cpp)
The return value is not used (any more), I removed the return value.
=========================================================== src/hotspot/share/logging/logPrefix.hpp - Copyright year update
=========================================================== test/hotspot/jtreg/gc/concurrentphasecontrol/TestConcurrentPhaseCon trolG1Basics.java 2 * Copyright (c) 2018, Oracle and/or its affiliates. All rights reserved. - Should be '2017, 2018' =========================================================== src/hotspot/share/gc/g1/g1RemSetTrackingPolicy.cpp 36 bool G1RemSetTrackingPolicy::needsscanforrebuild(HeapRegion* r) const{ - Need a space between const and { 85 if ((totallivebytes > 0) && 86 (isinterestinghumongousregion(r) || CollectionSetChooser::regionoccupancylowenoughforevac(totallive bytes)) && 87 !r->remset()->istracked()) { - Maybe I'm wrong but Line 86 and 87 seem to need more spaces to align with line 85. =========================================================== src/hotspot/share/gc/g1/g1RemSetTrackingPolicy.hpp 52 // pause. Called at safepoint in the remark pause. - cleanup pause
All done.
http://cr.openjdk.java.net/~tschatzl/8180415/webrev.3_to_4 (diff) http://cr.openjdk.java.net/~tschatzl/8180415/webrev.4 (full)
In addition to that I added the following minor changes:
renamed the "CLEAR_REMEMBERED_SET" phase in the concurrent gc phase manager to "REBUILD_REMEMBERED_Set" as this matches the code better.
several fixes to later re-enable the TestVerifyGCType (JDK-8193067: gc/g1/TestVerifyGCType.java still unstable) later. That test parses the log messages done during verification, and expects them to have a "During GC" prefix.
there is one minor I think pre-existing issue fixed, in g1Policy.cpp::decide_on_conc_mark_initiation(): it is possible that a user initiated concurrent mark request can happen right after the Cleanup pause before the "last gc before mixed gc". During that time there is a collection set, and that one gets messed up in the following marking (the evacuation efficiency of the regions may change). This triggers some (benign) assert in the collection set chooser. We need to clear any existing collection set before initiating concurrent mark.
Thanks, Thomas
- Previous message (by thread): RFR [L][4/7]: 8180415: Rebuild remembered sets during the concurrent cycle
- Next message (by thread): RFR [L][4/7]: 8180415: Rebuild remembered sets during the concurrent cycle
- Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]