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

[sangheon.kim at oracle.com]

Hi Thomas,

On 5/11/18 6:47 AM, Thomas Schatzl wrote:
> 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.
For G1, yes.

But looking at the description of preclean_discovered_references(), it 
says "incrementalize" or "abort".
And G1 is one of users of ReferenceProcessor class, so I think general 
comment seems better.
  285   // The first argument is a predicate on an oop that indicates
  286   // its (strong) reachability and the fourth is a closure that
  287   // may be used to incrementalize or abort the precleaning process.

I am okay to proceed as is. But wanted to remind it.

Thanks,
Sangheon


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