RFR (S) 8251302: Create dedicated OopStorages for Management and Jvmti

[Coleen Phillimore]

Hi David,  Thank you for reviewing.

On 8/10/20 8:04 PM, David Holmes wrote:
> Hi Coleen,
>
> This looks good to me too!
>
> Were the changes in src/hotspot/share/utilities/macros.hpp just for 
> completeness? You don't seem to use the new macro.

It was left over from an earlier edit.  I reverted it.
>
>  src/hotspot/share/runtime/init.cpp
>
> It seems a little odd having an explicit call to 
> JvmtiExport::initialize_oop_storage() rather than that call being 
> inside on of the other init functions. But I don't really see an 
> appropriate place for it. I thought perhaps management_init as it 
> seems to combine a few related things, but it isn't really the right 
> place either.

management_init() isn't the right place for JVMTI.   I could have added 
a jvmti_init() that calls JvmtiExport::initialize_oop_storage() but 
honestly I think this whole thing should be rewritten to call static 
functions rather than through these forward declarations.  There are 
other places that call qualified static initialization functions in this 
code and I think this should migrate to that.

> I was also a little unsure if this initialization point would always 
> be early enough, but it seems the oop-storage can't be touched by 
> anything until the live phase, so this seems okay. Though I was 
> wondering whether it should be done in vm_init_globals after 
> universe_oopstorage_init, to maintain the same initialization point as 
> it currnetly has?

There are no jvmti specific initializations in the short amount of 
initialization code between vm_init_globals and init_globals so I chose 
the later initialization because it worked, and the later initialization 
risks dragging more dependencies forward with it. Doing jvmti 
initialization in what looks like very early initialization doesn't seem 
appropriate.  And isn't needed for correctness.

thanks,
Coleen
>
> Thanks,
> David
> -----
>
>
> On 11/08/2020 8:39 am, Coleen Phillimore wrote:
>> Adding back serviceability-dev.
>> Thanks for reviewing Serguei.
>> Coleen
>>
>> On 8/10/20 6:11 PM, Coleen Phillimore wrote:
>>>
>>>
>>> On 8/10/20 5:28 PM, Coleen Phillimore wrote:
>>>>
>>>>
>>>> On 8/10/20 4:38 PM, serguei.spitsyn at oracle.com wrote:
>>>>> On 8/10/20 13:34, serguei.spitsyn at oracle.com wrote:
>>>>>> Hi Coleen,
>>>>>>
>>>>>> It looks good to me.
>>>>>> Minor:
>>>>>> +void JvmtiExport::initialize_oop_storage() {
>>>>>> + // OopStorage needs to be created early in startup and 
>>>>>> unconditionally
>>>>>> + // because of OopStorageSet static array indices.
>>>>>> + _jvmti_oop_storage = OopStorageSet::create_strong("Thread 
>>>>>> OopStorage");
>>>>>> +}
>>>>>> +
>>>>>> Would it better to replace "Thread Oopstorage" with "JVMTI 
>>>>>> OopStorage"?
>>>>>
>>>>> In the file
>>>>> http://cr.openjdk.java.net/~coleenp/2020/8251302.01/webrev/test/jdk/jdk/jfr/event/gc/collection/TestG1ParallelPhases.java.udiff.html 
>>>>>
>>>>> I see this:
>>>>>               "Thread OopStorage",
>>>>> + "ThreadService OopStorage",
>>>>> It is not clear if we can simply add ""JVMTI OopStorage" above.
>>>>
>>>> Serguei,  Thank you for finding this.  I was wondering why I didn't 
>>>> have to add JVMTI OopStorage to the test.  I'd cut/pasted the same 
>>>> string for Thread OopStorage.
>>>>
>>>> I'll fix this and the test and retest.
>>>
>>> Hi Serguei,
>>>
>>> open webrev at 
>>> http://cr.openjdk.java.net/~coleenp/2020/8251302.02.incr/webrev
>>>
>>> This fixes the test as well.
>>>
>>> Thanks!
>>> Coleen
>>>
>>>>
>>>> thanks,
>>>> Coleen
>>>>>
>>>>> Thanks,
>>>>> Serguei
>>>>>
>>>>>
>>>>>> No need in another webrev.
>>>>>>
>>>>>> Thanks,
>>>>>> Serguei
>>>>>>
>>>>>> On 8/10/20 12:37, Coleen Phillimore wrote:
>>>>>>> These OopHandles are created and released during breakpoints and 
>>>>>>> Thread stack walking operations.  They should have their own 
>>>>>>> OopStorage so that GC can detect whether these things affect 
>>>>>>> timing.
>>>>>>>
>>>>>>> Tested with tier1-6.
>>>>>>>
>>>>>>> open webrev at 
>>>>>>> http://cr.openjdk.java.net/~coleenp/2020/8251302.01/webrev
>>>>>>> bug link https://bugs.openjdk.java.net/browse/JDK-8251302
>>>>>>>
>>>>>>> Thanks,
>>>>>>> Coleen
>>>>>>
>>>>>
>>>>
>>>
>>