RFR: 8214118: HeapRegions marked as archive even if CDS mapping fails
Thomas Schatzl
thomas.schatzl at oracle.com
Wed Nov 21 21:52:29 UTC 2018
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
More information about the hotspot-dev
mailing list