Introduce "region type" field to G1 HeapRegions

Thomas Schatzl thomas.schatzl at oracle.com
Tue Sep 9 11:59:34 UTC 2014


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

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

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

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

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

  However, the heapRegionType.?pp files need some cleanup:
    - copyright dates for new files are just the last year
    - I do not think we need the #if INCLUDE_ALL_GCS here or in any
heapRegion* file. We can clean that up later.
    - 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.

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

Thanks,
  Thomas




More information about the hotspot-gc-dev mailing list