RFR (S): 8153170: Card Live Data does not correctly handle eager reclaim

[Derek White]

Hi Thomas,

On 4/14/16 6:08 AM, Thomas Schatzl wrote:
> Hi Kim,
>
>    thanks for your review.
>
> On Thu, 2016-04-14 at 00:31 -0400, Kim Barrett wrote:
>>> On Apr 13, 2016, at 11:42 AM, Thomas Schatzl <
>>> thomas.schatzl at oracle.com> wrote:
>>>
>>> Hi all,
>>>
>>>
> [...]
>>> CR:
>>> https://bugs.openjdk.java.net/browse/JDK-8153170
>>> Webrev:
>>> http://cr.openjdk.java.net/~tschatzl/8153170/webrev/
>>> Testing:
>>> jprt, vm.gc
>> I think this looks ok.
>>
>> My one concern is that I'm having trouble figuring out when a heap
>> region might have reset_gc_time_stamp called on it.  Can that happen
>> in such a way that it would interfere with the use of the time_stamp
>> being added by these changes?  I will continue to study, but maybe
>> you already know and can provide guidance.
> I do not think so.
>
> Global reset_gc_time_stamp() (reset_gc_time_stamp() called on
> G1CollectedHeap and all regions) is called during full gc (which aborts
> getting live data anyway), and at the end of the cleanup pause, after
> finalizing live data.
>
> So the reset of the gc time stamps should be safe and not interfere
> with this use.
>
>> Also a couple of minor things:
>>
>> ---------------------------------------------------------------------
>> ---------
>> src/share/vm/gc/g1/g1CardLiveData.hpp
>>    49   // While concurrently creating live data regions may be
>> reclaimed (e.g. humongous
>>
>> I keep having a hard time reading this because I see
>>
>>    "live data regions"
>>
>> and wondering what those might be.  How about
>>
>>    Regions may be reclaimed while concurrently creating live data
>>
>> ---------------------------------------------------------------------
>> ---------
>> src/share/vm/gc/g1/g1CardLiveData.cpp
>> Several logging statements added, but this file doesn't #include
>> logging.
>>
> ---------
> Now we know why the previous CR had a superfluous #include right
> there...
>
> Thanks a lot for catching these.
>
> New webrevs at
> http://cr.openjdk.java.net/~tschatzl/8153170/webrev.0_to_1/ (diff)
> http://cr.openjdk.java.net/~tschatzl/8153170/webrev.1/ (full)
>
> Thanks,
>    Thomas

This looks good. A couple of minor items.

g1CardLiveData.cpp:

- Line 488 vs. (496, 512).
The comment at 488 doesn't match the code. The comment talks about the 
old (expected && !actual) condition. Is (!expected && actual) somehow an 
OK condition or a failure? Or is the change part of having more exact 
card liveness information?

heapRegion.hpp
   - Copyright.

I don't need to see an updated webrev though.
  - Derek
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://mail.openjdk.org/pipermail/hotspot-gc-dev/attachments/20160414/d5f31d65/attachment.htm>