Introduce "region type" field to G1 HeapRegions

Sat Sep 13 02:45:35 UTC 2014

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/

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

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

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

@TonyPrintezis
tprintezis at twitter.com