RFR (M): 8208498: Put archive regions into a first-class HeapRegionSet

[Hohensee, Paul]

I'm fine with going along with whatever the current coding standard is. old_set_bulk_remove() and friends aren't exactly setters, but I suppose they're close enough. If you've defined an accessor, though, I recommend using it instead of the directly using the field, because having and using two ways of doing the same thing is duplicated code, which is a major source of bugs when one gets changed and the other doesn't. And, if you change the implementation, you'll need to find two sets of uses instead of one. If you add an accessor, you'd change all direct field uses to use the accessor instead.

I believe the original reason const ints were preferred was that some C++ compilers couldn't handle declaring enums within a class definition. That's no longer true, so I'd use an enum to encapsulate the kind values. 

Going further, you could use HeapRegionType directly, but it would require a few other changes. Since it only has a single scalar field, the compiler will treat it as a scalar, so no extra overhead involved relative to using an int or enum. If _region_kind were a HeapRegionType, then you'd need to make HeapRegionType::Tag public so you could pass it to the HeapRegionSetBase constructor, you'd have e.g.,

HeapRegionSetBase(HeapRegionSet::HumongousTag, new HumongousRegionSetMtSafeChecker());

You could eliminate the name string argument if HeapRegionType had a method that returned the string you want.

Anyway, that's complex enough to be its own RFE, so I'd be happy if you just put the For* constants in an enum. :)

Paul

On 8/13/18, 2:07 AM, "Thomas Schatzl" <thomas.schatzl at oracle.com> wrote:

    Hi,
    
    On Fri, 2018-08-10 at 16:21 +0000, Hohensee, Paul wrote:
    > Hi Thomas,
    > 
    > LGTM except for the following.
    > 
    > Analogous to old_set_add() and old_set_remove(), you might add
    > old_set_bulk_remove(), humongous_set_add(),
    > humongous_set_bulk_remove(), archive_set_add(), and
    > archive_set_remove() methods and use them instead of directly
    > accessing _old_set, _humongous_set and _archive_set.
    
      I considered this, but I prefer to not introduce trivial getters that
    are used exactly once, and all of them within the owner of each
    particular member (i.e. G1CollectedHeap).
    
    We in the past have spent time (and removed lots of such getters e.g.
    in the generational collector's code) to reduce (source) code size.
    
    From a pure OO perspective having getters is obviously nicer, but as
    they add virtually nothing (not even e.g. additional parameter
    checking), so it seems better to reduce the G1CollectedHeap interface
    for these particular instances.
    
    As soon as there are external users of these, we can add them. Their
    usages can be trivially found by any basic IDE.
    
    > g1CollectedHeap.cpp:
    >        // We ignore humongous and archive regions, we're not tearing
    > down the
    >        // humongous regions set.
    > => 
    >        // We ignore humongous and archive regions, we're not tearing
    > down the
    >        // humongous and archive region sets.
    > 
    
    Fixed.
    
    > g1CollectedHeap.hpp:
    >     return (_old_set.length() + _archive_set.length() +
    > _humongous_set.length()) * HeapRegion::GrainBytes;
    > =>
    >     return (old_regions_count() + archive_regions_count() +
    > humongous_regions_count()) * HeapRegion::GrainBytes
    
    See above.
    
    > 
    > heapRegionSet.hpp:
    > Can you use an enum instead of a set of "const int" for the
    > ForxxxRegion constants? Or, you could just use HeapRegionType.
    
    Do enums have an advantage here?
    
    The current style guide [1] indicates some discouragement to use them,
    i.e. " Inside class definitions, integer constants can be defined with
    "static const".  (Historically, enums have been also been used.)" for
    various reasons (no defined type basically).
    With C++11 or whatever this might change again.
    
    I am open for better names of the constants, but they can be changed
    later as needed again too.
    
    HeapRegionType does not expose any type to use, and "Tag" would be too
    confusing here.
    
    Thanks,
      Thomas
    
    [1] https://wiki.openjdk.java.net/display/HotSpot/StyleGuide