RFR: 8202813: Move vm_weak processing from SystemDictionary::do_unloading to WeakProcessor

[Kim Barrett]

> On May 17, 2018, at 3:13 AM, Stefan Johansson <stefan.johansson at oracle.com> wrote:
> 
> 
> 
> On 2018-05-17 01:51, Kim Barrett wrote:
>>> On May 16, 2018, at 8:42 AM, Stefan Johansson <stefan.johansson at oracle.com> wrote:
>>> 
>>> Hi Kim,
>>> 
>>> On 2018-05-08 21:34, Kim Barrett wrote:
>>>> Please review this change to move the clearing of dead entries in
>>>> vm_weak_oop_storage from SystemDictionary::do_unloading to preceeding
>>>> calls to WeakProcessor::weak_oops_do.
>>>> This involves conditionalizing WeakProcessor invocations, to allow
>>>> different calls to process different subsets of the native weak
>>>> references.  For example, vm_weak_oop_storage is treated as strong
>>>> rather than weak when ClassUnloading is disabled.
>>>> CR:
>>>> https://bugs.openjdk.java.net/browse/JDK-8202813
>>>> Webrev:
>>>> http://cr.openjdk.java.net/~kbarrett/8202813/open.00/
>>> Thanks for doing this work to unify weak root processing.
>> Thanks for looking at it.
>>> I might be missing the bigger picture, so before starting to commenting the code I have a couple of questions:
>>> 1. With this patch SystemDictionary::vm_weak_oop_storage is now processed both by the WeakProcessor in weak_oops_do and the SystemDictionary in oops_do() and roots_oops_do(). Shouldn't all processing be moved to the WeakProcessor to make it clear who is responsible for doing the processing? Or why do we want to treat do_unloading special?
>> Yes, it should be moved completely.  Baby steps.
>> I started to change (remove, actually, and update callers)
>> SystemDictionary::oops_do, but discovered jfr was using it, and didn't
>> want to touch that in the middle of the jfr review.
>> And there are some preliminary cleanups I'm working on around
>> roots_oops_do, but haven't finished them yet.
> I agree that baby steps is often good, but now when JFR is in would it make sense to include more in this change?

OK, I can do that.

> 
>>> 2. For this patch I don't see the need for the PhaseSet class and the serial and parallel phases distinction. Couldn't we just pass in a bool to weak_oops_do saying if we should handle the vm_weak_oop_storage or not? I understand that for upcoming patches this might be needed, but then I think this should be added in that change.
>> I agree it's over-engineered for the needs of this change set.
>> However, further conditionalization beyond a boolean for
>> class-unloading is going to be needed RSN for the new StringTable
>> (based on the new concurrent hash table + oopstorage). StringTable
>> processing is another place where the various GCs and their sub-parts
>> vary about where it's treated as strong or weak. That should be
>> showing up shortly, and is being worked on by folks who aren't me. So
>> unless you want me to separate that out into a separate change that
>> will likely be my next one to support that work...
> I would like it to be added when needed, to make sure we only include the parts we really need. Right now it would be hard for me to review those parts since I can't verify that they work as intended. I'm mostly concerned about the parts around serial/parallel phases, which is currently unused.

Since you and (privately) Coleen are both complaining about this, I’ll reconsider.

Withdrawing this change for now.