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

[Thomas Schatzl]

Hi,

On Tue, 2018-08-14 at 17:24 +0000, Hohensee, Paul wrote:
> 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 fixed the use of the count() methods, I agree about them. I can't see
where e.g. bulk_remove() is used twice for the humongous regions, i.e.
in this change I am not adding new single-use methods.

I am aware that there will be more to add for the new MXBeans.

> 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. 

I think the current reason is that the type of an enum value is
basically whatever the compiler thinks it can get away with it, causing
lots of casting needed if used "properly" in other code (see e.g. in
gc/shared/weakProcessorPhases.hpp)

Since in this case there is no further use than for passing them to
that single parameter, and I am not really hung up on that, I changed
it to an enum. Let's see if it is okay with others.

> 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.

HeapRegionType and its Tag enum has been designed to allow for fast
checks for subsets of region types during GC, which can be seen in its
definition.

It imho provides too much information about encoding to be used in less
performance critical code, and there are too many ways misusing and
getting confused about it (i.e. using non-existing values). Actually at
the moment you can't even describe free regions using it (there is no
"FreeMask"), and it would break down if you wanted to have one a bit
more complex set of regions.

This kind of shows of the use of the non-existing HumongousTag in your
given example too. :)

The given name does not describe the region type, but the kind of set
and how/when it is used, mainly for debugging purposes. You can't
derive it from the region type it contains only.

> 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. :)

I agree. I have some patches for cleaning this up already, effectively
removing this new enum. However it just adds some noise for this
particular change.

New webrevs at
http://cr.openjdk.java.net/~tschatzl/8208498/webrev.0_to_1/ (diff)
http://cr.openjdk.java.net/~tschatzl/8208498/webrev.1/ (full)

Thanks,
  Thomas