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

[Per Liden]

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