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

Mikael Gerdin mikael.gerdin at oracle.com
Wed Mar 23 12:56:14 UTC 2016

Hi Thomas,

On 2016-03-18 14:36, Thomas Schatzl wrote:

Hi all,

can I have reviews for this (large) change that moves the "Liveness Count Data" from the g1ConcurrentMark files into their own class, and following that, cleaning it up? In JDK-8077144 the way we generate the per-card live data used for various liveness analysis (well, remembered set scrubbing for now) significantly. At the same time it showed that it would be a very good idea to move it out into a separate class, to be, in the future be able to improve the information it contains (JDK-8151846) and its representation more easily. This is the change that tries to do that exactly, followed by some cleanup. Improvements to the code: - add timing log output for related work where that has been missing - the new class name that contains this information has been changed to G1CardLiveData to indicate that it contains liveness information on a card basis. - the owner of G1CardLiveData is G1RemSet: at the moment only the remembered set uses it for scrubbing data. That may change in the future as necessary. - instead of passing around multiple bitmaps for scrubbing, only pass around that G1CardLiveData, which with its given API makes it nicer to use (imo). - fix the memory leak Kim critized in the very first review for 8077144. - tried to improve general naming etc. - incorporated lots of Erik H.'s cleanup changes he made for his review for 8077144 we talked about in private. A (consciously) remaining wart: - the region live bitmap (G1CardLiveData::liveregionsbm) uses allocatelargebitmap() to be allocated in virtual memory. This is kind of a waste of memory, as the region live bitmap is typically very small (a few thousand bits). However, the region live bitmap will be removed in the future, as it will be first replaced by a live card count in JDK-8151846 (the next RFR I have in the pipeline), and the coarse bitmap as its only user removed during rememberd set refactoring (JDK-8017163) reviews which will probably take one or two more RFRs. So I did not want to spend effort optimizing that. Similarly, in the review for 8077144 Mikael commented about the handling of the liveregionsbm in G1CardLiveDataHelper: " void setbitforregion(HeapRegion* hr) { BitMap::idxt index = (BitMap::idxt) hr->hrmindex(); regionbm->paratput(index, true); }" I'm not convinced that the regionbm even needs to be part of the "helper". Perhaps it would be cleaner if the helper class was only responsible for the card bitmap and the modification of the region bit map was moved elsewhere?" Again, the region live bitmap will go away soon, additionally moving that to somewhere else would likely mean the need to allow access to that method by more classes. I would like to avoid that, and make G1CardLiveData read-only from outside (in the public interface). CR: https://bugs.openjdk.java.net/browse/JDK-8151386 To ease review a little, I split this change into two parts, that I intend to push as a single change: first, trying to move the information out of G1ConcurrentMark, and second, try to brush up the code a little. 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)

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

g1CardLiveData.inline.pp const BitMap G1CardLiveData::live_card_bitmap 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.

bitMap.hpp/cpp 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.

g1RemSet.hpp 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?

/Mikael

This change is based on the changes for JDK-8077144. Testing: jprt multiple times (partially with other changes), vm.gc with -XX:+VerifyDuringGC specifically and with other changes. Some micro- and macro-benchmarks. Thanks a lot, Thomas

More information about the hotspot-gc-dev mailing list