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

[Thomas Schatzl]

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

Thanks,
  Thomas