RFR: 8047818: G1 HeapRegions can no longer be ContiguousSpaces

[Mikael Gerdin]

Jon,

On Tuesday 24 June 2014 09.34.57 Jon Masamitsu wrote:
> On 06/24/2014 07:43 AM, Mikael Gerdin wrote:
> > Jon,
> > 
> > On Tuesday 24 June 2014 07.26.52 Jon Masamitsu wrote:
> >> Mikael,
> >> 
> >> Did you consider creating a base class for ContiguousSpace and
> >> G1OffsetTableContigSpace that has a  _top but does not assume
> >> parsability?
> > 
> > I did consider it but I thought that the added complexity of having even
> > more levels of inheritance were not worth the benefit of sharing the _top
> > field. Especially since G1 attempts to hack around the semantics around
> > the _top field with respect to concurrent access. See the disjunction I
> > removed from allocate_impl and when G1 calls cas_allocate and allocate.
> 
> Ok.  That's enough of a reason.

Thanks. I'm planning on filing an RFE for unifying the allocation code between 
G1 and ContiguousSpace somehow.

> 
> >> Could allocate_inner() have been called allocate_impl() as it is
> >> in ContiguousSpace?  I don't know what the "inner" in the name
> >> is telling me.
> > 
> > "inner" tries to signal that this function is internal and wrapped by
> > other
> > methods providing the external API. I don't have a particular naming
> > preference here, if the other reviewers are fine with "impl" or don't have
> > a preference I'm fine with changing it.
> 
> If no one objects, the "impl" has more of a meaning to me.

Ok, naming it "impl" also makes it closer to the ContiguousSpace version.

> 
> 
> http://cr.openjdk.java.net/~mgerdin/8047818/webrev/src/share/vm/gc_implement
> ation/g1/heapRegion.inline.hpp.frames.html
> 
> You use the  if-then {return ...} return NULL style.
> 
>    52 inline HeapWord* G1OffsetTableContigSpace::allocate_inner(size_t size)
> { 53   HeapWord* obj = top();
>    54   if (pointer_delta(end(), obj) >= size) {
>    55     HeapWord* new_top = obj + size;
>    56     assert(is_aligned(obj) && is_aligned(new_top), "checking
> alignment"); 57     set_top(new_top);
>    58     return obj;
>    59   }
>    60   return NULL;
>    61 }
> 
> The ContiguousSpace uses the if-then {return ...} else {return NULL} style.
> 
> Any reason not to use the same style?

No good reason. I'll keep the new ones in the same style.

> 
> block_is_obj() seems more like an is_in() method.
> 
>    89 inline bool
>    90 HeapRegion::block_is_obj(const HeapWord* p) const {
>    91   return p < top();
>    92 }
> 

It's actually the same as ContiguousSpace::block_is_obj
When G1 class unloading is integrated the implementation of 
HeapRegion::block_is_obj will change.

> 
> Could you add a specification for this block_size()?
> 
>    94 inline size_t
>    95 HeapRegion::block_size(const HeapWord *addr) const {
>    96   const HeapWord* current_top = top();
>    97   if (addr < current_top) {
>    98     return oop(addr)->size();
>    99   } else {
>   100     assert(addr == current_top, "just checking");
>   101     return pointer_delta(end(), addr);
>   102   }
>   103 }

Similar to block_is_obj this is currently the same as the ContiguousSpace 
variant. When G1 class unloading is integrated the implementation will change.

The other declarations of block_size are not that clearly specified, can you 
elaborate on what kind of specification you are looking for?

/Mikael

> 
> Jon
> 
> >> I've just started on the review so more to come.
> > 
> > Great!
> > 
> > Thanks
> > /Mikael
> > 
> >> Jon
> >> 
> >> On 06/23/2014 07:26 AM, 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
> >>> 
> >>> Thanks
> >>> /Mikael