RFR: 8047818: G1 HeapRegions can no longer be ContiguousSpaces

[Jon Masamitsu]

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.

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


http://cr.openjdk.java.net/~mgerdin/8047818/webrev/src/share/vm/gc_implementation/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?

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 }


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 }

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