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

[Jiangli Zhou]

On 11/21/18 1:52 PM, Thomas Schatzl wrote:

> 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).
> Okay. Thanks for clarification.
>
>>> - 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...
> Not against using a map to do that, my suggestion is to merge them into
> a single map carrying an enum value (none/closed/open) instead of two
> each being a bool.
>
> There should be (almost) no difference in comparing to true/false than
> comparing to a small integer value.
>
> Just an idea.

Sounds good.

Thanks!
Jiangli
>
> Thanks,
>    Thomas
>
>