RFR: 8047818: G1 HeapRegions can no longer be ContiguousSpaces

[Thomas Schatzl]

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

Thanks,
  Thomas