RFR (S): 8131734: Add free_archive_regions support to G1 for -Xshared:auto

[Thomas Schatzl]

Hi Tom,

  sorry for the delay...

On Thu, 2015-08-13 at 15:32 -0400, Tom Benson wrote:
> Hi Thomas,
> 
> On 8/12/2015 7:00 AM, Thomas Schatzl wrote:
> > Hi,
> >
> > On Tue, 2015-08-11 at 13:43 -0400, Tom Benson wrote:
> >> Hi,
> >> On 8/7/2015 10:56 AM, Tom Benson wrote:
> >> After some discussion, I've changed the definition and name of
> >> free_archive_regions.  Now called dealloc_archive_regions, it uncommits
> >> the specified regions, unmapping the memory, rather than adding them to
> >> the free list.  This means the CDS code will no longer do the unmapping
> >> on verification failures.
> >>
> >> Updated full and incremental webrevs of the GC code are at:
> >> http://cr.openjdk.java.net/~tbenson/8131734/webrev.02/
> >> http://cr.openjdk.java.net/~tbenson/8131734/webrev.02.vs.01/
> >>
> >> Tested with JPRT and running benchmarks with the dealloc_ performed
> >> explicitly.  Jiangli also tested the original failing cases, and will be
> >> posting an updated webrev.
> > - is it possible that shrink_by() uses shrink_at()? This would avoid two
> > paths that uncommit regions like expand_by()/expand_at()?
> 
> OK, I made the change.  I didn't do it originally because the asserts I 
> wanted to add for the call from g1CollectedHeap seemed superfluous for 
> the other call, and shrink_at was so small.    Now shrink_at takes a 
> region count as well.
> 


> Updated full and incremental webrevs are at:
>    http://cr.openjdk.java.net/~tbenson/8131734/webrev.03/
>    http://cr.openjdk.java.net/~tbenson/8131734/webrev.03.vs.02/
> 
> 

Looks good.

> >
> > - I think the change should call at least HeapRegion::hr_clear() on the
> > region to remove or reset any auxiliary data structures, if not
> > G1CollectedHeap::free_region() (without adding the region to the free
> > list).
> > Since the HeapRegion* is not deallocated by the uncommit, this may cause
> > strange behavior later when the region is reused.
> 
> I don't think calling hr_clear should be necessary...  If it is, we 
> should be doing it in shrink_by as well, and I don't think we are. I 
> don't see how a HeapRegion can be 'reused' without having gone through 
> the constructor when expand_ asks (indirectly) for 'new HeapRegion', and 
> that does an hr_clear() as well as the rest of init.     Or am I missing 
> something there?

Leave it as is. I thought that a full gc (which is the only case where
the heap shrinks at the moment) also clears the remset of these regions
at least.

It should, I filed  JDK-8134048 for looking in this issue.

Looks good.

Thanks,
  Thomas