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

[sangheon.kim at oracle.com]

Hi Thomas,

On 8/20/18 7:58 AM, Thomas Schatzl wrote:
> 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)
Looks good to me too.

Thanks,
Sangheon


>
> Thanks,
>    Thomas
>