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

[Jiangli Zhou]

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