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

[Tom Benson]

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/


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

Thanks,
Tom


>
> Other than that it looks okay.
>
> Thanks,
>    Thomas
>
>