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

[Kim Barrett]

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

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