RFR [L][4/7]: 8180415: Rebuild remembered sets during the concurrent cycle

[sangheon.kim]

Hi Thomas,

On 03/22/2018 03:37 AM, Thomas Schatzl wrote:
> 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.2_to_3 (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 do_heap_region()?
> 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 size_t 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->heap_region_containing(obj);
>> - Getting HeapRegionRemSet and using it seems better for 147,148,149
>> lines.
>>
>> ===========================================================
>> src/hotspot/share/gc/g1/g1RemSet.cpp
>>    764         log_debug(gc,remset)("Humongous obj region %u marked
>> %d
>> start " PTR_FORMAT " region start " PTR_FORMAT " tams " PTR_FORMAT "
>> tars " PTR_FORMAT,
>>    842                                       "bot " PTR_FORMAT " ntams
>> "
>> PTR_FORMAT " TARS " PTR_FORMAT,
>> - Uppercase for tams and tars used in log message?
>>
>>    888  uint num_workers = workers->active_workers();
>> - Need one more space before 'uint'.
>>
>> ===========================================================
>> src/hotspot/share/gc/g1/heapRegionRemSet.hpp
>>    135   bool add_reference(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 add_reference(). 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/concurrent_phase_control/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::needs_scan_for_rebuild(HeapRegion*
>> r)
>> const{
>> - Need a space between const and {
>>
>>     85     if ((total_live_bytes > 0) &&
>>     86       (is_interesting_humongous_region(r) ||
>> CollectionSetChooser::region_occupancy_low_enough_for_evac(total_live
>> _bytes))
>> &&
>>     87       !r->rem_set()->is_tracked()) {
>> - 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.
Webrev.4 looks good to me.
Thank you for fixing all!

Sangheon


>
> Thanks,
>    Thomas
>
>