Introduce "region type" field to G1 HeapRegions

[Thomas Schatzl]

Hi,

On Tue, 2014-09-09 at 16:43 -0400, Tony Printezis wrote:
> Thomas,
> 
> Thanks for looking at this. Inline.
> 
> On 9/9/14, 7:59 AM, Thomas Schatzl wrote:
> >    - 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?

Yes, let's leave the rename out for now.

I quickly thought about the naming, the state names were StartsHumongous
and ContinuesHumongous, so similar to the others, I would somewhat
slightly prefer to keep them together (i.e. is_<state-name>) and not add
underscores to break the two words of the state name. Not really against
the proposed naming though (adding the underscore).

What I would really like to avoid is to have different order of the
words in the state name: right now the change uses
is_continues_humongous() in HeapRegion but is_humongous_cont() in
HeapRegionType. That seems to be unnecessary.

My suggestion is to use the same name in HeapRegionType and HeapRegion.

(Btw, the change should also remove the HumongousType enum in
heapRegion.hpp)

> >    - 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");
> 

Okay.

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

Actually I think I already fixed that in the change I gave you.

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

Need to look at that again, but I would somewhat prefer to keep the
short names, e.g. HC/HS in that output.

A get_short_str() method isn't be that hard to add.

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

Thanks,
  Thomas