RFR (XXL): 8038423: G1: Decommit memory within the heap
thomas.schatzl at oracle.com
Thu Aug 14 13:37:02 UTC 2014
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/
> 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.
> 55 G1RegionsLargerThanCommitSizeMapper(ReservedSpace rs, size_t
> 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.
> 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.
> 38 // exacuted after commit of a region already needs to do some re-
> initialization of
> Typo: should be "executed"
New webrev at:
Diff webrev at:
More information about the hotspot-gc-dev