RFR (XL): 8151386: Extract card live data out of G1ConcurrentMark
thomas.schatzl at oracle.com
Wed Mar 23 15:39:25 UTC 2016
thanks for your review.
On Wed, 2016-03-23 at 13:56 +0100, Mikael Gerdin wrote:
> Hi Thomas,
> On 2016-03-18 14:36, Thomas Schatzl wrote:
> > Hi all,
> > [...]
> > http://cr.openjdk.java.net/~tschatzl/8151386/webrev.0a/ contains
> > the
> > "move" with adaptations to make it work. (Note that I know that
> > this
> > change alone leaks memory with -XX:+VerifyDuringGC as the temporary
> > bitmaps used there are not freed)
> > http://cr.openjdk.java.net/~tschatzl/8151386/webrev.0b/ is the
> > change
> > that tries to improve the code quality and fixes the mentioned
> > issue.
> > http://cr.openjdk.java.net/~tschatzl/8151386/webrev.0/ contains the
> > full change.
> in g1CardLiveData.cpp
> I don't think card live data should recompute the region size.
> Ff you are feeling paranoid about the heap region size then
> keep some asserts and use HeapRegion::GrainBytes (it's set up ac the
> construction of G1CollectorPolicy, which is before G1CollectedHeap is
For unit-testing the remembered set later (scrubbing tests) I need to
be completely independent of global variables with this data structure.
Ideally there would be a global "G1HeapSettings" class that collects
these that is used everywhere and could be referenced, but this is not
the case right now, and is a huge change.
Even if that were not a problem, for testing it is less resource
intensive to create smaller than usually available heaps/regions.
I can change this now, but will need to add something like this soon
anyway. If you want I can remove it for now, but I kind of not see the
> 71 _cards_per_region = region_size >>
> Can you make this a division instead? There's no extra cost and it
> makes the code more obviously correct.
> const BitMap G1CardLiveData::live_card_bitmap
> does return a const BitMap, but one that can be automatically
> to a non-const BitMap
> I suggest attempting to make the code const-correct for later.
> I suppose that this will allow you to revert the change to
> BitMap::set_intersection as well.
> While it would be nice if all of those methods had a const& other I
> think that's part of a larger redesign.
> I don't like that you are making BitMap::map public
> I'd prefer if g1CardLiveData managed the memory backing the BitMap
> explicitly. I realize that it's not a particularly attractive
> solution but I don't think BitMap should expose this just for the
> sake of one user.
CardLiveData now tracks the BitMap in their components (pointer + size)
and constructs the BitMap on the fly as needed.
> G1CardLiveData _cur_live_data;
> since there is only one of these the _cur prefix seems a bit strange.
> _card_live_data would perhaps be more descriptive?
Later, when the card live data is used for fully coarsened we need to
maintain a prev/next card live data to support this while the new card
live data is created. So we need a "current" one that is used for
remembered set scan.
This is a remnant of that change. I will fix this for now.
New webrev at
More information about the hotspot-gc-dev