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

[Jiangli Zhou]

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

>
> - this is a cleanup request for the future: is there a reason to have
> separate tables for open and closed archives? I understand that the
> code happened this way because these types of archives were added at
> different times, but in my understanding a region in that table can
> either be one or the other, and by using a uint8_t as per-region entry
> we can even save space.

One benefit of two separate maps is that we can quickly find which range 
an object is in. It probably could be done without using the maps...

Thanks,
Jiangli

>
> Thanks,
>    Thomas
>
>