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

[Thomas Schatzl]

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.

> > - G1CollectedHeap::free_region_if_empty() was inlined

Seems okay. At least I saw no better way immediately. I will think about it.

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

> >
> > - Removed the HRSPhaseSetter, which may soften the verification a bit 
> > but not much.

I do not mind. Does not seem really important.

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

- In heapRegionSet.hpp there is a comment about a "HeapRegionLinkedList" which does not exist any more.

- in FreeRegionList::remove_head_or_null() the MT check seems missing now.

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

- 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...").

Overall I like this refactoring :)

Thanks,
  Thomas