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

[Tom Benson]

Hi Thomas,
Thanks for reviewing.

On 8/5/2015 9:37 AM, Thomas Schatzl wrote:
> Hi,
>
> On Tue, 2015-08-04 at 18:06 -0400, Kim Barrett wrote:
>> 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

missing end of sentence?   If you were going to say....  the mapping 
fails, so we need to free the archive regions that were just 
allocated..., then I agree.  8^)

>
>> 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 don't think this was a suggestion...   was it?

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

As you said in a subsequent message, this can happen if the G1 region 
size at dump time is smaller than region size at restore time.
> 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.

These restore-time routines (alloc_/free_ archive regions) are called at 
the beginning of JVM init, not at a safepoint.  The dump-time archive 
alloc routines (such as archive_mem_allocate) do check for safepoint 
context.
Thanks,
Tom

>
> Thanks,
>    Thomas
>
>
>