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

[Coleen Phillimore]

Thanks David!
Coleen

On 8/10/20 10:07 PM, David Holmes wrote:
> On 11/08/2020 10:48 am, Coleen Phillimore wrote:
>>
>> 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.
>
> Okay.
>
>>>
>>>  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.
>
> Okay.
>
> Thanks,
> David
>
>> 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
>>>>>>>>
>>>>>>>
>>>>>>
>>>>>
>>>>
>>