RFR (M): 8201491: G1 support for java.lang.ref.Reference precleaning

[Thomas Schatzl]

Hi Sangheon,

On Thu, 2018-05-10 at 14:17 -0700, sangheon.kim at oracle.com wrote:
> Hi Thomas,
> 
> On 5/8/18 12:47 AM, Thomas Schatzl wrote:
> > Hi,
> > 
> > On Mon, 2018-05-07 at 21:08 -0400, Kim Barrett wrote:
> > > > On May 7, 2018, at 6:49 PM, Kim Barrett <kim.barrett at oracle.com
> > > > >
> > > > wrote:
> > > > 
> > > > > On Apr 26, 2018, at 5:36 AM, Thomas Schatzl <thomas.schatzl at o
> > > > > racl
> > > > > e.com> wrote:
> > > > > 
> > > > > Hi all,
> > > > > 
> > > > > can I have reviews for this change that implements reference
> > > > > precleaning like CMS for G1?
> > > > > 
[...]
> > There is a new webrev for a second reviewer at
> > http://cr.openjdk.java.net/~tschatzl/8201491/webrev.1 (full)
> > http://cr.openjdk.java.net/~tschatzl/8201491/webrev.0_to_1/ (diff
> > )
> 
> Webrev.1 looks good.
> But I have a couple of minor comments.
> 
> src/hotspot/share/gc/shared/referenceProcessor.cpp
> 1046         log_reflist("SoftRef abort: ", _discoveredSoftRefs, 
> _max_num_queues);
> 1063         log_reflist("WeakRef abort: ", _discoveredWeakRefs, 
> _max_num_queues);
> 1080         log_reflist("FinalRef abort: ", _discoveredFinalRefs, 
> _max_num_queues);
> 1097         log_reflist("PhantomRef abort: ",
> _discoveredPhantomRefs, 
> _max_num_queues);
> - You can ignore this. 'yield' seems better fit than 'abort' to me.

A return value of preclean_discovered_reflist() indeed indicates an
abort and not a yield.

> preclean_discovered_reflist
> 305   // Returns whether the operation should be aborted.
> - Same as above. 'yielded'?

The existing comment is correct.

The YieldClosure internally yields and only needs to return true if
yielding is not sufficient to progress, ie. there has been a full gc.

> src/hotspot/share/memory/iterator.hpp
>   322  virtual bool should_return() = 0;
>   323  // Yield on a fine-grain level. The check in case of not
> yielding 
> should be very fast.
>   324  virtual bool should_return_fine_grain() { return false; }
> - Looks like missing 1 space at line 322, 323, 324.
> 
> I don't need a new webrev for this.

Okay, I will fix this before pushing.

Thanks,
  Thomas