RFR: 8214118: HeapRegions marked as archive even if CDS mapping fails
stefan.johansson at oracle.com
Fri Nov 23 15:05:27 UTC 2018
Thanks for the reviews Jiangli and Thomas,
I pushed this without any changes after talking to Thomas offline.
On 2018-11-21 23:47, Jiangli Zhou wrote:
> Hi Thomas,
> On 11/21/18 2:00 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:
>>>> On Wed, 2018-11-21 at 15:43 +0100, Stefan Johansson wrote:
>>>>> 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
>>> 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)?
More information about the hotspot-dev