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

[Jiangli Zhou]

Could someone also help review the runtime part? I need one more Reviewer.

  http://cr.openjdk.java.net/~jiangli/8131734/webrev.02/ <http://cr.openjdk.java.net/~jiangli/8131734/webrev.02/>

Thanks,
Jiangli


> On Aug 23, 2015, at 1:54 PM, Dmitry Dmitriev <dmitry.dmitriev at oracle.com> wrote:
> 
> Hi Jiangli,
> 
> Looks good to me!
> 
> Thank you,
> Dmitry
> 
> On 22.08.2015 4:47, Jiangli Zhou wrote:
>> Hi Dmitry,
>> 
>> On Aug 21, 2015, at 8:25 AM, Dmitry Dmitriev <dmitry.dmitriev at oracle.com> wrote:
>> 
>>> Hello Jiangli,
>>> 
>>> This looks good to me, but I'm not a reviewer.
>> Thanks.
>> 
>>> Also, I have question to you. Probably you can clarify me one moment.
>>> num_ranges and string_ranges are modified only in code under "#if INCLUDE_ALL_GCS", so it make sense to include all usage of these variables also under "#if INCLUDE_ALL_GCS"? I mean following functions:
>>> FileMapInfo::fixup_string_regions() and new FileMapInfo::dealloc_string_regions() function.
>> Agreed. Here is the updated webrev: http://cr.openjdk.java.net/~jiangli/8131734/webrev.02/src/share/vm/memory/filemap.cpp.sdiff.html.
>> 
>> I also reverified JPRT builds with the new #ifdef changes.
>> 
>> Thanks for the detailed review!
>> 
>> Jiangli
>> 
>>> Thank you,
>>> Dmitry
>>> 
>>> On 20.08.2015 22:55, Jiangli Zhou wrote:
>>>> Hi Dmitry,
>>>> 
>>>> Here is the updated runtime webrev that reflects Tom’s latest GC changes.
>>>> 
>>>> http://cr.openjdk.java.net/~jiangli/8131734/webrev.01/
>>>> 
>>>> I renamed the FileMapInfo::unmap_string_regions() to FileMapInfo::dealloc_string_regions(), which only deallocates the archived string region from the java heap without unmapping. The unmapping is handled by the GC system as the archived string region is part of the java heap. I also added dealloc_string_regions() call to the case where the string region verification fails.
>>>> 
>>>> Thanks,
>>>> Jiangli
>>>> 
>>>> On Aug 20, 2015, at 7:12 AM, Tom Benson <tom.benson at oracle.com> wrote:
>>>> 
>>>>> 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
>>>>>> 
>>>>>> 
> 

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://mail.openjdk.org/pipermail/hotspot-gc-dev/attachments/20150825/c6ae2f57/attachment.htm>