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

[Mikael Gerdin]

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   _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.
>
> Now.
>
> http://cr.openjdk.java.net/~tschatzl/8151386/webrev.3/ (full)
> http://cropenjdk.java.net/~tschatzl/8151386/webrev.2_to_3 (diff)
>

Looks good.

/Mikael

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