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

Mikael Gerdin mikael.gerdin at oracle.com
Tue Mar 29 13:39:17 UTC 2016

Hi,

On 2016-03-29 15:38, Thomas Schatzl wrote:

Hi,

On Tue, 2016-03-29 at 15:06 +0200, Mikael Gerdin wrote: Hi Thomas,

I looked at webrev.2 but I'll reply here since you cut out the email context. On 2016-03-23 16:39, Thomas Schatzl wrote: 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/ contain s 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. I see, let's leave your change as is for now then. 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 :) No :) http://cr.openjdk.java.net/~tschatzl/8151386/webrev.2/src/share/vm/gc /g1/g1CardLiveData.cpp.html 71 cardsperregion = regionsize >> G1SATBCardTableModRefBS::cardshift; 72 Also, you are a bit inconsistent in using the G1CardLiveData::bmwordt typedef, if you want to keep it you should probably try to use it in more places, otherwise you should just remove it IMO. Now. http://cr.openjdk.java.net/~tschatzl/8151386/webrev.3/ (full) http://cropenjdk.java.net/~tschatzl/8151386/webrev.2to3 (diff)

Looks good.

/Mikael

Btw, in the meantime I did another jprt run with the changes (not including these particular ones).

Thomas

More information about the hotspot-gc-dev mailing list