RFR (M): 8034079: G1: Refactor the HeapRegionSet hierarchy

[Per Liden]

I sat down with Bengt and reviewed this. Looks good over all, we found 
and discussed a few minor thing. I'll summarize them here:

* HRSMtSafeChecker::check() does not need to return true/false

* Default initialize HRSMtSafeChecker to NULL as that seems to be a 
fairly common use case.

* Maybe these should return "const HeapRegionSetCount&" instead?

   HeapRegionSetCount old_regions_removed();
   HeapRegionSetCount humongous_regions_removed()

* The call to G1CollectedHeap::decrement_summary_bytes_mt() could be 
moved into the locked region and converted to the less less version (to 
avoid taking the same lock twice in a row)?

* G1CollectedHeap::remove_from_sets() rename to 
G1CollectedHeap::remove_from_old_sets()

* Add verify_counts() instead of having the following fields public?

   HeapRegionSetCount _old_count;
   HeapRegionSetCount _humongous_count;
   HeapRegionSetCount _free_count;

* This field doesn't need to be public:

   static uint _unrealistically_long_length;


cheers,
/Per

On 02/12/2014 11:24 PM, Bengt Rutisson wrote:
>
> Hi Thomas,
>
> Thanks for looking at this webrev!
>
> On 2/12/14 7:10 PM, Thomas Schatzl wrote:
>> Hi,
>>
>> On Tue, 2014-02-11 at 11:50 +0100, Bengt Rutisson wrote:
>>> Hi again,
>>>
>>> I fixed the serviceability agent (SA) and couple of minor things (in
>>> particular HeapRegionSetBase::_is_linked was not used). Here is an
>>> updated webrev:
>>>
>>> http://cr.openjdk.java.net/~brutisso/8034079/webrev.01/
>>>
>>> Here is the diff compared to the previous version in case anybody has
>>> started looking at it:
>>>
>>> http://cr.openjdk.java.net/~brutisso/8034079/webrev00.01.diff/
>>>
>>> Thanks,
>>> Bengt
>>>
>>> On 2014-02-10 14:04, Bengt Rutisson wrote:
>>>> Hi everyone,
>>>>
>>>> Could I have a couple of reviews for this change?
>>>>
>>>> http://cr.openjdk.java.net/~brutisso/8034079/webrev.00/
>>>>
>>>> It tries to simplify the structure of the HeapRegionSets. I need this
>>>> to be able to introduce new types of heap region collections.
>>>>
>>>> Some other cleanups are done as a consequence of this. But there are
>>>> probably still more cleanups to do. I hope that this is a step in the
>>>> right direction.
>>>>
>>>> Here's a short list of some of the changes:
>>>>
>>>> - Class hierarchy simplified
>>>> No longer specific subclasses for all instances. Verification code has
>>>> been collected in the super classes and the MT safety checks are
>>>> aggregated instead.
>>>>
>>>> - Proxy sets replaced by HeapRegionSetCount
>>>> The use of "proxy sets" was removed in favor of a light weight class
>>>> to keep track of length and capacity.
>> - is it required to keep track of both length and capacity? To me it
>> seems that the value passed to capacity is strictly always length *
>> HeapRegionSize.
>
> We need both due to the kind of strange way we handle humongous regions.
> We only add the humongous start regions, but their capacity is the
> capacity of the start humongous region and all the corresponding
> continues humongous regions. It would be nice to implement that
> differently in my opinion, but it is out of scope for this change.
>
>>
>>>> - G1CollectedHeap::free_region_if_empty() was inlined
>> Seems okay. At least I saw no better way immediately. I will think about it.
>
> Good.
>
>>
>>>> - G1CollectedHeap::update_sets_after_freeing_regions() was split up
>>>> into three (or actually four) methods.
>> Seems a good idea because very frequently it got passed many constant NULL or 0
>> values for parameters anyway.
>
> Yes, that's what I thought.
>
>>
>>>> - Removed the HRSPhaseSetter, which may soften the verification a bit
>>>> but not much.
>> I do not mind. Does not seem really important.
>
> Agreed.
>
>>
>>>> - The verification code also prints less information in some cases.
>>>> But instead it prints the relevant information. There is still more
>>>> cleanup that can be done to the messages from the asserts and
>>>> verifications.
>> :)
>>
>>>> - Moved usage accounting up to avoid having to pass it around as much.
>>>>
>>>> - HeapRegionSetBase::total_used_bytes() was only used in asserts. Did
>>>> not seem worth keeping around.
>>>>
>>>> After these changes the file heapRegionSets.hpp (notice the s at the
>>>> end of the name) was empty so I removed it. However, the
>>>> heapRegionSets.cpp file is still present. I would like to move all the
>>>> code there in to heapRegionSet.cpp, but that would make the webrev
>>>> more difficult to follow. I'm still open to doing that if someone
>>>> would like me to do it.
>> I am all for it, bug a separate CR would be fine for me.
>
> Great. I'll file a CR for that.
>
>>
>> - In heapRegionSet.hpp there is a comment about a "HeapRegionLinkedList" which does not exist any more.
>
> Thanks for catching that. I actually went with your suggestion below to
> just remove all ///////// comments.
>
>>
>> - in FreeRegionList::remove_head_or_null() the MT check seems missing now.
>
> Right. Added it back. Good catch!
>
>>
>> - heapRegionSets.cpp contains a "//// MasterFreeRegionList ///" comment that seems obsolete. Maybe all these lines with the many slashes could be removed or changed to something less obtrusive ;)
>
> Yes, I removed all the ///////// comments. I agree that they are mostly
> in the way.
>
>>
>> - in heapRegionSet.cpp some verification code is duplicated (e.g. line 158). Could this be factored out? (around the comment "// In set_containing_set() we check that we either...").
>
> Good point. I added a common method called add_as_head_or_tail().
>
>>
>> Overall I like this refactoring :)
>
> Thanks!
>
> Updated webrev here:
> http://cr.openjdk.java.net/~brutisso/8034079/webrev.02/
>
> And here is the diff compared to the 01 version:
> http://cr.openjdk.java.net/~brutisso/8034079/webrev.01-02.diff/
>
> Thanks again for looking at this!
> Bengt
>
>
>
>>
>> Thanks,
>>    Thomas
>>
>>
>>
>>
>