RFR (XXL): 8038423: G1: Decommit memory within the heap

[Mikael Gerdin]

Hi Thomas,

On 2014-08-14 15:37, Thomas Schatzl wrote:
> Hi Mikael,
>
>    thanks for the review :)
>
> On Thu, 2014-08-14 at 11:39 +0200, Mikael Gerdin wrote:
>> Hi Thomas,
>>
>> On Tuesday 12 August 2014 17.12.07 Thomas Schatzl wrote:
>>> Hi all,
>>>
>>>    can I have reviews for this change? It implements the capability for
>>> G1 to shrink/expand the heap on a per-region basis without the
>>> constraint that shrinking/expansion needs to occur at the highest
>>> address of the reserved space.
>>>
>>> Further it implements automatic commit/uncommit of all (relevant) large
>>> heap data structures (auxiliary data like block offset table, card
>>> table, mark bitmaps, hot card cache) at the same time the corresponding
>>> heap region is (un-)committed.
>>>
>>> This change implements basic support for this new behavior: selection of
>>> regions to (un-)commit is not particularly sophisticated.
>>>
>>> For easier reviewing, some notes:
>>> - start at the new initialization code in G1CollectedHeap::initialize()
>>> - all memory commit/uncommit activity of auxiliary data is tied to
>>> regions: the new G1RegionToSpaceMapper class manages translation between
>>> regions and whatever granularity the auxiliary data is
>>> committed/uncommitted.
>>> - these G1RegionToSpaceMappers are managed by HeapRegionSeq.
>>>
>>> This change is based on the review for JDK-8054818. As mentioned there,
>>> there will be some cleanup CRs after this change: at least a CR to
>>> rename HeapRegionSeq to HeapRegionManager, and another one that cleans
>>> up the card table code.
>>>
>>> They were split out for easier review, as they are quite large too but
>>> do not contribute to the functionality.
>>>
>>> CR:
>>> https://bugs.openjdk.java.net/browse/JDK-8038423
>>>
>>> Webrev:
>>> http://cr.openjdk.java.net/~tschatzl/8038423/webrev/
>>
>> g1CollectedHeap.cpp
>>
>> 2006   g1_barrier_set()->initialize(cardtable_storage);
>>
>> Can this initialization be moved to below all the create_mapper calls?
>> It's easy to miss it in all the similar calls there.
>
> Fixed.
>
>> g1RegionToSpaceMapper.cpp
>>
>>    55   G1RegionsLargerThanCommitSizeMapper(ReservedSpace rs, size_t
>> os_commit_granularity,
>>    56     size_t alloc_granularity, size_t commit_factor, MemoryType type) :
>>    57      G1RegionToSpaceMapper(rs, os_commit_granularity, alloc_granular
>>
>> Please line up the parameters like you did in the SmallerThan variant.
>>
>>    94  public:
>>    95   G1RegionsSmallerThanCommitSizeMapper(ReservedSpace rs,
>>    96                                         size_t os_commit_granularity,
>>
>> The rest of the parameters are not aligned with the ReservedSpace one.
>
> Fixed. I also did more alignment.
>
>> g1BlockOffsetTable.inline.hpp
>>
>>   50 #define check_index(index, msg)
>> \
>>    51   assert((index) < (_reserved.word_size() >> LogN_words),
>> \
>>    52          err_msg("%s - "
>> \
>>    53                  "index: " SIZE_FORMAT ", _vs.committed_size: "
>> SIZE_FORMAT,   \
>>    54                  msg, (index), (_reserved.word_size() >> LogN_words)));
>> \
>>
>> You should be able to do string concat on msg in the macro instead of using %s
>> in err_msg, something like
>> err_msg(msg  " - index: " ...)
>>
>
> No, that does not work. I kept it, but realigned the lines in the macro.
>
>> g1BlockOffsetTable.cpp
>
>>
>>    38   // exacuted after commit of a region already needs to do some re-
>> initialization of
>>
>> Typo: should be "executed"
>
> Done.
>
> New webrev at:
> http://cr.openjdk.java.net/~tschatzl/8038423/webrev.1/
> Diff webrev at:
> http://cr.openjdk.java.net/~tschatzl/8038423/webrev.0_to_1/

Looks good.

/Mikael

>
> Thanks,
>    Thomas
>
>