Introduce "region type" field to G1 HeapRegions

[Bengt Rutisson]

Hi Tony,

On 2014-09-13 04:45, Tony Printezis wrote:
> Hi Bengt, Thanks for looking at it. Please see inline.
>
> On 9/12/14, 8:20 AM, Bengt Rutisson wrote:
>>
>> 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.
>
> I've always written macros really defensively, but that's fine, I'll 
> change it.
>
>> 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 ?
>
> Yes, good catch. Bad copy-and-paste.
>
>> 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.
>
> Thanks also for catching this. Yes, the comment is wrong. We 
> definitely have to remove the old regions from the old set.
>
>> 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.
> I removed the comment and I also collapsed the if-statements given 
> that they are not needed any more because we can now check is_old().
>
> The new webrev is here:
>
> http://cr.openjdk.java.net/~tonyp/g1-region-type/webrev.2/
>
> And these are the diffs over the previous one:
>
> http://cr.openjdk.java.net/~tonyp/g1-region-type/webrev.2.diff/

Looks good! Thanks for fixing this!


>
> Thanks again. BTW, I don't know your current policy: Is this a change 
> you'll be interested in back-porting to JDK 8?

I think it would be good to backport this to the 8u repo to make other 
backports simpler. But I can't say right now when we can do the 
backport. As you can see from this schedule:

http://openjdk.java.net/projects/jdk8u/releases/8u40.html

the next 8u release is close to being feature complete so we might have 
to wait until the next 8u release to push this.

BTW, I can sponsor this fix and make sure it gets pushed to the JDK 9 
repo as soon as we have complete reviews.

Thanks,
Bengt

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