RFR: 8047818: G1 HeapRegions can no longer be ContiguousSpaces

[Mikael Gerdin]

Hi Thomas,

On Wednesday 25 June 2014 13.28.09 Thomas Schatzl wrote:
> Hi,
> 
> On Tue, 2014-06-24 at 17:39 +0200, Mikael Gerdin wrote:
> > On Tuesday 24 June 2014 14.20.30 Thomas Schatzl wrote:
> > > Hi,
> > > 
> > > On Mon, 2014-06-23 at 16:26 +0200, Mikael Gerdin wrote:
> > > > Hi!
> > > > 
> > > > When G1 is modified to unload classes without doing full collections
> > > > the
> > > > old HeapRegions can contain unparseable objects. This makes
> > > > ContiguousSpace unsuitable as a base class for HeapRegion since it
> > > > assumes that all objects below _top are parseable.
> > > > 
> > > > Modify G1OffsetTableContigSpace to implement allocation with a
> > > > separate
> > > > _top and reimplement some Space pure virtuals to make object iteration
> > > > work as expected.
> > > > 
> > > > This change is the last part of a set of 4 changes: 8047818, 8047819,
> > > > 8047820, 8047821 which are needed to refactor the HeapRegion class and
> > > > its superclasses in order to simplify the G1 class unloading change
> > > > which
> > > > is coming. This change depends on the 19, 20 and 21 changes.
> > > > 
> > > > Bug:
> > > > https://bugs.openjdk.java.net/browse/JDK-8047818
> > > > Webrev:
> > > > http://cr.openjdk.java.net/~mgerdin/8047818/webrev/
> > > > 
> > > > Notes:
> > > > The moving of set_offset_range is due to an introduced circular
> > > > dependency
> > > > between g1BlockOffsetTable.inline.hpp and heapRegion.inline.hpp
> > > > 
> > >   a few minor nits:
> > >  - in G1OffsetTableContigSpace::cas_allocate_inner(), the method should
> > > 
> > > access _top directly per coding guidelines
> > 
> > I interpret this as a request to change to
> > 
> >   HeapWord* obj = _top;
> > 
> > Should I change other uses of top() as well?
> > 
> > I could only find
> > https://wiki.openjdk.java.net/display/HotSpot/StyleGuide#StyleGuide-Access
> > ors as a reference here. Do you interpret that as "only use public
> > accessors if outside the class"?
> 
> I remember having been made aware of that we are supposed to use members
> directly within a class and its descendants a (small) few times when I
> started here - because I otherwise tend to add accessors except for very
> simple ones, mainly for private variables. I may have misunderstood
> something too.
> 
> Looking through the code, this might be (again) G1 code specific where it's
> done (relatively) frequently in code that is not almost copy&paste from CMS.
> 
> I remember some changes that also actively removed by-accessor accesses
> within a given class hierarchy (e.g. collector policy).
> 
> So in the end I may have misunderstood something, and as I did not really
> search for something "written down and generally accepted" at that time I am
> grateful to be corrected. I like this way better too.
> 
> So keep it as is.

Ok, I see your point.
I agree that adding accessors for all members just for the sake of it is 
generally not that useful. In this case I was sort-of mimicing the code in 
ContiguousSpace.

Per Jon and Stefan's requests I've copied the ContiguousSpace versions of 
allocate_impl and par_allocate_impl straight off instead of having slightly 
different own versions, so in the end I added a top_addr() as well since 
ContiguousSpace has one.

A new webrev is coming.

/Mikael

> 
> Thanks,
>   Thomas