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

[Thomas Schatzl]

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

>