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

Hohensee, Paul hohensee at amazon.com
Tue Aug 21 14:49:50 UTC 2018


Looks good.

Thanks,

Paul

On 8/20/18, 7:58 AM, "Thomas Schatzl" <thomas.schatzl at oracle.com> 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)
    
    Thanks,
      Thomas
    
    



More information about the hotspot-gc-dev mailing list