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

[Bengt Rutisson]

Hi Thomas,

Latest webrev looks good to me.

One very minor nit.
Line 133 in the new version of cardTableModRefBS.cpp has three spaces 
indentation. This was already like that, but since you have correctly 
indented the lines after it might be worth indenting this line correctly 
also. :)

Thanks,
Bengt


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/
>
> Thanks,
>    Thomas
>
>