Introduce "region type" field to G1 HeapRegions

[Tony Printezis]

Thomas,

Thanks for looking at this. Inline.

On 9/9/14, 7:59 AM, Thomas Schatzl wrote:
> Hi Tony,
>
>    the change is useful. I created JDK-8057768 for it.
>
> On Fri, 2014-09-05 at 17:31 -0400, Tony Printezis wrote:
>> Hi all,
>>
>> One of the things that has always annoyed me in G1 is that working out
>> the type of a heap region is a complete mess:
>>
>> - there are separate fields for "humongous type" and "young type"
>> - to check whether a region is free we have to check is_empty()
>> - we cannot explicitly check whether a region is old (if it's none of
>> the other types, then it's old)
>>
>> If anyone wants to add new region types (as we all seem to want to do
>> these days!!!), doing so is a bit of a pain and more error-prone than it
>> should be. Could I ask you to consider the following patch that
>> introduces a HeapRegionType class, which encapsulates all the region
>> type information, and replaces the young / humongous type fields with
>> HeapRegionType _type?
>>
>> http://cr.openjdk.java.net/~tonyp/g1-region-type/webrev.0/
> Some comments:
>
>    - there is a compile error on non-gcc compilers: they do not allow a
> final comma after the last enum value in HeapRegionType::Tag.

Sorry, that was a mistake. I hate the use of comma as a terminator. :-)

> This is only allowed in C++11.
>
>    - please avoid use of the automatic coercion of ints to bools in the
> tests for is_young/is_humongous :).
> Actually some compilers complain about it.

Yeah, good point. != 0 is right here.

>    - while we are at touching this code I would appreciate if the method
> names for isHumongous(), startsHumongous() and continuesHumongous() in
> the HeapRegion class would be changed to fit the others as well. I.e.
> is_humongous(), is_startsHu.., is_contin...().
>
> That could probably done in an extra CR as it adds lots of noise.

How about this: I never liked the lower camel case in the *Humongous() 
methods on HeapRegion and I'd like to rename them to is_humongous(), 
is_starts_humongous(), is_continues_humongous(). However, as you pointed 
out that'd be a big noisy change (there are 100+ instances of those 
methods in the G1 codebase). So, how about I leave the methods on 
HeapRegionType as we'd like them to be and rename the rest as a 
follow-up change (and I'll be happy to work on that once this goes in)? 
Sounds reasonable?

>    - possibly rename reset_Humongous() to clear_humongous() too since we
> unset/clear the humongous-related fields.

Good suggestion.

>    - in HeapRegion::hr_clear(), instead of removing the assert, its
> condition could be changed to !is_humongous().

Actually, this doesn't hold any more, which is why I removed the assert. 
Note that clear_humongous() I doesn't change the type of the region any 
more, as I removed:

    _humongous_type = NotHumongous;

The type is set to free in hr_clear() (which is called immediately after 
clear_humongous()) (same reason I removed the same assert from 
g1HotCardCache.cpp BTW). I could set the type to free in 
clear_humongous() too and re-add the assert but I don't think it 
matters. The main requirement is that these two fields should be cleared 
in clear_humongous():

   assert(_humongous_start_region == NULL,
          "we should have already filtered out humongous regions");
   assert(_end == _orig_end,
          "we should have already filtered out humongous regions");

>    - it's fine with me to add a HeapRegionType class. Others may have a
> different preference.

I also like it too. I could also put the extra functionality in 
HeapRegion.* (but those files are already bloated).

>    However, the heapRegionType.?pp files need some cleanup:
>      - copyright dates for new files are just the last year

Thanks for fixing this.

>      - I do not think we need the #if INCLUDE_ALL_GCS here or in any
> heapRegion* file. We can clean that up later.

OK.

>      - the encoding table for the Tag should probably be moved to the
> enum. Further it would be nice to write an introductory sentence what
> this table is about and why.

OK. (And yes I'll expand the comment on the tags a bit)

One more thing I should have made clear: given that I now use 
HeapRegion::get_type_str() in HR_FORMAT_PARAMS() and in 
G1PrintRegionLivenessInfoClosure (concurrentMark.cpp), instead of the 
ad-hoc code that was there, the output of both (and there's a fair 
amount of output that relies on the former) is going to change. Is that 
a problem? I could make sure the output remains the same (by adding more 
get_*_str() methods). I personally don't think it's worth it.

> I put up a webrev with some of the suggested changes at
> http://cr.openjdk.java.net/~tschatzl/8057768/webrev/

Thanks for making the changes. I'll send an update based on your patch 
(and once we agree on some of the issues I mentioned above).

Tony

-- 
Tony Printezis | JVM/GC Engineer / VM Team | Twitter

@TonyPrintezis
tprintezis at twitter.com