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

[Thomas Schatzl]

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