RFR(M): 8139867: Change how startsHumongous and continuesHumongous regions work in G1.

[Thomas Schatzl]

Hi David,

On Thu, 2015-10-29 at 14:34 +0100, David Lindholm wrote:
> Hi,
> 
> Please review the following patch that changes how startsHumongous and 
> continuesHumongous regions work in G1. Before this patch the top and end 
> of the startsHumongous regions were set so that they covered the entire 
> object, pointing into the last continousHumongous region for this 
> object. So used() and capacity() of the startsHumongous region could be 
> substantially larger than a normal region size.
> 
> Now each region is "self-contained", so top and end will not point 
> outside the region. This makes it possible to remove many special cases 
> in the code, and the model is more clear. Also, one doesn't have to 
> figure out if G1CollectedHeap::heap_region_containing_raw() or 
> G1CollectedHeap::heap_region_containing() should be called (only the 
> latter is left in the code).
> 
> 
> Bug: https://bugs.openjdk.java.net/browse/JDK-8139867
> Webrev: http://cr.openjdk.java.net/~david/JDK-8139867/webrev.00/
> 
> Testing: Passed JPRT.
> 

Thanks for the change. Overall I think it makes the code more
understandable.

Some comments:

 - in CheckBitmapClearHRClosure: while HeapRegion::end() cannot change,
it would still be nice if the value would be guaranteed to be only read
once. I.e. either make Space::_end volatile, or the assignment to "end"
from it.
Could you also modify HeapRegion::set_end() to make sure that it can
only be set to one value? (i.e. _bottom + HeapRegion::GrainBytes?) Or
alternatively only called once if that is possible?

 - pre-existing: maybe the call to BitMap::par_at_put() in
CMCountDataClosureBase::set_bit_for_region() could be changed to
BitMap::par_set_bit().

 - please add a comment in CalcLiveObjectsClosure::doHeapRegion() that
the new early exit has to do with how humongous objects are handled.

 - it would be nice to explain the (new) invariants for humongous
continues regions are in regards to top() and other accounting are. I
think at the top of the HeapRegion declaration should be a fairly good
place.

 - the long comment in ConcurrentMark::claim_region() is not correct any
more.

 - the comment in ConcurrentMark::verify_no_cset_oops() starting from
line 2672 does not make sense any more. I mean, the comment reads as if
the code does something special, but there is no alternative way now.
(Like the _raw method)

 - please fix the newline in g1BlockOffsetTable.cpp:502

 - PostMCRemSetClearClosure::doHeapRegion(): I still think that the
assert that has been deleted is valid. Humongous continues regions
should not have a remembered set as they do not contain a "full" object.

 - PostCompactionPrinterClosure::doHeapRegion: what's the reason for the
removal of the SingleHumongous HR printer event? While it is debatable
whether there should be a difference between humongous objects that span
only a single region and others, I think this should be a different
change.

 - pre-existing: potentially remove the comment in the implementation of
G1CollectedHeap::is_in(). It does not explain anything imo.

 - I think the code in VerifyRegionClosure::doHeapRegion() should not
skip humongous regions and the code fixed but not skipped entirely.

 - indentation of arguments in the definition of
G1CollectedHeap::free_humongous_region() is off.

 - G1CollectedHeap::free_humongous_region() could assert that the passed
region is a humongous region. They could replace the deleted
"hr->is_starts_humongous()" asserts.

 - in the trace messages in
G1FreeHumongousRegionClosure::doHeapRegion(), please add a space in
"objectsize". Also, the size in regions is interesting. While it could
be derived from the object size easily, it's nicer to have it on hand.

Maybe add a G1CollectedHeap::humongous_obj_size_in_regions(size_t
word_size) that does that calculation?

 - is it possible to (at least) wrap the code sequence:

r->set_containing_set(NULL);
<some_heap_regionsetcount>.increment(1u, r->capacity())
g1h->free_humongous_region(r, some_free_region_list, false)

return r->used();

into a method? That method could at least be used in two places (if not
three).

 - G1CollectedHeap::next_region_by_index(): I would prefer that method
would be limited to the case it is used for now, i.e. humongous objects.
That method as is allows iteration of the entire heap, and that is
functionality that seems way too powerful to be included in this change.

It is also underspecified in behavior regarding concurrent operation on
the underlying memory. It does not have anything to do with an "index"
as suggested by the name.

I.e. some method that lets you get the next region in the regions
occupied by a single humongous object would be significantly better.

E.g. in pseudo code:

HeapRegion::get_next_humongous_region(HeapRegion* r) {
  assert(r->is_humongous() && r is part of the current humongous
object*, "invariant");
  if (is last region) {
    return NULL;
  } else {
    return region[r->index + 1];
  }
}

* either a humongous start region, or the object in the associated
humongous start region covers that region.

G1 guarantees that the memory associated with a humongous object is
contiguous and valid/live as long as the humongous object is live.

This will also simplify the two while loops in the places we iterate
over all regions in the humongous objects.

Then G1CollectedHeap::next_region_by_index and the method in
HeapRegionManager can go away too.

 - G1PrepareCompactClosure::doHeapRegion(): I think that
HeapRegion::humongous_start_region() references itself in a humongous
start region, i.e. there is no need to have different paths to find out
"obj".

 - why the removal of the assert in G1RemSet::par_write_ref()? I do
think that _from cannot be NULL, because it is explicitly checked in the
only caller of par_write_ref, and write_ref is never called. (I filed
JDK-8141135 to remove write_ref())

 - not sure about the reason for the removal of the _hr parameters in
HeapRegionDCTOC::walk_mem_region(). They seem okay as the HeapRegion is
readily available.

 - I do not understand the change in VerifyLiveClosure::do_oop_work. Why
would the code in the if-clause fail when the source is a humongous
continues object?

 - I would prefer that the verification code in HeapRegion::verify()
would check that the region is part of the object instead of skipping
the test entirely (in line heapRegion.cpp:811)

 - the removed assert in heapRegion::reset_during_compaction() could be
replaced by is_humongous() instead of removing it entirely.

 - can we add an assert in HeapRegion::block_is_obj() that the new case
can only occur with humongous objects, and that p must be in the same
humongous object?

Thanks,
  Thomas