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

[Stefan Karlsson]

Thanks, Per!

StefanK

On 2017-10-18 14:00, Per Liden wrote:
> Looks good!
> 
> /Per
> 
> On 2017-10-18 13:55, Stefan Karlsson wrote:
>> 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.
>>>
>>>