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

[Mikael Gerdin]

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.


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.


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: " ...)

g1BlockOffsetTable.cpp

  38   // exacuted after commit of a region already needs to do some re-
initialization of

Typo: should be "executed"

/Mikael

> 
> Testing:
> jprt, aurora (nightly, bigapps, lots of test suites) with -XX:+UseG1GC,
> crm fuse
> 
> Thanks,
>   Thomas