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

[David Holmes]

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