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

[Tom Benson]

Hi Kim,
Thanks very much for the review.

On 8/4/2015 6:06 PM, Kim Barrett wrote:
> ------------------------------------------------------------------------------
> src/share/vm/gc/g1/g1CollectedHeap.cpp
> 1134     size_used += ranges[i].word_size() * HeapWordSize;
>
> Why not ranges[i].byte_size() ?

Indeed, why not?  I'll change it.

>
> ------------------------------------------------------------------------------
> 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.

I agree.  Leftover from the alloc_archive_regions code I started with.

>
> ------------------------------------------------------------------------------
> 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.

Both alloc_archive_regions and fill_archive_regions are written this way 
(the former for a slightly better reason), so I think I'll leave them 
all as-is.

>
> But, is next_region_in_heap really the right stepper function?

I see Thomas talked about this in his reply, but, yes.
>   It
> skips over regions that are not "is_available".  Are the regions we're
> dealing with in this function all guaranteed to be available?
This is basically part of an error path in mapping shared string space.  
We know the alloc_ succeeded, but the file mapping subsequently failed.
> 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.

OK.  I'll add a comment that the args should be the same as passed to 
alloc_archive_regions.
>
> ------------------------------------------------------------------------------
> 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?
>
> ------------------------------------------------------------------------------
>
I considered this, but didn't see much difference between "marking" a 
range as archive/non-archive and "setting" one.  However, I'll change it 
to "set" to avoid any possible confusion with "marking" in the normal GC 
sense.
Thanks,
Tom