RFR (XL): 8151386: Extract card live data out of G1ConcurrentMark (original) (raw)

Thomas Schatzl thomas.schatzl at oracle.com
Wed Mar 23 15:39:25 UTC 2016

Hi Mikael,

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 G1CardLiveData::initialize 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 created)

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 point.

71 cardsperregion = regionsize >> G1SATBCardTableModRefBS::cardshift; Can you make this a division instead? There's no extra cost and it makes the code more obviously correct.

Done :)

g1CardLiveData.inline.pp const BitMap G1CardLiveData::livecardbitmap does return a const BitMap, but one that can be automatically converted to a non-const BitMap I suggest attempting to make the code const-correct for later.

Fixed.

bitMap.hpp/cpp I suppose that this will allow you to revert the change to BitMap::setintersection 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.

Done.

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.

g1RemSet.hpp G1CardLiveData curlivedata;

since there is only one of these the cur prefix seems a bit strange. cardlivedata 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

http://cr.openjdk.java.net/~tschatzl/8151386/webrev.1/ (full) http://cropenjdk.java.net/~tschatzl/8151386/webrev.0_to_1 (diff)

Thanks, Thomas

More information about the hotspot-gc-dev mailing list