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

[David Lindholm]

Hi Thomas,

Thanks for looking at this! Comments inline. The new webrevs are 
available here:

Webrev: http://cr.openjdk.java.net/~david/JDK-8139867/webrev.02/
Webrev: http://cr.openjdk.java.net/~david/JDK-8139867/webrev.01-02/ (diff)


On 2015-11-02 13:23, Thomas Schatzl wrote:
> 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.

I started doing _end volatile, but that became a quite large change in 
itself. I would like to add a comment that end() never changes in G1 and 
do the _end volatile in another change.

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

Ok, added an assert().

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

I would like to not fix this in this change, since it is a separate issue.

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

Ok.

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

Done.

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

Removed.

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

Yes, removed.

>   - please fix the newline in g1BlockOffsetTable.cpp:502

Ok.

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

You are correct. The problem here was that 
_g1h->reset_gc_time_stamps(r); were not called for ContinousHumongous. I 
moved that call
above the check and reinserted the asserts.

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

Because SingleHumongous doesn't seem to be a valid HeapRegionType, and 
that HeapRegion::region_num() doesn't exist any more, since it is always 
1 now. Sure, I could check if the next hr (by index) is not a 
ContinousHumongous or if the object is smaller than a region size, but 
since SingleHumongous is not a concept in HeapRegionTypes, I didn't.

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

Removed.

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

It is not skipped, r->verify is still called. The only thing that is not 
called is the size calculation and size check. Would it be ok to leave 
it that way?

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

Fixed.

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

Ok, fixed.

>   - in the trace messages in
> G1FreeHumongousRegionClosure::doHeapRegion(), please add a space in
> "objectsize".

Fixed.

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

But you have this information anyway, and this is under a Traceflag.

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

This function could be used at 2 places (I think), but it would have to 
take 2 arguments (the Count and the regionlist).
I don't really see the point in this refactorization.

>   - 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];
>    }
> }

I disagree. I don't think this HeapRegion should have any knowledge 
about the other HeapRegions, that breaks the abstraction imho. Making 
HeapRegion (more) self contained is one of the points of this change. If 
we want information about other HeapRegions we should go via the 
HeapRegionManager.

About the name: It has everything to do with index. It is "next by 
index", not next by next()-pointer. When doing this I first used the 
next() but I found out that next() is something else.

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

Ok, fixed.

>   - 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())

Because this asserts fails if p is in a ContinousHumongous (from is the 
StartsHumongous). I have modified the assert to reflect this.

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

That is a good question. I put the original code back. Sorry for the noise.

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

Ok, fixed.

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

Yes ok.
>   - 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?

Ok, fixed.


Thanks,
David
> Thanks,
>    Thomas
>
>