RFR: 8214118: HeapRegions marked as archive even if CDS mapping fails
jiangli.zhou at oracle.com
Sat Nov 24 02:30:17 UTC 2018
Hi Thomas and Stefan,
I've filed https://bugs.openjdk.java.net/browse/JDK-8214277 for merging
the open and closed maps.
On 11/21/18 2:47 PM, 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