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

Stefan Karlsson stefan.karlsson at oracle.com
Wed Oct 18 11:55:34 UTC 2017

Hi all,

Updated webrevs:

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

When running with CMS this executes the following code:

   void ParNewRefProcTaskExecutor::set_single_threaded_mode() {
     GenCollectedHeap* gch = GenCollectedHeap::heap();

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 

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


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.

More information about the hotspot-dev mailing list