RFR (S): 8209700: Remove HeapRegionSetBase::RegionSetKind for a more flexible approach

[Hohensee, Paul]

I'd probably keep the HeapRegionSetChecker subclass declarations/definitions in heapRegionSet.* rather than moving them to g1CollectedHeap.cpp and heapRegionManager.cpp. heapRegionSet.* is where the base class is defined, and future refactoring might require moving them out of g1CollectedHeap.cpp and heapRegionManager.cpp. If they stay in heapRegionSet.*, that'd be unnecessary. But, it's your call.

In heapRegionManager.cpp:

#include "heapRegionManager.hpp"
=>
#include "gc/g1/heapRegionManager.hpp"

This #include isn't strictly necessary given that there already is

#include "gc/g1/heapRegionManager.inline.hpp"

In heapRegionSet.hpp:

Since you've replaced _mt_safety_checker with _verifier, you might want to change the corresponding formal parameter names from mt_safety_checker to verifier, e.g., in the HeapRegionSetBase, HeapRegionSet, and FreeRegionList constructors.

Paul

On 8/21/18, 4:53 AM, "hotspot-gc-dev on behalf of Thomas Schatzl" <hotspot-gc-dev-bounces at openjdk.java.net on behalf of thomas.schatzl at oracle.com> wrote:

    Hi all,
    
      in the review for JDK-8208498 there has been a discussion about using
    enums to indicate which types of regions can be added to a given
    HeapRegionSet; looking at the code, ultimately this enum is only used
    for verification, i.e. if the HeapRegionSet contains the right kind of
    regions or not.
    
    Instead of adding random enum values every time you need a new
    HeapRegionSet, I refactored the code so that the existing
    HRSMtSafeChecker passed to HeapRegionSet instances also gets methods
    for other verification, and implemented them.
    
    I also moved the various HRSMtSafeChecker instances to their point of
    use in cpp files to imho reduce the clutter in hpp files a bit (i.e.
    hide them completely in cpp files).
    
    CR:
    https://bugs.openjdk.java.net/browse/JDK-8209700
    Webrev:
    http://cr.openjdk.java.net/~tschatzl/8209700/webrev/
    Testing:
    hs-tier1-3
    
    This change is based on JDK-8208498 that is also out for review.
    
    Thanks,
      Thomas