RFR: 8227054: ServiceThread needs to know about all OopStorage objects

[Erik Osterlund]

Hi Per,

> On 2 Aug 2019, at 17:09, Per Liden <per.liden at oracle.com> wrote:
> 
> Thanks a lot Erik, for taking the time to do this proposal. This looks a lot more straight forward and sane to me.

Glad you liked it!

> src/hotspot/share/runtime/jniHandles.cpp
> ----------------------------------------
> 186 void JNIHandles::weak_oops_do(OopClosure* f) {
> 187   OopStorageSet::weak_jni();
> 188 }
> 
> Missing "->weak_oops_do(f)" there.

Nice catch. Looks like that function is dead code. Will fix.

> Other than than that, I'd be happy with this proposal.

Thanks for having a look, and glad you liked it.

/Erik

> cheers,
> Per
> 
>> Thanks,
>> /Erik
>> On 2019-07-26 22:39, Kim Barrett wrote:
>>>> On Jul 26, 2019, at 10:38 AM, coleen.phillimore at oracle.com wrote:
>>>> 
>>>> 
>>>> I like this change!
>>>> 
>>>> http://cr.openjdk.java.net/~kbarrett/8227054/open.00/src/hotspot/share/gc/shenandoah/shenandoahRootProcessor.inline.hpp.udiff.html 
>>>> 
>>>> Can you remove some #include directives from the GC code since the oop storages are coming from oopStorages?
>>>> 
>>>> http://cr.openjdk.java.net/~kbarrett/8227054/open.00/src/hotspot/share/prims/resolvedMethodTable.cpp.udiff.html 
>>>> 
>>>> Does this still need to #include oopStorage.inline.hpp ?
>>>> 
>>>> http://cr.openjdk.java.net/~kbarrett/8227054/open.00/src/hotspot/share/prims/resolvedMethodTable.hpp.frames.html 
>>>> 
>>>> This doesn't seem to need to include oopStorage.hpp.  Might be others too.
>>> Thanks for suggesting some #include trimming.  I did some cleanup.
>>> 
>>>> http://cr.openjdk.java.net/~kbarrett/8227054/open.00/src/hotspot/share/gc/shared/oopStorages.hpp.html 
>>>> 
>>>> I thought our compilers were tolerant of a trailing comma in enumerations?
>>> Solaris Studio doesn't like trailing commas in enums.
>>> 
>>>> The macros aren't bad though.  It seems like it would be easy to add a new OopStorage if we wanted to, but it would be better to use an existing vm weak or vm strong oop storage, if we wanted to move more oops into an oop storage (say for JFR).
>>> Right.  Also easy to remove one.  (Someone told me there are ideas
>>> afloat that might remove the need for the ResolvedMethodTableWeak
>>> storage object.)
>>> 
>>> I don't expect the set to be changing frequently.  But it's a bit of a
>>> pain to track down all the right places.  (Even still, because the GCs
>>> still have explicit knowledge of the full set, though this change
>>> provides tools to address that.)
>>> 
>>>> This change looks great apart from trying to remove more #includes.
>>> Thanks.
>>> 
>>> New webrevs:
>>> full: http://cr.openjdk.java.net/~kbarrett/8227054/open.01/
>>> incr: http://cr.openjdk.java.net/~kbarrett/8227054/open.01.inc/
>>> 
>>>