RFR (S): 8214277: Use merged G1ArchiveRegionMap for open and closed archive heap regions

[Jiangli Zhou]

Hi Thomas,

The update looks good.

Best regards,
Jiangli

On Thu, Jan 9, 2020 at 3:39 AM Thomas Schatzl <thomas.schatzl at oracle.com> wrote:
>
> Hi Kim, Jiangli,
>
>    thanks for your reviews.
>
> On 09.01.20 01:25, Kim Barrett wrote:
> >> On Jan 8, 2020, at 8:38 AM, Thomas Schatzl <thomas.schatzl at oracle.com> wrote:
> >>
> >> Hi all,
> >>
> >>   could I have reviews for this small cleanup/simplification that merges the open and closed archive region map into a single one?
> >>
> >> CR:
> >> https://bugs.openjdk.java.net/browse/JDK-8214277
> >> Webrev:
> >> http://cr.openjdk.java.net/~tschatzl/8214277/webrev/
> >> Testing:
> >> hs-tier1-5 (almost done, no issues), local gc/g1 jtreg
> >>
> >> Thanks,
> >>   Thomas
> >
> > ------------------------------------------------------------------------------
> > src/hotspot/share/gc/g1/g1Allocator.inline.hpp
> >   151 inline void G1ArchiveAllocator::clear_range_archive(MemRegion range, bool open) {
> >
> > clear_range_archive no longer uses the open argument for anything
> > other than logging.  Is it worth keeping?
>
> Removed.
>
> >
> > ------------------------------------------------------------------------------
> > src/hotspot/share/gc/g1/g1Allocator.inline.hpp
> >   186   return (archive_check_enabled() &&
> >   187          (_archive_region_map.get_by_address((HeapWord*)object) != G1ArchiveRegionMap::NoArchive));
> >
> > The indentation of line 187 is confusing; the indentation suggests the
> > open paren there is at the same nesting level as the one on the
> > preceeding line directly above.  The first open paren on line 186 and
> > the associated close could just be removed.
> >
> > ------------------------------------------------------------------------------
> > src/hotspot/share/gc/g1/g1Allocator.inline.hpp
> >   162   // This is the out-of-line part of is_closed_archive_object test, done separately
> >   163   // to avoid additional performance impact when the check is not enabled.
> >
> > Pre-existing: Given that it is in an inline definition, the accuracy
> > of this comment seems questionable.
>
> Removed.
>
> >
> > ------------------------------------------------------------------------------
> >
> > Jiangli already pointed out that _open_archive_region_map is no longer
> > used.
> >
>
> All fixed in
> http://cr.openjdk.java.net/~tschatzl/8214277/webrev.0_to_1 (diff)
> http://cr.openjdk.java.net/~tschatzl/8214277/webrev.1/ (full)
>
> Thanks,
>    Thomas