Re: RFR (M) Close alignment gaps in InstanceKlass
Hi Dean, Thank you for looking at the JVMCI changes and the suggestion to add the test. I did this and found a bug. The new test is quite limited because there's no good test to see if a source file name can assertNotNull(type.getSourceFileName()), so I couldn't iterate through the list of loaded classes like the other tests in that file. http://cr.openjdk.java.net/~coleenp/2020/8238048.02.incr/webrev/index.html Thanks, Coleen On 4/21/20 9:51 PM, Dean Long wrote: Hi Coleen. The JVMCI changes look OK. It looks like there is a Graal unittest that covers getSourceFileName, but those tests don't always get run. If it's not too much trouble, could you look into enabling getSourceFileName() testing in test/hotspot/jtreg/compiler/jvmci/jdk.vm.ci.runtime.test/src/jdk/vm/ci/runtime/test/TestResolvedJavaType.java ? It's currently on the "untested" list. thanks, dl On 4/21/20 1:12 PM, coleen.phillim...@oracle.com wrote: Summary: moved fields around and some constant fields into ConstantPool This is a simple change except that I moved some constant fields from InstanceKlass into the constant pool so they can be shared read-only in the CDS archive. There are associated repercussions in SA and JVMCI, so please look at these changes. Also moved similarly sized fields together in the class so there's less likelihood of introducing gaps in future InstanceKlass changes. InstanceKlass is reduced from 544 to 520 bytes in a simple Hello World class. open webrev at http://cr.openjdk.java.net/~coleenp/2020/8238048.01/webrev bug link https://bugs.openjdk.java.net/browse/JDK-8238048 Tested with tier1-6. Thanks, Coleen
Re: RFR: 8242425: JVMTI monitor operations should use Thread-Local Handshakes
Hi Chris,
I filed it to JBS:
https://bugs.openjdk.java.net/browse/JDK-8243450
I will send review request later.
Thanks,
Yasumasa
On 2020/04/23 5:02, Chris Plummer wrote:
Hi Yasumasa,
Yes, it looks like VMOps in SA can be removed. It's probably bit rotted code.
My guess is at some point there was support, or were plans for supporting, the
debugging of VMOps from within SA. Given that there are no references to the
VMOps class, it looks like none of that support currently exists.
thanks,
Chris
On 4/20/20 5:24 PM, Yasumasa Suenaga wrote:
Hi Rechard,
Thank you for telling it to me.
I grep'ed jdk.hotspot.agent with VMOps (it is just enum), it does not seem to
be used.
In addition, VMOps has not already synced to HotSpot. For example,
VM_HandshakeOneThread does not appear in VMOps.
(I wonder how do we use VMOps in SA)
Thus I think we can (should) remove VMOps from jdk.hotspot.agent .
If other serviceability folks agree with it, I will file it to JBS.
Thanks,
Yasumasa
On 2020/04/21 0:02, Reingruber, Richard wrote:
Hi Yasumasa,
I'm obviously late to this review. Still I wanted to let you know, that there's
another file in the
source tree that lists the VM operations in the VM (just found out about it
myself):
src/jdk.hotspot.agent/share/classes/sun/jvm/hotspot/runtime/VMOps.java
Probably you should remove the VM operations from that file too.
It seems to be part of the serviceability agent, which I hardly know. Would be
good if somebody
who is more familiar with it could let us know...
Best regards,
Richard.
-Original Message-
From: serviceability-dev On
Behalf Of Yasumasa Suenaga
Sent: Montag, 20. April 2020 02:33
To: serviceability-dev@openjdk.java.net
Cc: yasue...@gmail.com
Subject: Re: RFR: 8242425: JVMTI monitor operations should use Thread-Local
Handshakes
Hi all,
Could you review it?
JBS: https://bugs.openjdk.java.net/browse/JDK-8242425
webrev: http://cr.openjdk.java.net/~ysuenaga/JDK-8242425/webrev.02/
I need one more reviewer to push.
Thanks,
Yasumasa
On 2020/04/17 5:13, serguei.spit...@oracle.com wrote:
Hi Yasumasa,
Thank you for the update.
It looks good.
Thanks,
Serguei
On 4/10/20 04:30, Yasumasa Suenaga wrote:
Hi Serguei,
I use current_jt in this webrev. Could you review again?
http://cr.openjdk.java.net/~ysuenaga/JDK-8242425/webrev.02/
I tested this change with vmTestbase/nsk/jvmti, they are fine on my Linux x64.
Thanks,
Yasumasa
On 2020/04/10 17:21, serguei.spit...@oracle.com wrote:
Hi Yasumasa,
Thank you for the update.
Minor:
http://cr.openjdk.java.net/~ysuenaga/JDK-8242425/webrev.01/src/hotspot/share/prims/jvmtiEnvBase.cpp.udiff.html
+ err = get_locked_objects_in_frame(JavaThread::current(), java_thread, jvf,
owned_monitors_list, depth-1); . . .
+ JvmtiMonitorClosure jmc(java_thread, JavaThread::current(),
owned_monitors_list, this);
You can use current_jt instead of JavaThread::current() above.
There is also some test coverage in the vmTestbase/nsk/jvmti/scenarios tests.
This one as well:
test/hotspot/jtreg/vmTestbase/nsk/jvmti/unit/functions/nosuspendMonitorInfo/JvmtiTest
Probably it is easier to run all vmTestbase/nsk/jvmti tests.
Thanks,
Serguei
On 4/10/20 01:05, Yasumasa Suenaga wrote:
Hi Serguei,
Thanks for your comment!
I uploaded new webrev:
http://cr.openjdk.java.net/~ysuenaga/JDK-8242425/webrev.01/
I ran following tests, and all of them were passed on my Linux x64.
- vmTestbase/nsk/jvmti/GetCurrentContendedMonitor
- vmTestbase/nsk/jvmti/GetOwnedMonitorInfo
- vmTestbase/nsk/jdi
- vmTestbase/nsk/jdwp
- serviceability/jvmti/GetOwnedMonitorInfo
- serviceability/jvmti/GetOwnedMonitorStackDepthInfo
- serviceability/jdwp
Thanks,
Yasumasa
On 2020/04/10 14:50, serguei.spit...@oracle.com wrote:
Hi Yasumasa,
It looks pretty good in general.
A couple of comments though.
http://cr.openjdk.java.net/~ysuenaga/JDK-8242425/webrev.00/src/hotspot/share/prims/jvmtiEnvBase.cpp.frames.html
650 JvmtiEnvBase::get_current_contended_monitor(JavaThread *java_thread,
jobject *monitor_ptr) {
651 assert(!Thread::current()->is_VM_thread() &&
652 (Thread::current() == java_thread ||
653 Thread::current() == java_thread->active_handshaker()),
654 "call by myself or at direct handshake");
...
685 JvmtiEnvBase::get_owned_monitors(JavaThread* java_thread,
686 GrowableArray *owned_monitors_list) {
687 jvmtiError err = JVMTI_ERROR_NONE;
688 assert(!Thread::current()->is_VM_thread() &&
689 (Thread::current() == java_thread ||
690 Thread::current() == java_thread->active_handshaker()),
691 "call by myself or at direct handshake");
I'd suggest to add this at the beginning:
JavaThread *current_jt = JavaThread::current();
676 JavaThread *current_jt = reinterpret_cast(JavaThread::current());
There is not need in reinterpret_cast. Also, you can use the
current_jt defined above.
Also, please, removed these two definitions as they became unused with your fix:
Re: RFR (M) Close alignment gaps in InstanceKlass
On 4/22/20 9:00 PM, Dean Long wrote:
It looks like calling the JVMCI getSourceFileName() on an array would
have accessed random memory because it was expecting an
InstanceKlass. Instead of returning null we might want to throw an
exception like in HotSpotResolvedPrimitiveType.
It was never called, except when I tried to call it on an array in the
test. It caused an assert in the hotspot code. How about this?
Something else in that file throws JVMCIError with a similar message.
public String getSourceFileName() {
if (isArray()) {
throw new JVMCIError("Cannot call getSourceFileName() on an
array klass type: %s", this);
}
return getConstantPool().getSourceFileName();
}
dl
On 4/22/20 5:39 PM, Dean Long wrote:
Can you compare the result to some string, like "Object.java"? That
seems to be what HotSpotResolvedObjectTypeTest.java is doing.
Also, did getSourceFileName() return null for arrays before your change?
Fixed:
public void getSourceFileNameTest() {
Class c = Object.class;
ResolvedJavaType type = metaAccess.lookupJavaType(c);
assertEquals(type.getSourceFileName(), "Object.java");
}
thanks,
Coleen
dl
On 4/22/20 8:18 AM, coleen.phillim...@oracle.com wrote:
Hi Dean, Thank you for looking at the JVMCI changes and the
suggestion to add the test. I did this and found a bug. The new
test is quite limited because there's no good test to see if a
source file name can assertNotNull(type.getSourceFileName()), so I
couldn't iterate through the list of loaded classes like the other
tests in that file.
http://cr.openjdk.java.net/~coleenp/2020/8238048.02.incr/webrev/index.html
Thanks,
Coleen
On 4/21/20 9:51 PM, Dean Long wrote:
Hi Coleen. The JVMCI changes look OK. It looks like there is a
Graal unittest that covers getSourceFileName, but those tests don't
always get run. If it's not too much trouble, could you look into
enabling getSourceFileName() testing in
test/hotspot/jtreg/compiler/jvmci/jdk.vm.ci.runtime.test/src/jdk/vm/ci/runtime/test/TestResolvedJavaType.java
? It's currently on the "untested" list.
thanks,
dl
On 4/21/20 1:12 PM, coleen.phillim...@oracle.com wrote:
Summary: moved fields around and some constant fields into
ConstantPool
This is a simple change except that I moved some constant fields
from InstanceKlass into the constant pool so they can be shared
read-only in the CDS archive. There are associated repercussions
in SA and JVMCI, so please look at these changes. Also moved
similarly sized fields together in the class so there's less
likelihood of introducing gaps in future InstanceKlass changes.
InstanceKlass is reduced from 544 to 520 bytes in a simple Hello
World class.
open webrev at
http://cr.openjdk.java.net/~coleenp/2020/8238048.01/webrev
bug link https://bugs.openjdk.java.net/browse/JDK-8238048
Tested with tier1-6.
Thanks,
Coleen
Re: RFR: 8243450: Reomve VMOps from jdk.hotspot.agent
Looks good. Chris On 4/22/20 7:22 PM, Yasumasa Suenaga wrote: Hi all, Please review removing file from jdk.hotspot.agent: JBS: https://bugs.openjdk.java.net/browse/JDK-8243450 webrev: http://cr.openjdk.java.net/~ysuenaga/JDK-8243450/webrev.00/ VMOps is enum class which lists all of VM Operation, but it has not been synced with HotSpot source. In addition, it is not used in src/ and test/. This change has passed tests on submit repo (mach5-one-ysuenaga-JDK-8243450-20200423-0039-10418768). Thanks, Yasumasa
Re: RFR (M) Close alignment gaps in InstanceKlass
Can you compare the result to some string, like "Object.java"? That seems to be what HotSpotResolvedObjectTypeTest.java is doing. Also, did getSourceFileName() return null for arrays before your change? dl On 4/22/20 8:18 AM, coleen.phillim...@oracle.com wrote: Hi Dean, Thank you for looking at the JVMCI changes and the suggestion to add the test. I did this and found a bug. The new test is quite limited because there's no good test to see if a source file name can assertNotNull(type.getSourceFileName()), so I couldn't iterate through the list of loaded classes like the other tests in that file. http://cr.openjdk.java.net/~coleenp/2020/8238048.02.incr/webrev/index.html Thanks, Coleen On 4/21/20 9:51 PM, Dean Long wrote: Hi Coleen. The JVMCI changes look OK. It looks like there is a Graal unittest that covers getSourceFileName, but those tests don't always get run. If it's not too much trouble, could you look into enabling getSourceFileName() testing in test/hotspot/jtreg/compiler/jvmci/jdk.vm.ci.runtime.test/src/jdk/vm/ci/runtime/test/TestResolvedJavaType.java ? It's currently on the "untested" list. thanks, dl On 4/21/20 1:12 PM, coleen.phillim...@oracle.com wrote: Summary: moved fields around and some constant fields into ConstantPool This is a simple change except that I moved some constant fields from InstanceKlass into the constant pool so they can be shared read-only in the CDS archive. There are associated repercussions in SA and JVMCI, so please look at these changes. Also moved similarly sized fields together in the class so there's less likelihood of introducing gaps in future InstanceKlass changes. InstanceKlass is reduced from 544 to 520 bytes in a simple Hello World class. open webrev at http://cr.openjdk.java.net/~coleenp/2020/8238048.01/webrev bug link https://bugs.openjdk.java.net/browse/JDK-8238048 Tested with tier1-6. Thanks, Coleen
RFR: 8243450: Reomve VMOps from jdk.hotspot.agent
Hi all, Please review removing file from jdk.hotspot.agent: JBS: https://bugs.openjdk.java.net/browse/JDK-8243450 webrev: http://cr.openjdk.java.net/~ysuenaga/JDK-8243450/webrev.00/ VMOps is enum class which lists all of VM Operation, but it has not been synced with HotSpot source. In addition, it is not used in src/ and test/. This change has passed tests on submit repo (mach5-one-ysuenaga-JDK-8243450-20200423-0039-10418768). Thanks, Yasumasa
Re: RFR: 8243450: Reomve VMOps from jdk.hotspot.agent
Hi Yasumasa, On 23/04/2020 12:22 pm, Yasumasa Suenaga wrote: Hi all, Please review removing file from jdk.hotspot.agent: JBS: https://bugs.openjdk.java.net/browse/JDK-8243450 webrev: http://cr.openjdk.java.net/~ysuenaga/JDK-8243450/webrev.00/ VMOps is enum class which lists all of VM Operation, but it has not been synced with HotSpot source. In addition, it is not used in src/ and test/. Strange. It was added by: https://bugs.openjdk.java.net/browse/JDK-8046282 http://hg.openjdk.java.net/jdk9/jdk9/hotspot/rev/74ab5b554535 but appears to be unused even then. So I guess it is okay to remove it. Thanks, David This change has passed tests on submit repo (mach5-one-ysuenaga-JDK-8243450-20200423-0039-10418768). Thanks, Yasumasa
Re: RFR: 8243450: Reomve VMOps from jdk.hotspot.agent
Thanks Chris and David for quick review! Yasumasa On 2020/04/23 11:22, Yasumasa Suenaga wrote: Hi all, Please review removing file from jdk.hotspot.agent: JBS: https://bugs.openjdk.java.net/browse/JDK-8243450 webrev: http://cr.openjdk.java.net/~ysuenaga/JDK-8243450/webrev.00/ VMOps is enum class which lists all of VM Operation, but it has not been synced with HotSpot source. In addition, it is not used in src/ and test/. This change has passed tests on submit repo (mach5-one-ysuenaga-JDK-8243450-20200423-0039-10418768). Thanks, Yasumasa
Re: RFR(L): 8215624: add parallel heap inspection support for jmap histo(G1)(Internet mail)
Thanks Paul! I agree with using "parallel", will make the update in next patch, Thanks for help update the CSR. BRs, Lin On 2020/4/23, 4:42 AM, "Hohensee, Paul" wrote: For the interface, I'd use "parallel" instead of "parallelThreadNum". All the other options are lower case, and it's a lot easier to type "parallel". I took the liberty of updating the CSR. If you're ok with it, you might want to change variable names and such, plus of course JMap.usage. Thanks, Paul On 4/22/20, 2:29 AM, "serviceability-dev on behalf of linzang(臧琳)" wrote: Dear Stefan, Thanks a lot! I agree with you to decouple the heap inspection code with GC's. I will start from your POC code, may discuss with you later. BRs, Lin On 2020/4/22, 5:14 PM, "Stefan Karlsson" wrote: Hi Lin, I took a look at this earlier and saw that the heap inspection code is strongly coupled with the CollectedHeap and G1CollectedHeap. I'd prefer if we'd abstract this away, so that the GCs only provide a "parallel object iteration" interface, and the heap inspection code is kept elsewhere. I started experimenting with doing that, but other higher-priority (to me) tasks have had to take precedence. I've uploaded my work-in-progress / proof-of-concept: https://cr.openjdk.java.net/~stefank/8215624/webrev.01.delta/ https://cr.openjdk.java.net/~stefank/8215624/webrev.01/ The current code doesn't handle the lifecycle (deletion) of the ParallelObjectIterators. There's also code left unimplemented in around CollectedHeap::run_task. However, I think this could work as a basis to pull out the heap inspection code out of the GCs. Thanks, StefanK On 2020-04-22 02:21, linzang(臧琳) wrote: > Dear all, > May I ask you help to review? This RFR has been there for quite a while. > Thanks! > > BRs, > Lin > > > On 2020/3/16, 5:18 PM, "linzang(臧琳)" wrote:> > >>Just update a new path, my preliminary measure show about 3.5x speedup of jmap histo on a nearly full 4GB G1 heap (8-core platform with parallel thread number set to 4). >> webrev: http://cr.openjdk.java.net/~lzang/jmap-8214535/8215624/webrev_02/ >> bug: https://bugs.openjdk.java.net/browse/JDK-8215624 >> CSR: https://bugs.openjdk.java.net/browse/JDK-8239290 >> BRs, >> Lin >> > On 2020/3/2, 9:56 PM, "linzang(臧琳)" wrote: >> > >> >Dear all, >> > Let me try to ease the reviewing work by some explanation :P >> > The patch's target is to speed up jmap -histo for heap iteration, from my experience it is necessary for large heap investigation. E.g in bigData scenario I have tried to conduct jmap -histo against 180GB heap, it does take quite a while. >> > And if my understanding is corrent, even the jmap -histo without "live" option does heap inspection with heap lock acquired. so it is very likely to block mutator thread in allocation-sensitive scenario. I would say the faster the heap inspection does, the shorter the mutator be blocked. This is parallel iteration for jmap is necessary. >> > I think the parallel heap inspection should be applied to all kind of heap. However, consider the heap layout are different for GCs, much time is required to understand all kinds of the heap layout to make the whole change. IMO, It is not wise to have a huge patch for the whole solution at once, and it is even harder to review it. So I plan to implement it incrementally, the first patch (this one) is going to confirm the implemention detail of how jmap accept the new option, passes it to attachListener of the jvm process and then how to make the parallel inspection closure be generic enough to make it easy to extend to different heap layout. And also how to implement the heap inspection in specific gc's heap. This patch use G1's heap as the begining. >> > This patch actually do several things: >> > 1. Add an option "parallelThreadNum=" to jmap -histo, the default behavior is to set N to 0, means let's JVM decide how many threads to use for heap inspection. Set this option to 1 will disable parallel heap inspection. (more details in CSR: https://bugs.openjdk.java.net/browse/JDK-8239290) >> > 2. Make a change in how Jmap passing arguments, changes in
Re: RFR (M) Close alignment gaps in InstanceKlass
It looks like calling the JVMCI getSourceFileName() on an array would have accessed random memory because it was expecting an InstanceKlass. Instead of returning null we might want to throw an exception like in HotSpotResolvedPrimitiveType. dl On 4/22/20 5:39 PM, Dean Long wrote: Can you compare the result to some string, like "Object.java"? That seems to be what HotSpotResolvedObjectTypeTest.java is doing. Also, did getSourceFileName() return null for arrays before your change? dl On 4/22/20 8:18 AM, coleen.phillim...@oracle.com wrote: Hi Dean, Thank you for looking at the JVMCI changes and the suggestion to add the test. I did this and found a bug. The new test is quite limited because there's no good test to see if a source file name can assertNotNull(type.getSourceFileName()), so I couldn't iterate through the list of loaded classes like the other tests in that file. http://cr.openjdk.java.net/~coleenp/2020/8238048.02.incr/webrev/index.html Thanks, Coleen On 4/21/20 9:51 PM, Dean Long wrote: Hi Coleen. The JVMCI changes look OK. It looks like there is a Graal unittest that covers getSourceFileName, but those tests don't always get run. If it's not too much trouble, could you look into enabling getSourceFileName() testing in test/hotspot/jtreg/compiler/jvmci/jdk.vm.ci.runtime.test/src/jdk/vm/ci/runtime/test/TestResolvedJavaType.java ? It's currently on the "untested" list. thanks, dl On 4/21/20 1:12 PM, coleen.phillim...@oracle.com wrote: Summary: moved fields around and some constant fields into ConstantPool This is a simple change except that I moved some constant fields from InstanceKlass into the constant pool so they can be shared read-only in the CDS archive. There are associated repercussions in SA and JVMCI, so please look at these changes. Also moved similarly sized fields together in the class so there's less likelihood of introducing gaps in future InstanceKlass changes. InstanceKlass is reduced from 544 to 520 bytes in a simple Hello World class. open webrev at http://cr.openjdk.java.net/~coleenp/2020/8238048.01/webrev bug link https://bugs.openjdk.java.net/browse/JDK-8238048 Tested with tier1-6. Thanks, Coleen
Re: RFR(L): 8215624: add parallel heap inspection support for jmap histo(G1)
Hi Lin, I took a look at this earlier and saw that the heap inspection code is strongly coupled with the CollectedHeap and G1CollectedHeap. I'd prefer if we'd abstract this away, so that the GCs only provide a "parallel object iteration" interface, and the heap inspection code is kept elsewhere. I started experimenting with doing that, but other higher-priority (to me) tasks have had to take precedence. I've uploaded my work-in-progress / proof-of-concept: https://cr.openjdk.java.net/~stefank/8215624/webrev.01.delta/ https://cr.openjdk.java.net/~stefank/8215624/webrev.01/ The current code doesn't handle the lifecycle (deletion) of the ParallelObjectIterators. There's also code left unimplemented in around CollectedHeap::run_task. However, I think this could work as a basis to pull out the heap inspection code out of the GCs. Thanks, StefanK On 2020-04-22 02:21, linzang(臧琳) wrote: Dear all, May I ask you help to review? This RFR has been there for quite a while. Thanks! BRs, Lin > On 2020/3/16, 5:18 PM, "linzang(臧琳)" wrote:> Just update a new path, my preliminary measure show about 3.5x speedup of jmap histo on a nearly full 4GB G1 heap (8-core platform with parallel thread number set to 4). webrev: http://cr.openjdk.java.net/~lzang/jmap-8214535/8215624/webrev_02/ bug: https://bugs.openjdk.java.net/browse/JDK-8215624 CSR: https://bugs.openjdk.java.net/browse/JDK-8239290 BRs, Lin > On 2020/3/2, 9:56 PM, "linzang(臧琳)" wrote: > >Dear all, > Let me try to ease the reviewing work by some explanation :P > The patch's target is to speed up jmap -histo for heap iteration, from my experience it is necessary for large heap investigation. E.g in bigData scenario I have tried to conduct jmap -histo against 180GB heap, it does take quite a while. > And if my understanding is corrent, even the jmap -histo without "live" option does heap inspection with heap lock acquired. so it is very likely to block mutator thread in allocation-sensitive scenario. I would say the faster the heap inspection does, the shorter the mutator be blocked. This is parallel iteration for jmap is necessary. > I think the parallel heap inspection should be applied to all kind of heap. However, consider the heap layout are different for GCs, much time is required to understand all kinds of the heap layout to make the whole change. IMO, It is not wise to have a huge patch for the whole solution at once, and it is even harder to review it. So I plan to implement it incrementally, the first patch (this one) is going to confirm the implemention detail of how jmap accept the new option, passes it to attachListener of the jvm process and then how to make the parallel inspection closure be generic enough to make it easy to extend to different heap layout. And also how to implement the heap inspection in specific gc's heap. This patch use G1's heap as the begining. > This patch actually do several things: > 1. Add an option "parallelThreadNum=" to jmap -histo, the default behavior is to set N to 0, means let's JVM decide how many threads to use for heap inspection. Set this option to 1 will disable parallel heap inspection. (more details in CSR: https://bugs.openjdk.java.net/browse/JDK-8239290) > 2. Make a change in how Jmap passing arguments, changes in http://cr.openjdk.java.net/~lzang/jmap-8214535/8215624/webrev_01/src/jdk.jcmd/share/classes/sun/tools/jmap/JMap.java.udiff.html, originally it pass options as separate arguments to attachListener, this patch change to that all options be compose to a single string. So the arg_count_max in attachListener.hpp do not need to be changed, and hence avoid the compatibility issue, as disscussed at https://mail.openjdk.java.net/pipermail/serviceability-dev/2019-March/027334.html > 3. Add an abstract class ParHeapInspectTask in heapInspection.hpp / heapInspection.cpp, It's work(uint worker_id) method prepares the data structure (KlassInfoTable) need for every parallel worker thread, and then call do_object_iterate_parallel() which is heap specific implementation. I also added some machenism in KlassInfoTable to support parallel iteration, such as merge(). >4. In specific heap (G1 in this patch), create a subclass of ParHeapInspectTask, implement the do_object_iterate_parallel() for parallel heap inspection. For G1, it simply invoke g1CollectedHeap's object_iterate_parallel(). >5. Add related test. >6. it may be easy to extend this patch for other kinds of heap by creating subclass of ParHeapInspectTask and implement the do_object_iterate_parallel(). > >Hope these info could help on code review and initate the discussion :-) >Thanks! > >BRs, >Lin >>On 2020/2/19, 9:40 AM,
Re: RFR(L): 8215624: add parallel heap inspection support for jmap histo(G1)(Internet mail)
Dear Stefan, Thanks a lot! I agree with you to decouple the heap inspection code with GC's. I will start from your POC code, may discuss with you later. BRs, Lin On 2020/4/22, 5:14 PM, "Stefan Karlsson" wrote: Hi Lin, I took a look at this earlier and saw that the heap inspection code is strongly coupled with the CollectedHeap and G1CollectedHeap. I'd prefer if we'd abstract this away, so that the GCs only provide a "parallel object iteration" interface, and the heap inspection code is kept elsewhere. I started experimenting with doing that, but other higher-priority (to me) tasks have had to take precedence. I've uploaded my work-in-progress / proof-of-concept: https://cr.openjdk.java.net/~stefank/8215624/webrev.01.delta/ https://cr.openjdk.java.net/~stefank/8215624/webrev.01/ The current code doesn't handle the lifecycle (deletion) of the ParallelObjectIterators. There's also code left unimplemented in around CollectedHeap::run_task. However, I think this could work as a basis to pull out the heap inspection code out of the GCs. Thanks, StefanK On 2020-04-22 02:21, linzang(臧琳) wrote: > Dear all, > May I ask you help to review? This RFR has been there for quite a while. > Thanks! > > BRs, > Lin > > > On 2020/3/16, 5:18 PM, "linzang(臧琳)" wrote:> > >>Just update a new path, my preliminary measure show about 3.5x speedup of jmap histo on a nearly full 4GB G1 heap (8-core platform with parallel thread number set to 4). >> webrev: http://cr.openjdk.java.net/~lzang/jmap-8214535/8215624/webrev_02/ >> bug: https://bugs.openjdk.java.net/browse/JDK-8215624 >> CSR: https://bugs.openjdk.java.net/browse/JDK-8239290 >> BRs, >> Lin >> > On 2020/3/2, 9:56 PM, "linzang(臧琳)" wrote: >> > >> >Dear all, >> > Let me try to ease the reviewing work by some explanation :P >> > The patch's target is to speed up jmap -histo for heap iteration, from my experience it is necessary for large heap investigation. E.g in bigData scenario I have tried to conduct jmap -histo against 180GB heap, it does take quite a while. >> > And if my understanding is corrent, even the jmap -histo without "live" option does heap inspection with heap lock acquired. so it is very likely to block mutator thread in allocation-sensitive scenario. I would say the faster the heap inspection does, the shorter the mutator be blocked. This is parallel iteration for jmap is necessary. >> > I think the parallel heap inspection should be applied to all kind of heap. However, consider the heap layout are different for GCs, much time is required to understand all kinds of the heap layout to make the whole change. IMO, It is not wise to have a huge patch for the whole solution at once, and it is even harder to review it. So I plan to implement it incrementally, the first patch (this one) is going to confirm the implemention detail of how jmap accept the new option, passes it to attachListener of the jvm process and then how to make the parallel inspection closure be generic enough to make it easy to extend to different heap layout. And also how to implement the heap inspection in specific gc's heap. This patch use G1's heap as the begining. >> > This patch actually do several things: >> > 1. Add an option "parallelThreadNum=" to jmap -histo, the default behavior is to set N to 0, means let's JVM decide how many threads to use for heap inspection. Set this option to 1 will disable parallel heap inspection. (more details in CSR: https://bugs.openjdk.java.net/browse/JDK-8239290) >> > 2. Make a change in how Jmap passing arguments, changes in http://cr.openjdk.java.net/~lzang/jmap-8214535/8215624/webrev_01/src/jdk.jcmd/share/classes/sun/tools/jmap/JMap.java.udiff.html, originally it pass options as separate arguments to attachListener, this patch change to that all options be compose to a single string. So the arg_count_max in attachListener.hpp do not need to be changed, and hence avoid the compatibility issue, as disscussed at https://mail.openjdk.java.net/pipermail/serviceability-dev/2019-March/027334.html >> > 3. Add an abstract class ParHeapInspectTask in heapInspection.hpp / heapInspection.cpp, It's work(uint worker_id) method prepares the data structure (KlassInfoTable) need for every parallel worker thread, and then call do_object_iterate_parallel() which is heap specific implementation. I also added some machenism in KlassInfoTable to support parallel iteration, such as merge(). >> >4. In specific heap (G1 in this patch), create a subclass of ParHeapInspectTask, implement the do_object_iterate_parallel()
RFR: 8238561 serviceability/sa tests continue to run out of memory on Win* machines
Please review the change [1] that ensures that VM and test options are forwarded to j*-tools when they are launched from serviceability/sa tests. In particular, it will ensure that passed to the tests maximum heap size settings ( -XX:MaxRAMPercentage) are also honored by j*-tools serviceability/sa tests launch. The tests that expect an empty output were corrected to ignore the product version printed in the error stream since in some tiers the tests are run with ' -showversion' VM option. Testing: Mach5 tests for tier1 - tier7 passed. [1] http://cr.openjdk.java.net/~dtitov/8238561/webrev.01 [2] https://bugs.openjdk.java.net/browse/JDK-8238561 Thank you, Daniil
Re: RFR(XS) 8243210: ClhsdbScanOops fails with NullPointerException in FileMapHeader.inCopiedVtableSpace
Hi Chris, the change looks good to me, too. Thanks - Ioi On 4/21/20 6:35 PM, serguei.spit...@oracle.com wrote: Hi Chris, It looks reasonable. Thanks, Serguei On 4/21/20 16:07, Chris Plummer wrote: Hello, Please review the following: https://bugs.openjdk.java.net/browse/JDK-8243210 http://cr.openjdk.java.net/~cjplummer/8243210/webrev.01/index.html Description is in the bug. Hopefully this is the last in a series of about 6 bugs being addressed that finally lets us get this test off the problem list for all platforms. thanks, Chris
RFR (L): 8241214: Test debugging of hidden classes using jdb
Please, review a fix for the sub-task: https://bugs.openjdk.java.net/browse/JDK-8241214 This fix has already been reviewed internally (in Vlahalla project) by Mandy, Chris and Alex. This RFR is to collect more comments (if there are any) before push. Webrev: http://cr.openjdk.java.net/~sspitsyn/webrevs/2020/valhalla-hidden-jdb.7/ Summary: We want to be able to debug hidden classes with the debuggers. One way to test the JDI/JDWP is by using the JDB, so we want the JDB to support hidden classes. The PatternReferenceTypeSpec::checkClassName is used to check if the class is valid when a event requests is set in jdb. Good example is to set a breakpoint which can be deferred. The fix is to accept hidden class names as valid to be able to debug hidden classes with the JDB. New test is to provide basic coverage for JDB (amd JDI as well). It verifies that the following JDB commands handle hidden classes correctly: - class, classes - fields and methods - stop in, watch, unwatch - where, up and down - eval, print and dump for fields (both positive and negative checks) Testing: Mach5 test run of the vmTestbase_nsk_jdb is in progress. Thanks, Serguei
Re: RFR: 8242425: JVMTI monitor operations should use Thread-Local Handshakes
Hi Yasumasa,
Yes, it looks like VMOps in SA can be removed. It's probably bit rotted
code. My guess is at some point there was support, or were plans for
supporting, the debugging of VMOps from within SA. Given that there are
no references to the VMOps class, it looks like none of that support
currently exists.
thanks,
Chris
On 4/20/20 5:24 PM, Yasumasa Suenaga wrote:
Hi Rechard,
Thank you for telling it to me.
I grep'ed jdk.hotspot.agent with VMOps (it is just enum), it does not
seem to be used.
In addition, VMOps has not already synced to HotSpot. For example,
VM_HandshakeOneThread does not appear in VMOps.
(I wonder how do we use VMOps in SA)
Thus I think we can (should) remove VMOps from jdk.hotspot.agent .
If other serviceability folks agree with it, I will file it to JBS.
Thanks,
Yasumasa
On 2020/04/21 0:02, Reingruber, Richard wrote:
Hi Yasumasa,
I'm obviously late to this review. Still I wanted to let you know,
that there's another file in the
source tree that lists the VM operations in the VM (just found out
about it myself):
src/jdk.hotspot.agent/share/classes/sun/jvm/hotspot/runtime/VMOps.java
Probably you should remove the VM operations from that file too.
It seems to be part of the serviceability agent, which I hardly know.
Would be good if somebody
who is more familiar with it could let us know...
Best regards,
Richard.
-Original Message-
From: serviceability-dev
On Behalf Of Yasumasa
Suenaga
Sent: Montag, 20. April 2020 02:33
To: serviceability-dev@openjdk.java.net
Cc: yasue...@gmail.com
Subject: Re: RFR: 8242425: JVMTI monitor operations should use
Thread-Local Handshakes
Hi all,
Could you review it?
JBS: https://bugs.openjdk.java.net/browse/JDK-8242425
webrev: http://cr.openjdk.java.net/~ysuenaga/JDK-8242425/webrev.02/
I need one more reviewer to push.
Thanks,
Yasumasa
On 2020/04/17 5:13, serguei.spit...@oracle.com wrote:
Hi Yasumasa,
Thank you for the update.
It looks good.
Thanks,
Serguei
On 4/10/20 04:30, Yasumasa Suenaga wrote:
Hi Serguei,
I use current_jt in this webrev. Could you review again?
http://cr.openjdk.java.net/~ysuenaga/JDK-8242425/webrev.02/
I tested this change with vmTestbase/nsk/jvmti, they are fine on my
Linux x64.
Thanks,
Yasumasa
On 2020/04/10 17:21, serguei.spit...@oracle.com wrote:
Hi Yasumasa,
Thank you for the update.
Minor:
http://cr.openjdk.java.net/~ysuenaga/JDK-8242425/webrev.01/src/hotspot/share/prims/jvmtiEnvBase.cpp.udiff.html
+ err = get_locked_objects_in_frame(JavaThread::current(),
java_thread, jvf, owned_monitors_list, depth-1); . . .
+ JvmtiMonitorClosure jmc(java_thread, JavaThread::current(),
owned_monitors_list, this);
You can use current_jt instead of JavaThread::current() above.
There is also some test coverage in the
vmTestbase/nsk/jvmti/scenarios tests.
This one as well:
test/hotspot/jtreg/vmTestbase/nsk/jvmti/unit/functions/nosuspendMonitorInfo/JvmtiTest
Probably it is easier to run all vmTestbase/nsk/jvmti tests.
Thanks,
Serguei
On 4/10/20 01:05, Yasumasa Suenaga wrote:
Hi Serguei,
Thanks for your comment!
I uploaded new webrev:
http://cr.openjdk.java.net/~ysuenaga/JDK-8242425/webrev.01/
I ran following tests, and all of them were passed on my Linux x64.
- vmTestbase/nsk/jvmti/GetCurrentContendedMonitor
- vmTestbase/nsk/jvmti/GetOwnedMonitorInfo
- vmTestbase/nsk/jdi
- vmTestbase/nsk/jdwp
- serviceability/jvmti/GetOwnedMonitorInfo
- serviceability/jvmti/GetOwnedMonitorStackDepthInfo
- serviceability/jdwp
Thanks,
Yasumasa
On 2020/04/10 14:50, serguei.spit...@oracle.com wrote:
Hi Yasumasa,
It looks pretty good in general.
A couple of comments though.
http://cr.openjdk.java.net/~ysuenaga/JDK-8242425/webrev.00/src/hotspot/share/prims/jvmtiEnvBase.cpp.frames.html
650 JvmtiEnvBase::get_current_contended_monitor(JavaThread
*java_thread, jobject *monitor_ptr) {
651 assert(!Thread::current()->is_VM_thread() &&
652 (Thread::current() == java_thread ||
653 Thread::current() == java_thread->active_handshaker()),
654 "call by myself or at direct handshake");
...
685 JvmtiEnvBase::get_owned_monitors(JavaThread* java_thread,
686 GrowableArray
*owned_monitors_list) {
687 jvmtiError err = JVMTI_ERROR_NONE;
688 assert(!Thread::current()->is_VM_thread() &&
689 (Thread::current() == java_thread ||
690 Thread::current() == java_thread->active_handshaker()),
691 "call by myself or at direct handshake");
I'd suggest to add this at the beginning:
JavaThread *current_jt = JavaThread::current();
676 JavaThread *current_jt = reinterpret_cast*>(JavaThread::current());
There is not need in reinterpret_cast. Also, you
can use the current_jt defined above.
Also, please, removed these two definitions as they became
unused with your fix:
src/hotspot/share/runtime/vmOperations.hpp:
template(GGetCurrentContendedMonitoretOwnedMonitorInfo) \
src/hotspot/share/runtime/vmOperations.hpp:
template()
Re: RFR (L): 8241214: Test debugging of hidden classes using jdb
Looks good,
Here are several comments, mostly about code style/compliance. No
another review is needed if you want to fix them.
http://cr.openjdk.java.net/~sspitsyn/webrevs/2020/valhalla-hidden-jdb.7/test/hotspot/jtreg/vmTestbase/nsk/jdb/hidden_class/hc001/hc001.java.html
36 interface HC_Interface {
The name HC_Interface is not "CamelCase".
82 log = new PrintStream("Debuggee.log");
This PrintStream is not closed. Should not be a problem int this case,
just good style.
Leonid
On 4/22/20 11:22 AM, serguei.spit...@oracle.com wrote:
Please, review a fix for the sub-task:
https://bugs.openjdk.java.net/browse/JDK-8241214
This fix has already been reviewed internally (in Vlahalla project) by
Mandy, Chris and Alex.
This RFR is to collect more comments (if there are any) before push.
Webrev:
http://cr.openjdk.java.net/~sspitsyn/webrevs/2020/valhalla-hidden-jdb.7/
Summary:
We want to be able to debug hidden classes with the debuggers.
One way to test the JDI/JDWP is by using the JDB,
so we want the JDB to support hidden classes.
The PatternReferenceTypeSpec::checkClassName is used to check
if the class is valid when a event requests is set in jdb.
Good example is to set a breakpoint which can be deferred.
The fix is to accept hidden class names as valid to be able
to debug hidden classes with the JDB.
New test is to provide basic coverage for JDB (amd JDI as well).
It verifies that the following JDB commands handle hidden classes
correctly:
- class, classes
- fields and methods
- stop in, watch, unwatch
- where, up and down
- eval, print and dump for fields (both positive and negative checks)
Testing:
Mach5 test run of the vmTestbase_nsk_jdb is in progress.
Thanks,
Serguei
Re: RFR (L): 8241214: Test debugging of hidden classes using jdb
Hi Leonid,
Thank you for the review!
I'll address your comments.
Thanks,
Serguei
On 4/22/20 13:06, Leonid Mesnik wrote:
Looks good,
Here are several comments, mostly about code style/compliance.
No another review is needed if you want to fix them.
http://cr.openjdk.java.net/~sspitsyn/webrevs/2020/valhalla-hidden-jdb.7/test/hotspot/jtreg/vmTestbase/nsk/jdb/hidden_class/hc001/hc001.java.html
36 interface HC_Interface {
The name HC_Interface is not "CamelCase".
82 log = new PrintStream("Debuggee.log");
This PrintStream is not closed. Should not be a problem int
this case, just good style.
Leonid
On 4/22/20 11:22 AM, serguei.spit...@oracle.com wrote:
Please, review a fix for the sub-task:
https://bugs.openjdk.java.net/browse/JDK-8241214
This fix has already been reviewed internally (in Vlahalla
project) by Mandy, Chris and Alex.
This RFR is to collect more comments (if there are any) before
push.
Webrev:
http://cr.openjdk.java.net/~sspitsyn/webrevs/2020/valhalla-hidden-jdb.7/
Summary:
We want to be able to debug hidden classes with the debuggers.
One way to test the JDI/JDWP is by using the JDB,
so we want the JDB to support hidden classes.
The PatternReferenceTypeSpec::checkClassName is used to check
if the class is valid when a event requests is set in jdb.
Good example is to set a breakpoint which can be deferred.
The fix is to accept hidden class names as valid to be able
to debug hidden classes with the JDB.
New test is to provide basic coverage for JDB (amd JDI as
well).
It verifies that the following JDB commands handle hidden
classes correctly:
- class, classes
- fields and methods
- stop in, watch, unwatch
- where, up and down
- eval, print and dump for fields (both positive and negative
checks)
Testing:
Mach5 test run of the vmTestbase_nsk_jdb is in progress.
Thanks,
Serguei
RE: RFR(L): 8215624: add parallel heap inspection support for jmap histo(G1)(Internet mail)
For the interface, I'd use "parallel" instead of "parallelThreadNum". All the other options are lower case, and it's a lot easier to type "parallel". I took the liberty of updating the CSR. If you're ok with it, you might want to change variable names and such, plus of course JMap.usage. Thanks, Paul On 4/22/20, 2:29 AM, "serviceability-dev on behalf of linzang(臧琳)" wrote: Dear Stefan, Thanks a lot! I agree with you to decouple the heap inspection code with GC's. I will start from your POC code, may discuss with you later. BRs, Lin On 2020/4/22, 5:14 PM, "Stefan Karlsson" wrote: Hi Lin, I took a look at this earlier and saw that the heap inspection code is strongly coupled with the CollectedHeap and G1CollectedHeap. I'd prefer if we'd abstract this away, so that the GCs only provide a "parallel object iteration" interface, and the heap inspection code is kept elsewhere. I started experimenting with doing that, but other higher-priority (to me) tasks have had to take precedence. I've uploaded my work-in-progress / proof-of-concept: https://cr.openjdk.java.net/~stefank/8215624/webrev.01.delta/ https://cr.openjdk.java.net/~stefank/8215624/webrev.01/ The current code doesn't handle the lifecycle (deletion) of the ParallelObjectIterators. There's also code left unimplemented in around CollectedHeap::run_task. However, I think this could work as a basis to pull out the heap inspection code out of the GCs. Thanks, StefanK On 2020-04-22 02:21, linzang(臧琳) wrote: > Dear all, > May I ask you help to review? This RFR has been there for quite a while. > Thanks! > > BRs, > Lin > > > On 2020/3/16, 5:18 PM, "linzang(臧琳)" wrote:> > >>Just update a new path, my preliminary measure show about 3.5x speedup of jmap histo on a nearly full 4GB G1 heap (8-core platform with parallel thread number set to 4). >> webrev: http://cr.openjdk.java.net/~lzang/jmap-8214535/8215624/webrev_02/ >> bug: https://bugs.openjdk.java.net/browse/JDK-8215624 >> CSR: https://bugs.openjdk.java.net/browse/JDK-8239290 >> BRs, >> Lin >> > On 2020/3/2, 9:56 PM, "linzang(臧琳)" wrote: >> > >> >Dear all, >> > Let me try to ease the reviewing work by some explanation :P >> > The patch's target is to speed up jmap -histo for heap iteration, from my experience it is necessary for large heap investigation. E.g in bigData scenario I have tried to conduct jmap -histo against 180GB heap, it does take quite a while. >> > And if my understanding is corrent, even the jmap -histo without "live" option does heap inspection with heap lock acquired. so it is very likely to block mutator thread in allocation-sensitive scenario. I would say the faster the heap inspection does, the shorter the mutator be blocked. This is parallel iteration for jmap is necessary. >> > I think the parallel heap inspection should be applied to all kind of heap. However, consider the heap layout are different for GCs, much time is required to understand all kinds of the heap layout to make the whole change. IMO, It is not wise to have a huge patch for the whole solution at once, and it is even harder to review it. So I plan to implement it incrementally, the first patch (this one) is going to confirm the implemention detail of how jmap accept the new option, passes it to attachListener of the jvm process and then how to make the parallel inspection closure be generic enough to make it easy to extend to different heap layout. And also how to implement the heap inspection in specific gc's heap. This patch use G1's heap as the begining. >> > This patch actually do several things: >> > 1. Add an option "parallelThreadNum=" to jmap -histo, the default behavior is to set N to 0, means let's JVM decide how many threads to use for heap inspection. Set this option to 1 will disable parallel heap inspection. (more details in CSR: https://bugs.openjdk.java.net/browse/JDK-8239290) >> > 2. Make a change in how Jmap passing arguments, changes in http://cr.openjdk.java.net/~lzang/jmap-8214535/8215624/webrev_01/src/jdk.jcmd/share/classes/sun/tools/jmap/JMap.java.udiff.html, originally it pass options as separate arguments to attachListener, this patch change to that all options be compose to a single string. So the arg_count_max in attachListener.hpp do not need to be changed, and hence avoid the compatibility issue, as disscussed at https://mail.openjdk.java.net/pipermail/serviceability-dev/2019-March/027334.html
Re: RFR(XS) 8243210: ClhsdbScanOops fails with NullPointerException in FileMapHeader.inCopiedVtableSpace
Thanks Ioi and Serguei! Chris On 4/22/20 11:42 AM, Ioi Lam wrote: Hi Chris, the change looks good to me, too. Thanks - Ioi On 4/21/20 6:35 PM, serguei.spit...@oracle.com wrote: Hi Chris, It looks reasonable. Thanks, Serguei On 4/21/20 16:07, Chris Plummer wrote: Hello, Please review the following: https://bugs.openjdk.java.net/browse/JDK-8243210 http://cr.openjdk.java.net/~cjplummer/8243210/webrev.01/index.html Description is in the bug. Hopefully this is the last in a series of about 6 bugs being addressed that finally lets us get this test off the problem list for all platforms. thanks, Chris
Re: RFR: 8238561 serviceability/sa tests continue to run out of memory on Win* machines
Hi Daniil, Thanks for cleaning this up. I think this should be fixed under JDK-8242009. JDK-8238561 involves more than just this one issue. Is there a reason why you didn't just change JDKToolLauncher to have an option or API to add the args? Why are you calling Utils.addTestJavaOpts() instead of Utils.getTestJavaOpts()? Is your change causing -Xshowversion to be passed? Do you know where it is coming from? thanks, Chris [1] https://bugs.openjdk.java.net/browse/JDK-8242009 On 4/22/20 10:48 AM, Daniil Titov wrote: Please review the change [1] that ensures that VM and test options are forwarded to j*-tools when they are launched from serviceability/sa tests. In particular, it will ensure that passed to the tests maximum heap size settings ( -XX:MaxRAMPercentage) are also honored by j*-tools serviceability/sa tests launch. The tests that expect an empty output were corrected to ignore the product version printed in the error stream since in some tiers the tests are run with ' -showversion' VM option. Testing: Mach5 tests for tier1 - tier7 passed. [1] http://cr.openjdk.java.net/~dtitov/8238561/webrev.01 [2] https://bugs.openjdk.java.net/browse/JDK-8238561 Thank you, Daniil
