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

[Tom Benson]

Hi Thomas,
OK, thanks!
Tom

On 8/20/2015 10:06 AM, Thomas Schatzl wrote:
> 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
>
>