RFR: 8047818: G1 HeapRegions can no longer be ContiguousSpaces

[Stefan Karlsson]

On 2014-06-23 16:26, 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/

http://cr.openjdk.java.net/~mgerdin/8047818/webrev/src/share/vm/gc_implementation/g1/heapRegion.hpp.udiff.html

+  inline HeapWord* cas_allocate_inner(size_t size);
+  inline HeapWord* allocate_inner(size_t size);


Could you move these declarations to a point after the variable 
declarations?

+  inline void set_top(HeapWord* value) { _top = value; }


No need for the inline keyword here.

  void G1OffsetTableContigSpace::clear(bool mangle_space) {
-  ContiguousSpace::clear(mangle_space);
+  set_top(bottom());
+  CompactibleSpace::clear(mangle_space);


ContiguousSpace::clear calls   void set_saved_mark()            { 
_saved_mark_word = top();    }

I think it might be worth being a bit defensive and doing the same from 
G1OffsetTableContigSpace::clear.

+void G1OffsetTableContigSpace::object_iterate(ObjectClosure* blk) {
+  HeapWord* p = bottom();
+  if (!block_is_obj(p)) {
+    p += block_size(p);
+  }
+  while (p < top()) {
+    blk->do_object(oop(p));
+    p += block_size(p);
+  }

Shouldn't blk->do_object(oop(p)) be guarded by: if (!block_is_obj(p))


  G1OffsetTableContigSpace::
  G1OffsetTableContigSpace(G1BlockOffsetSharedArray* sharedOffsetArray,
                           MemRegion mr) :
+  _top(bottom()),
    _offsets(sharedOffsetArray, mr),
    _par_alloc_lock(Mutex::leaf, "OffsetTableContigSpace par alloc lock", true),
    _gc_time_stamp(0)
  {
    _offsets.set_space(this);
    // false ==> we'll do the clearing if there's clearing to be done.
-  ContiguousSpace::initialize(mr, false, SpaceDecorator::Mangle);
+  CompactibleSpace::initialize(mr, false, SpaceDecorator::Mangle);


bottom() is used before _bottom has been initialized to the correct 
value. As the code stands we set _top to NULL. _bottom is initialized 
through this call chain:
  CompactibleSpace::initialize.
   Space::initialize
    set_bottom

And the SA agent needs to be updated. =)

thanks,
StefanK

>
> Notes:
> The moving of set_offset_range is due to an introduced circular dependency
> between g1BlockOffsetTable.inline.hpp and heapRegion.inline.hpp
>
> Thanks
> /Mikael
>