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

[Thomas Schatzl]

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