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

[Mikael Gerdin]

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::_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.

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
>