Introduce "region type" field to G1 HeapRegions

[Bengt Rutisson]

Hi Tony,

Nice cleanup! I like it!

On 2014-09-10 03:03, Tony Printezis wrote:
> Latest webrev which I think includes all that we discussed:
>
> http://cr.openjdk.java.net/~tonyp/g1-region-type/webrev.1/

This webrev looks good. A few minor comments below.

Thanks,
Bengt


heapRegionType.hpp


   30 #define hrt_assert_is_valid(__tag__) \
   31 do { \
   32   assert(is_valid((__tag__)), \
   33          err_msg("invalid HR type: %u", (uint) (__tag__))); \
   34 } while (0)

I don't think we normally use "__" for parameters to macros, so I would 
prefer "__tag__" to be called just "tag". The way the macro is being 
used I don't see any need for the do {} while loop.


g1CollectedHeap.cpp

1243     } else if (hr->is_old()) {
1244       _hr_printer->post_compaction(hr, 
G1HRPrinter::ContinuesHumongous);

This looks wrong. Shouldn't we be printing G1HRPrinter::Old ?


In TearDownRegionSetsClosure this comment confuses me:

6702     } else if (r->is_old()) {
6703       // Add it to the old set.
6704       _old_set->remove(r);

The comment says "add", but we do "remove". The old comment "// The rest 
should be old" wasn't great but makes a little sense. We assume at this 
point that all regions are old and thus should be removed from the old 
set since we are tearing down all old region. I would propose to remove 
the comment all together now that we explicitly identify old regions 
instead. Or possibly revert back to the old behavior with a simple else 
case and then assert that the region is old.


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