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

[Kim Barrett]

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.

------------------------------------------------------------------------------ 
src/share/vm/gc/g1/g1CollectedHeap.cpp  
1134     size_used += ranges[i].word_size() * HeapWordSize;

Why not ranges[i].byte_size() ?

------------------------------------------------------------------------------ 
src/share/vm/gc/g1/g1CollectedHeap.cpp  
1144     if ((prev_last_region != NULL) && (start_region == prev_last_region)) {

I think (prev_last_region != NULL) is unnecessary here.

------------------------------------------------------------------------------
src/share/vm/gc/g1/g1CollectedHeap.cpp 
1155     HeapRegion* curr_region = start_region;
1156     while (curr_region != NULL) {
...
1162       if (curr_region != last_region) {
1163         curr_region = _hrm.next_region_in_heap(curr_region);
1164       } else {
1165         curr_region = NULL;
1166       }
1167     }

I think I would have found this easier to read if structured something
like:

  while (true) {
    ...
    if (curr_region == last_region) {
      break;
    } else {
      curr_region = _hrm.next_region_in_heap(curr_region);
      assert(curr_region != NULL, ...);
    }
  }

I’ll leave that up to you.

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?
Clearly, bad things happen if last_region is not available while using
next_region_in_heap here.

------------------------------------------------------------------------------ 
src/share/vm/gc/g1/g1CollectedHeap.hpp
 789   // For each of the specified MemRegions, free the containing G1 regions 
 790   // which had been allocated by alloc_archive_regions. This should be called
 791   // rather than fill_archive_regions at JVM init time if the archive file 
 792   // mapping failed.
 793   void free_archive_regions(MemRegion* range, size_t count);

The implementation presently requires the ranges to be non-overlapping
and sorted in increasing order.  That ought to be said in the
description.

------------------------------------------------------------------------------
src/share/vm/gc/g1/g1MarkSweep.hpp
  61   // Mark or un-mark the regions containing the specified address range as archives.
  62   static void mark_range_archive(MemRegion range, bool is_archive);

With the functionality change, maybe the name should be changed.
Perhaps set_range_archive?

------------------------------------------------------------------------------