RFR (S): 8131734: Add free_archive_regions support to G1 for -Xshared:auto

[Thomas Schatzl]

Hi,

On Tue, 2015-08-04 at 18:06 -0400, Kim Barrett wrote:
> On Aug 4, 2015, at 2:25 PM, Tom Benson <tom.benson at oracle.com> wrote:
> > 
> > Hi,
> > Please review this change for JDK-8131734, an addition to the Archive Region support in G1 to handle -Xshared:auto behavior.  With Xshared:auto, if the shared string region file mapping fails, sharing is disabled and execution continues.  To allow this, the archive region space which has already been allocated needs to be freed.   A free_archive_regions() entry is added to g1CollectedHeap, to be called if the file mapping performed by CDS fails.  Its arguments are the same as those given to alloc_archive_regions (and that would have been given to fill_archive_regions if the mapping had succeeded).
> > 
> > The CDS code change will be posted separately by Jiangli Zhou.
> > 
> > JBS: https://bugs.openjdk.java.net/browse/JDK-8131734
> > Webrev: http://cr.openjdk.java.net/~tbenson/8131734/webrev/
> > Tested: JPRT, and benchmarks run with alloc/free_archive_regions calls forced at init time and heap verification enabled.
> > Jiangli also tried this with the failing Xshared:auto test and the corresponding CDS code change to call free_archive_regions.
> 
> ----------------------------------

[...]
> 
> But, is next_region_in_heap really the right stepper function?  It
> skips over regions that are not "is_available".  Are the regions we're
> dealing with in this function all guaranteed to be available?

Yes. We assume that in the earlier call in allocate_containing_regions()
G1 made them available (committed them).

I assume that when mmap'ing it, the  

> Clearly, bad things happen if last_region is not available while using
> next_region_in_heap here.

One could do regular pointer arithmetic *within* the MemRegion (which is
always a guaranteed contiguous range) and then map the address back to
the HeapRegion*.

- I have some question about this code, particularly about the comment:

1137     HeapRegion* start_region = _hrm.addr_to_region(start_address);
1138     HeapRegion* last_region = _hrm.addr_to_region(last_address);
1139 
1140     // Check for ranges that start in the same G1 region in which the previous
1141     // range ended, and adjust the start address so we don't try to free
1142     // the same region again. If the current range is entirely within that
1143     // region, skip it.
1144     if ((prev_last_region != NULL) && (start_region == prev_last_region)) {
1145       start_address = start_region->end();
1146       if (start_address > last_address) {
1147         continue;
1148       }
1149       start_region = _hrm.addr_to_region(start_address);
1150     }

How could the situation mentioned in line 1140 happen? Are the given
memory regions not overlapping already, and the start addresses of these
MemRegions at the start of these regions?

Since last_region is the region containing the last address within the
memory range, wouldn't that mean given above preconditions, this could
not happen?

- I think the method should add a assert_at_safepoint(true) at the top
(and possibly all other archival methods if they are not yet through the
call chain), or decrease_used() made safe against concurrent
modification using the ParGC_Rare_Event_lock.

I would prefer just making sure the code is only run at a safepoint.

Thanks,
  Thomas