[prev in list] [next in list] [prev in thread] [next in thread] 

List:       openjdk-serviceability-dev
Subject:    Re: RFR (S) 8251302: Create dedicated OopStorages for Management and Jvmti
From:       Coleen Phillimore <coleen.phillimore () oracle ! com>
Date:       2020-08-11 11:28:01
Message-ID: 1ef431be-152c-1d7d-4390-c0a21594c8cc () oracle ! com
[Download RAW message or body]

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@oracle.com wrote:
> > > > > > > On 8/10/20 13:34, serguei.spitsyn@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
> > > > > > > > 
> > > > > > > 
> > > > > > 
> > > > > 
> > > > 
> > 


[prev in list] [next in list] [prev in thread] [next in thread]