RFR: 8214118: HeapRegions marked as archive even if CDS mapping fails

[Stefan Johansson]

Thanks for the reviews Jiangli and Thomas,

I pushed this without any changes after talking to Thomas offline.

Cheers,
Stefan

On 2018-11-21 23:47, Jiangli Zhou wrote:
> Hi Thomas,
> 
> 
> On 11/21/18 2:00 PM, Thomas Schatzl wrote:
>> Hi,
>>
>> On Wed, 2018-11-21 at 13:05 -0800, Jiangli Zhou wrote:
>>> Hi Thomas,
>>>
>>> Just some comments for your comments. :)
>>>
>>> On 11/21/18 12:29 PM, Thomas Schatzl wrote:
>>>> Hi,
>>>>
>>>> On Wed, 2018-11-21 at 15:43 +0100, Stefan Johansson wrote:
>>>>> Hi,
>>>>>
>>>>> Please review this fix to avoid leaving heap metadata in an
>>>>> inconsistent state when CDS archive mapping fails.
>>>>>
>>>>> Issue: https://bugs.openjdk.java.net/browse/JDK-8214118
>>>>> Webrev: http://cr.openjdk.java.net/~sjohanss/8214118/00/
>>>>     the change seems good. I have two comments on this though:
>>>>
>>>> - would it be useful to clear both maps in
>>>> G1ArchiveAllocator::clear_range_archive()? I mean in the failure
>>>> case these regions can't be used for one purpose or the other as
>>>> soon as we fail the mapping, and saves passing the bool flag.
>>> We map the 'closed' archive regions before 'open' archive regions.
>>> The 'open' archive regions are only mapped if the 'closed' ones are
>>> successfully mapped.
>>>
>>> The 'open' archive regions have dependency on the 'closed' regions
>>> (objects in the open regions may have references to the closed
>>> objects).
>>> The 'closed' archive regions can be used independently however. If
>>> 'open' archive regions fail to map, we can still keep the 'closed'
>>> archives in use (no need to clear the closed map).
>> G1ArchiveAllocator::clear_range_archive() does not clear the whole heap
>> region range, just the given range which we failed to map the closed or
>> open archive into as far as I understand. There can be no overlap, so
>> clearing the range in both maps in that method should be fine.
>>
>> That's also why I suggested having a single map containing an enum
>> value and not two maps with a bool each.
>>
>> But maybe I'm too tired already today, so feel free to ignore my
>> suggestions :)
> 
> That makes sense to me! If we use a single map for both 'open' and 
> 'closed' archive regions, clearing is a 'safe' operation for a requested 
> range since the heap archive ranges are guaranteed to not be overlapped. 
> I'll create a RFE for your merged suggestion (if you haven't created 
> one). Thanks!
> 
> With the current separate maps, Stefan's fix that checks the boolean 
> argument probably is slightly cleaner (reflecting to the 
> G1ArchiveAllocator::set_range_archive() API)?
> 
> Thanks!
> Jiangli
>>
>> Thanks,
>>    Thomas
>>
>>
>