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

[Thomas Schatzl]

Hi,

On Tue, 2015-11-03 at 14:26 +0100, David Lindholm wrote:
> 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.

Ok. That's good.

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

Thanks in general for re-adding asserts.

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

That's okay.

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

I would prefer that either the current behavior is kept, or
SingleHumongous removed completely. Either is fine for me, but it would
be awkward to sometimes label the same region with the same content as
SingleHumongous and sometimes different.

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

Okay.

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

Less need to search for uses of free_humongous_regions if something in
that code changes.

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

I would still prefer that it were guaranteed that the iteration cannot
go beyond the humongous object in any case.

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

:) I saw that this pattern has been used multiple times, great.

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

Okay, thanks.

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

Looks good except for the SingleHumongous and the iteration over the
humongous objects. Could that be fixed?

Thanks,
  Thomas