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

Thu Mar 13 21:31:33 UTC 2014

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