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

[Per Liden]

Looks good Bengt!

/Per

On 03/13/2014 10:31 PM, Bengt Rutisson wrote:
>
> Hi Per,
>
> Thanks for looking at this! I fixed all of your comments.
>
> Here is an updated review:
>
> http://cr.openjdk.java.net/~brutisso/8034079/webrev.03/
>
> And here is the diff compared to the previous version:
>
> http://cr.openjdk.java.net/~brutisso/8034079/webrev.02-03.diff/
>
> Thanks,
> Bengt
>
> On 3/13/14 2:01 PM, Per Liden wrote:
>> 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
>>>>
>>>>
>>>>
>>>>
>>>
>>
>