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

[sangheon.kim at oracle.com]

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 oracl
>>>> e.com> wrote:
>>>>
>>>> Hi all,
>>>>
>>>> can I have reviews for this change that implements reference
>>>> precleaning like CMS for G1?
>>>>
>>>> […]
>>>> CR:
>>>> https://bugs.openjdk.java.net/browse/JDK-8201491
>>>> Webrev:
>>>> http://cr.openjdk.java.net/~tschatzl/8201491/webrev/
>>>> Testing:
>>>> hs-tier1-5, *many* times Kitchensink reference stress test
>>>>
>>>> Thanks,
>>>> Thomas
>>> Looks good.
>> I forgot a couple of things.  I don’t need a new webrev for these.
>>
>> -------------------------------------------------------------------
>> -----------
>> src/hotspot/share/gc/shared/referenceProcessor.hpp
>>   291   // Returns whether the operation should be aborted.
>>   292   void preclean_discovered_references(BoolObjectClosure*
>> is_alive,
>>
>> Comment is wrong.  It's preclean_discovered_reflist that returns the
>> abort flag.
>>
>> -------------------------------------------------------------------
>> -----------
>> src/hotspot/share/gc/g1/g1ConcurrentMarkThread.cpp
>>   313           if (G1UseReferencePrecleaning) {
>>   314             {
>>
>> The second level of brackets here isn't needed.
>>
>> -------------------------------------------------------------------
>>
>    thanks for your review.
>
> 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.

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

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.

Thanks,
Sangheon



>
> Thanks,
>    Thomas