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

[Bengt Rutisson]

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

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://mail.openjdk.org/pipermail/hotspot-gc-dev/attachments/20140212/28c8f7c0/attachment.htm>