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