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

[Mikael Gerdin]

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

I see, let's leave your change as is for now then.

>
>>     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.
>
> Done :)

No :)

http://cr.openjdk.java.net/~tschatzl/8151386/webrev.2/src/share/vm/gc/g1/g1CardLiveData.cpp.html

71 _cards_per_region = region_size >> G1SATBCardTableModRefBS::card_shift;
72

Also, you are a bit inconsistent in using the G1CardLiveData::bm_word_t 
typedef, if you want to keep it you should probably try to use it in 
more places, otherwise you should just remove it IMO.

/Mikael


>
>>
>> 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.
>
> Fixed.
>
>>
>> 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.
>
> 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 _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?
>
> 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
>