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

[Thomas Schatzl]

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.

Thanks,
  Thomas