RFR (XL): 8151386: Extract card live data out of G1ConcurrentMark (original) (raw)
Thomas Schatzl thomas.schatzl at oracle.com
Fri Mar 18 13:36:36 UTC 2016
- Previous message (by thread): RFR(xs): 8152120: TLAB compute_size() should not allow any size larger than max_size
- Next message (by thread): RFR (XL): 8151386: Extract card live data out of G1ConcurrentMark
- Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]
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
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::_live_regions_bm) uses allocate_large_bitmap() 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 _live_regions_bm in G1CardLiveDataHelper:
" void set_bit_for_region(HeapRegion* hr) { BitMap::idx_t index = (BitMap::idx_t) hr->hrm_index(); _region_bm->par_at_put(index, true); }"
I'm not convinced that the _region_bm 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.
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
- Previous message (by thread): RFR(xs): 8152120: TLAB compute_size() should not allow any size larger than max_size
- Next message (by thread): RFR (XL): 8151386: Extract card live data out of G1ConcurrentMark
- Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]