Introduce "region type" field to G1 HeapRegions

Tony Printezis tprintezis at twitter.com
Wed Sep 10 00:09:19 UTC 2014


Thomas,

On 9/9/14, 5:23 PM, Thomas Schatzl wrote:
> 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.

OK.

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

is_humongous()
is_StartsHumongous()
is_ContinuesHumongous()

At the risk of sounding totally pedantic :-), lower case h on the first 
and capital on the next two bothers me a bit. I'd prefer the 
all-lowercase version with underscores if that's OK.

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

Yep. You're totally right.

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

And the YoungType! :-) Done.

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

Yes, you did.

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

Done.

I'll post the new webrev shortly.

Tony

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

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

@TonyPrintezis
tprintezis at twitter.com




More information about the hotspot-gc-dev mailing list