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