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

[Thomas Schatzl]

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.

Thanks,
  Thomas