RFR: 8189359: Move native weak oops cleaning out of ReferenceProcessor

[Stefan Karlsson]

Hi all,

Updated webrevs:
  http://cr.openjdk.java.net/~stefank/8189359/webrev.03.delta
  http://cr.openjdk.java.net/~stefank/8189359/webrev.03

Changes in the webrevs:
------------------------------------------------------------------------
I've added back all the missing evacuate followers closure to try to 
mimic the original code as much as possible.

This unveiled a bug for CMS with the original patch. I've re-added the 
following section to referenceProcessor.cpp:

   if (task_executor != NULL) {
     task_executor->set_single_threaded_mode();
   }

When running with CMS this executes the following code:

   void ParNewRefProcTaskExecutor::set_single_threaded_mode() {
     _state_set.flush();
     GenCollectedHeap* gch = GenCollectedHeap::heap();
     gch->save_marks();
   }

The missing call to GenCollectedHeap::save_marks() caused subsequent 
calls to the evacuate followers closure to assert that the same object 
were scanned twice.

------------------------------------------------------------------------
I also reverted to using PSKeepAliveClosure instead of 
PSScavengeRootsClosure in psScavenge.cpp.

------------------------------------------------------------------------
The comment I add for WeakProcessor::weak_oops_do previously stated that 
the function applied the "complete" closure after _each_ container had 
been processed. The next patch will move the call to 
JvmtiExport::weak_oops_do, and then the code wouldn't mimic the original 
code.

I've updated the comment to state that we only apply the "complete" 
closure once, after _all_ containers have been processed.

Thanks,
StefanK



On 2017-10-18 02:09, Kim Barrett wrote:
>> On Oct 17, 2017, at 5:38 PM, Per Liden <per.liden at oracle.com> wrote:
>>
>> Hi,
>>
>> On 2017-10-17 22:57, Stefan Karlsson wrote:
>> [...]
>>> Here are the updated webrevs:
>>>   http://cr.openjdk.java.net/~stefank/8189359/webrev.01.delta
>>>   http://cr.openjdk.java.net/~stefank/8189359/webrev.01
>>
>> Looks good. Just two comments.
>>
>> share/gc/parallel/psScavenge.cpp:
>>
>> 446     {
>> 447       GCTraceTime(Debug, gc, phases) tm("Weak Processing", &_gc_timer);
>> 448       WeakProcessor::weak_oops_do(&_is_alive_closure, &root_closure);
>> 449     }
>>
>> I see you've kept the "complete" closure in WeakProcessor::weak_oops_do(), which is fine and we can clean that out later, but here you don't seem to mimic exactly what the old code did. I think you want to pass in &evac_followers here, right?
>>
>> share/gc/serial/defNewGeneration.cpp:
>>
>> 662   WeakProcessor::weak_oops_do(&is_alive, &keep_alive);
>>
>> Same here, pass in &evacuate_followers?
>>
>> I don't need to see a new webrev.
>>
>> cheers,
>> Per
> 
> Oh, I missed that.  Same thing in cms/parNewGeneration.cpp, I think.
> 
> Otherwise, looks good.
> 
> I don’t need a new webrev either.
> 
>