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

[David Holmes]

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.

  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.

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?

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