Re: RFR 8066708: JMXStartStopTest fails to connect to port 38112
Hi Jaroslav,
I'm distant enough from this code that I don't think I'm in a position to say
no you can't check this in, and I'm mindful of the fact that this bug is a
high priority and you want to get a fix in. But having said that I think there's
a surprising amount of complexity here for what it does.
Implementing a retry-on-BindException policy seems to be fairly sensible, since
I assume it would be fairly invasive to modify the code in the JVMs being forked
to use an ephemeral port and send that information back to the test.
My conjecture is however that the open/close/reopen logic is the primary cause
of the BindExceptions that occur. If you're going to retry on BindException, you
might as well choose a random port number instead of doing the open/close to get
a port number from the system.
The range that Dmitry suggests is reasonable, though I note that the actual
ephemeral port range used by the kernel will differ from OS to OS and even from
system to system. I don't know if that's really significant though. If you end
up choosing a port outside the ephemeral range for some system, does it really
matter?
If you do decide to have PortAllocator open and close a ServerSocket (in order
to find a previously unused port) I'd suggest removing the logic that leaves the
socket open until the first call to get(). That logic reduces the possibility
that some other process will open the socket after the close but before the
reopen. In my experience that's not what's causing the BindExceptions. It could
still happen, though, but you're protected by the retry logic anyway. Leaving
the socket open longer actually hurts, I think, because it increases the chance
that the kernel won't have cleaned up the port by the time the test wants to
reopen it.
If you change PortAllocator to close the socket immediately, you can get rid of
the need to call release() in a finally-block of the caller. You could also
change the type of the functional interface to be
int[] - void
since the PortAllocator doesn't hold onto any resources that need to be cleaned
up. It just calls the execute() method and passes an array of n port numbers.
It's probably necessary to have the socket close() call in a retry loop. The
socket is always closed immediately from the user process point of view, so
isClosed() will always return true immediately after the close() call returns.
You can verify this easily by looking in the ServerSocket.java source code. I
believe the state that prevents the port from being reused immediately is
private to the kernel and cannot be observed from a user process, at least not
without attempting to reopen the socket.
Side note: one of the jcmd() overloads says that parameter 'c' (a Consumer) may
be null. It doesn't look like this is handled. If you really want to support
this, I'd assign () - { } to it if it's null so that it can be called
unconditionally. (Or just disallow null.)
s'marks
On 1/6/15 2:00 PM, Dmitry Samersoff wrote:
Jaroslav,
It might be better to just choose a random digit between 49152–65535
and attempt to use it.
-Dmitry
On 2014-12-18 17:29, Jaroslav Bachorik wrote:
On 12/11/2014 03:43 PM, Dmitry Samersoff wrote:
Jaroslav,
You can set SO_LINGER to zero, in this case socket will be closed
immediately without waiting in TIME_WAIT
But there are no reliable way to predict whether you can take this port
or not after you close it.
So the only valid solution is to try to connect to a random port and if
this attempt fails try another random port. Everything else will cause
more or less frequent intermittent failures.
Thanks for all the suggestions!
http://cr.openjdk.java.net/~jbachorik/8066708/webrev.02
I've enhanced the original patch with the retry logic using different
random port if starting the JMX agent on the provided port fails with
BindException.
I'm keeping there the changes for properly closing the ports opened for
the test purposes and also setting the SO_REUSEADDR - anyway, it does
not make sense to reuse the ephemeral test ports.
I've split the original test_06 test case in order to keep it readable
even with the new retry logic - and also to make each test case to test
just one scenario.
Cheers,
-JB-
-Dmitry
On 2014-12-11 17:06, Jaroslav Bachorik wrote:
On 12/09/2014 01:25 PM, Jaroslav Bachorik wrote:
On 12/09/2014 01:39 AM, Stuart Marks wrote:
On 12/8/14 12:35 PM, Jaroslav Bachorik wrote:
Please, review the following test change
Issue : https://bugs.openjdk.java.net/browse/JDK-8066708
Webrev: http://cr.openjdk.java.net/~jbachorik/8066708/webrev.00
The test fails very intermittently when RMI registry is trying to
bind
to a port
previously used in the test (via ServerSocket).
This seems to be caused by the sockets created via `new
ServerSocket(0)` and
being in reusable mode. The fix attempts to prevent this by
explicitly
forbidding the reusable mode.
Hi Jaroslav,
I happened to see this fly by, and there
Re: RFR(XXS): 8065226: sun/jvmstat/monitor/MonitoredVm/CR6672135.java should be quarantined
Looks good! Thanks, /Staffan On 8 jan 2015, at 09:17, Yekaterina Kantserova yekaterina.kantser...@oracle.com wrote: Hi, Could I please have a review of this very small fix. bug: https://bugs.openjdk.java.net/browse/JDK-8065226 webrev: http://cr.openjdk.java.net/~ykantser/8065226/webrev.00 Thanks, Katja
Re: [9] RFR (M) 8054888: Runtime: Add Diagnostic Command that prints the class hierarchy
On 2015-01-08 00:29, Chris Plummer wrote: At the moment not much testing has been done other than running the DCMD and looking at the output. I'll do more once it's clear the code has settled. I would like to know if there are any existing tests for GC.class_stats and GC.class_histogram (there are none in the test directory). If so, possibly one could serve as the basis for a new test for VM.class_hierarchy. Unfortunately, there are no open tests either of them as of yet. There are, however, internal ones. I can give you the details offline. Mikael
Re: RFR 8067447: Factor out the shared implementation of the VM flags manipulation code
On 8.1.2015 03:45, David Holmes wrote:
On 8/01/2015 1:59 AM, Jaroslav Bachorik wrote:
On 7.1.2015 02:31, David Holmes wrote:
On 17/12/2014 8:19 PM, Jaroslav Bachorik wrote:
Please, review the following change.
Issue : https://bugs.openjdk.java.net/browse/JDK-8067447
Webrev: http://cr.openjdk.java.net/~jbachorik/8067447/webrev.00
This patch is a precursor for implementing
https://bugs.openjdk.java.net/browse/JDK-8054890 which itself is a part
of JEP-228 (https://bugs.openjdk.java.net/browse/JDK-8043764).
Here, the code related to manipulating JVM flags is extracted to a
separate ManagedFlags class and the codebease is adjusted to use this
class.
Not clear to me what this is addressing exactly - do we really need
platform specific variants of set flag ??
It has just been moved from the corresponding attachListener_os.cpp
files. And it is 'pd_set_flag' what, I suppose, means platform
dependent?
Yes it does and it mostly made sense inside the already pd
attachListener implementations, but it isn't obvious to me that it makes
sense in the ManagedFlag context. It is the choice about whether the
flag can be changed that is pd not the actual setting and those
choices are inherent to the attachListener mechanism they are not
Why would the ability to set Solaris specific flags via DTrace be
specific to the attachListener mechanism? Also, AFAICS here it is the
mechanism of setting the flag which is pd and not the choice
(DTrace::* vs CommandLineFlags::*)
inherent to ManagedFlags - so this refactoring seems wrong to me. What
exactly is ManagedFlag supposed to represent?
A shared functionality between attachListener.cpp, management.cpp and
the new diagnostic commands to be introduced later (as mentioned in the
original synopsis of this RFR). It did seem preferable over just copying
the implementation over to a few more places.
All the new code seems incorrect:
jint ManagedFlags::pd_set_flag(const char* flag_name, const char*
flag_value, Flag::Flags origin, outputStream* out) {
out-print_cr(flag '%s' cannot be changed, op-arg(0));
return JNI_ERR;
};
op-arg(0) comes from the original code where op was an
AttachOperation*. Here is should be using flag_name.
Correct. Slipped through and then replicated :/
And obviously never compiled. RFRs should be for tested code.
Yes, one should run always make clean first, just in case. I should
remember this well to prevent further embarrassment.
-JB-
David
-
Updated webrev: http://cr.openjdk.java.net/~jbachorik/8067447/webrev.01
-JB-
David
Thanks,
-JB-
RFR(XXS): 8068584: Compiler attach tests should be quarantined
Hi, Could I please have a review of this bug quarantine. bug: https://bugs.openjdk.java.net/browse/JDK-8068584 webrev: http://cr.openjdk.java.net/~ykantser/8068584/webrev.00/ Thanks, Mattias
Re: [9] RFR (M) 8054888: Runtime: Add Diagnostic Command that prints the class hierarchy
Hi Chris,
On 2015-01-08 00:29, Chris Plummer wrote:
Hi,
Please review the following changes for the addition of the
VM.class_hierarchy DCMD. Please read the bug first for some background
information.
Webrev: http://cr.openjdk.java.net/~cjplummer/8054888/webrev.00/
Bug: https://bugs.openjdk.java.net/browse/JDK-8054888
This looks like a nice feature. I think your suggestion about supporting
a class name argument as the root of the hierarchy would be a nice addition.
Some comments:
Why do you need / use the super_stack? To me it seems like you could
simplify the could if you get rid of the super_stack and walk the
Klass::super() chain instead.
Why did you add this side-effect to KlassInfoHisto::print_class_state?:
- super_index = super_e-index();
+ e-set_super_index(super_e-index());
AFAICT, you are not using KlassInfoHisto::print_class_stats to implement
the VM.class_hierarchy DCMD, right? Or am I missing something in your patch?
A side-note, if it were not for the AnonymousClasses (created by
Unsafe_DefineAnonymousClass), then this could have be implemented with
less resources by just walking the Klass::subclass() and
Klass::next_sibling() links. Which means that you wouldn't have to use
any of the classes or functionality in heapInspection.hpp/cpp. But the
anonymous classes is unfortunately not present in the
subclass/next_sibling hierarchy.
And some style comments:
http://cr.openjdk.java.net/~cjplummer/8054888/webrev.00/src/share/vm/services/diagnosticCommand.cpp.frames.html
Maybe it would be nice to move:
66 #if INCLUDE_SERVICES
67 DCmdFactory::register_DCmdFactory(new
DCmdFactoryImplClassHierarchyDCmd(full_export, true, false));
68 #endif
into the already existing INCLUDE_SERVICE block:
55 #if INCLUDE_SERVICES // Heap dumping/inspection supported
56 DCmdFactory::register_DCmdFactory(new
DCmdFactoryImplHeapDumpDCmd(DCmd_Source_Internal | DCmd_Source_AttachAPI,
true, false));
57 DCmdFactory::register_DCmdFactory(new
DCmdFactoryImplClassHistogramDCmd(full_export, true, false));
58 DCmdFactory::register_DCmdFactory(new
DCmdFactoryImplClassStatsDCmd(full_export, true, false));
59 #endif // INCLUDE_SERVICES
http://cr.openjdk.java.net/~cjplummer/8054888/webrev.00/src/share/vm/memory/heapInspection.hpp.frames.html
I would prefer if you moved the following implementation to the cpp
file, so that we can keep our includes in our hpp files to a minimal.
That helps lowering our include complexity.
218 inline void KlassInfoEntry::add_subclass(KlassInfoEntry* cie) {
219 if (_subclasses == NULL) {
220 _subclasses = new (ResourceObj::C_HEAP, mtInternal)
GrowableArrayKlassInfoEntry*(4, true);
221 }
222 _subclasses-append(cie);
223 }
224
225 inline KlassInfoEntry::~KlassInfoEntry() {
226 if (_subclasses != NULL) {
227 delete _subclasses;
228 }
229 }
http://cr.openjdk.java.net/~cjplummer/8054888/webrev.00/src/share/vm/memory/heapInspection.cpp.frames.html
Could you move the local variables to where they are used:
316 void KlassHierarchy::print_class_hierarchy(outputStream* st) {
317 ResourceMark rm;
318 int i;
319 Stack KlassInfoEntry*, mtClass class_stack;
320 Stack KlassInfoEntry*, mtClass super_stack;
321 GrowableArrayKlassInfoEntry* elements;
322
For example, 'int i' into the for statements:
336 // Set the index for each class
337 for(i=0; i elements.length(); i++) {
338 elements.at(i)-set_index(i+1);
339 }
Could you add spaces around the operators (= and +):
337 for(i=0; i elements.length(); i++) {
338 elements.at(i)-set_index(i+1);
Some of your new comments are not capitalized and some lack a period.
Example:
399 // print indentation with proper indicators of superclass.
454 // Add the stats for this class to the overall totals
Thanks,
StefanK
I expect there will be further restructuring or additional feature
work. More discussion on that below. I'm not sure if that additional
work will be done later with a separately bug filed or with this
initial commit. That's one thing I want to work out with this review.
Currently the bulk of the DCMD is implemented in heapInspection.cpp.
The main purpose of this file is to implement the GC.class_stats and
GC.class_histogram DCMDs. Both of them require walking the java heap
to count live objects of each type, thus the name
heapInspection.cpp. This new VM.class_hierarchy DCMD does not
require walking the heap, but is implemented in this file because it
leverages the existing KlassInfoTable and related classes
(KlassInfoEntry, KlassInfoBucket, and KlassClosure).
KlassInfoTable makes it easy to build a database of all loaded
classes, save additional info gathered for each class, iterate over
them quickly, and also do quick lookups. This exactly what I needed
for this DCMD, thus the reuse. There is some downside to this. For
starters, heapInspection.cpp is
Re: RFR 8067447: Factor out the shared implementation of the VM flags manipulation code
On 8.1.2015 12:12, David Holmes wrote:
On 8/01/2015 7:22 PM, Jaroslav Bachorik wrote:
On 8.1.2015 03:45, David Holmes wrote:
On 8/01/2015 1:59 AM, Jaroslav Bachorik wrote:
On 7.1.2015 02:31, David Holmes wrote:
On 17/12/2014 8:19 PM, Jaroslav Bachorik wrote:
Please, review the following change.
Issue : https://bugs.openjdk.java.net/browse/JDK-8067447
Webrev: http://cr.openjdk.java.net/~jbachorik/8067447/webrev.00
This patch is a precursor for implementing
https://bugs.openjdk.java.net/browse/JDK-8054890 which itself is a
part
of JEP-228 (https://bugs.openjdk.java.net/browse/JDK-8043764).
Here, the code related to manipulating JVM flags is extracted to a
separate ManagedFlags class and the codebease is adjusted to use this
class.
Not clear to me what this is addressing exactly - do we really need
platform specific variants of set flag ??
It has just been moved from the corresponding attachListener_os.cpp
files. And it is 'pd_set_flag' what, I suppose, means platform
dependent?
Yes it does and it mostly made sense inside the already pd
attachListener implementations, but it isn't obvious to me that it makes
sense in the ManagedFlag context. It is the choice about whether the
flag can be changed that is pd not the actual setting and those
choices are inherent to the attachListener mechanism they are not
Why would the ability to set Solaris specific flags via DTrace be
specific to the attachListener mechanism? Also, AFAICS here it is the
mechanism of setting the flag which is pd and not the choice
(DTrace::* vs CommandLineFlags::*)
The attachListener allows for manipulating VM flags if the attach
mechanism is used. In the Solaris case it turns on some DTrace flags.
The attach mechanism factors that into a pd_set_flags method that is
called for a given AttachOperation and so allows per platform behaviour.
But this is all part of the attach mechanism it isn't part of some
general flag management process.
I think I see the problem. Sorry it took me so long.
But, why the DTrace flags are only allowed to be set via the
attachListener? Can we allow their manipulation via com.sun.Flag? Or
they need to stay restricted to the attach mechanism only for whatever
reason?
inherent to ManagedFlags - so this refactoring seems wrong to me. What
exactly is ManagedFlag supposed to represent?
A shared functionality between attachListener.cpp, management.cpp and
the new diagnostic commands to be introduced later (as mentioned in the
original synopsis of this RFR). It did seem preferable over just copying
the implementation over to a few more places.
I need to see a clearer bigger picture. What I currently see doesn't
look right to me - attach mechanism functionality doesn't belong in a
general purpose ManagedFlags abstraction.
Bigger picture is that introducing yet another copy of the flag
management code for the purpose of adding the VM.set_flag diagnostic
command did seem unwieldy. The purpose of this refactoring was to get
the shared parts to one place.
-JB-
David
-
All the new code seems incorrect:
jint ManagedFlags::pd_set_flag(const char* flag_name, const char*
flag_value, Flag::Flags origin, outputStream* out) {
out-print_cr(flag '%s' cannot be changed, op-arg(0));
return JNI_ERR;
};
op-arg(0) comes from the original code where op was an
AttachOperation*. Here is should be using flag_name.
Correct. Slipped through and then replicated :/
And obviously never compiled. RFRs should be for tested code.
Yes, one should run always make clean first, just in case. I should
remember this well to prevent further embarrassment.
-JB-
David
-
Updated webrev: http://cr.openjdk.java.net/~jbachorik/8067447/webrev.01
-JB-
David
Thanks,
-JB-
RFR 8068591: javax/management/remote/mandatory/notif/NotifReconnectDeadlockTest.java should be quarantined
Hi, Please review this simple fix: bug: https://bugs.openjdk.java.net/browse/JDK-8068591 webrev: http://cr.openjdk.java.net/~sjiang/JDK-8068591/00/ Thanks, Shanliang
Re: RFR 8068591: javax/management/remote/mandatory/notif/NotifReconnectDeadlockTest.java should be quarantined
Hi Shanliang, On 8.1.2015 14:59, shanliang wrote: Hi, Please review this simple fix: bug: https://bugs.openjdk.java.net/browse/JDK-8068591 webrev: http://cr.openjdk.java.net/~sjiang/JDK-8068591/00/ The entry is missing the label (eg. generic-all) -JB- Thanks, Shanliang
Re: RFR 8068591: javax/management/remote/mandatory/notif/NotifReconnectDeadlockTest.java should be quarantined
Jaroslav Bachorik wrote: On 8.1.2015 15:35, shanliang wrote: Jaroslav Bachorik wrote: Hi Shanliang, On 8.1.2015 14:59, shanliang wrote: Hi, Please review this simple fix: bug: https://bugs.openjdk.java.net/browse/JDK-8068591 webrev: http://cr.openjdk.java.net/~sjiang/JDK-8068591/00/ The entry is missing the label (eg. generic-all) Oops! http://cr.openjdk.java.net/~sjiang/JDK-8068591/01/ Hm, shouldn't javax/management/remote/mandatory/connection/ReconnectTest.java be quarantined too? Maybe, but better to have a new sub-bug of JDK-8042215 to quarantine ReconnectTest.java? Shanliang -JB- Thanks, Shanliang -JB- Thanks, Shanliang
Re: RFR 8068591: javax/management/remote/mandatory/notif/NotifReconnectDeadlockTest.java should be quarantined
On 8.1.2015 17:37, shanliang wrote: Jaroslav Bachorik wrote: On 8.1.2015 15:35, shanliang wrote: Jaroslav Bachorik wrote: Hi Shanliang, On 8.1.2015 14:59, shanliang wrote: Hi, Please review this simple fix: bug: https://bugs.openjdk.java.net/browse/JDK-8068591 webrev: http://cr.openjdk.java.net/~sjiang/JDK-8068591/00/ The entry is missing the label (eg. generic-all) Oops! http://cr.openjdk.java.net/~sjiang/JDK-8068591/01/ Hm, shouldn't javax/management/remote/mandatory/connection/ReconnectTest.java be quarantined too? Maybe, but better to have a new sub-bug of JDK-8042215 to quarantine ReconnectTest.java? Well, it's up to you. But the error report is originally for ReconnectTest nd it already mixes those two failures together anyway so you could spare one round of one-liner review process. If you decide to to squeeze in the javax/management/remote/mandatory/connection/ReconnectTest.java exclusion just reply to this thread with the updated webrev. Otherwise this change is good to go. -JB- Shanliang -JB- Thanks, Shanliang -JB- Thanks, Shanliang
Re: [9] RFR (M) 8054888: Runtime: Add Diagnostic Command that prints the class hierarchy
Chris, Thank you for doing this. I had a couple of questions/comments. I like your idea of being able to start with a specific class to show all subclasses of. I think the way I read the code this shows the superclass hierarchy, not the superinterfaces. With the addition of default methods in interfaces, I think we have increased the value in seeing superinterfaces. So what I personally would find useful would be to be able to start with a specific class and find the superclasses and superinterfaces of that class - for the debugging I do, I usually am trying to look up and need both sets of information. Today if you run -XX:+TraceDefaultMethods there is one sample way to display all the supertypes of a single type, all the way up. I don't know the best way to make that consistent with the current output approach, e.g. using the |- syntax. e.g. Class java.util.Arrays$ArrayList requires default method processing java/util/Arrays$ArrayList java/util/AbstractList java/util/AbstractCollection java/lang/Object java/util/Collection java/lang/Object java/lang/Iterable java/lang/Object java/util/List java/lang/Object java/util/Collection java/lang/Object java/lang/Iterable java/lang/Object java/util/RandomAccess java/lang/Object java/io/Serializable java/lang/Object Do you think there could be value to others in the ability to walk up the hierarchy as well as to see superclasses and superinterfaces at least from that perspective? Is there value in printing the defining class loader for each class - maybe optionally? If so, I'm wondering if there might be value in future for the jigsaw project adding the module for each class - maybe optionally as well? Love opinions on that - especially from the serviceability folks thanks, Karen On Jan 7, 2015, at 6:29 PM, Chris Plummer wrote: Hi, Please review the following changes for the addition of the VM.class_hierarchy DCMD. Please read the bug first for some background information. Webrev: http://cr.openjdk.java.net/~cjplummer/8054888/webrev.00/ Bug: https://bugs.openjdk.java.net/browse/JDK-8054888 I expect there will be further restructuring or additional feature work. More discussion on that below. I'm not sure if that additional work will be done later with a separately bug filed or with this initial commit. That's one thing I want to work out with this review. Currently the bulk of the DCMD is implemented in heapInspection.cpp. The main purpose of this file is to implement the GC.class_stats and GC.class_histogram DCMDs. Both of them require walking the java heap to count live objects of each type, thus the name heapInspection.cpp. This new VM.class_hierarchy DCMD does not require walking the heap, but is implemented in this file because it leverages the existing KlassInfoTable and related classes (KlassInfoEntry, KlassInfoBucket, and KlassClosure). KlassInfoTable makes it easy to build a database of all loaded classes, save additional info gathered for each class, iterate over them quickly, and also do quick lookups. This exactly what I needed for this DCMD, thus the reuse. There is some downside to this. For starters, heapInspection.cpp is not the proper place for a DCMD that has nothing to do with heap inspection. Also, KlassInfoEntry is being overloaded now to support 3 different DCMDs, as is KlassInfoTable. As a result each has a few fields and methods that are not used for all 3 DCMDs. Some subclassing might be in order here, but I'm not sure if it's worth it. Opinions welcomed. If I am going to refactor, I would prefer that be done as a next step so I'm not disturbing the existing DCMDs with this first implementation. I added some comments to code only used for GC.class_stats and GC.class_histogram. I did this when trying to figure them out so I could better understand how to implement VM.class_hierarchy. I can take them out if you think they are not appropriate for this commit. One other item I like to discuss is whether it is worth adding a class name argument to this DCMD. That would cause just the superclasses and subclasses of the named class to be printed. If you think that is useful, I think it can be added without too much trouble. At the moment not much testing has been done other than running the DCMD and looking at the output. I'll do more once it's clear the code has settled. I would like to know if there are any existing tests for GC.class_stats and GC.class_histogram (there are none in the test directory). If so, possibly one could serve as the basis for a new test for VM.class_hierarchy. thanks, Chris
Re: RFR 8068591: javax/management/remote/mandatory/notif/NotifReconnectDeadlockTest.java should be quarantined
On 8.1.2015 15:35, shanliang wrote: Jaroslav Bachorik wrote: Hi Shanliang, On 8.1.2015 14:59, shanliang wrote: Hi, Please review this simple fix: bug: https://bugs.openjdk.java.net/browse/JDK-8068591 webrev: http://cr.openjdk.java.net/~sjiang/JDK-8068591/00/ The entry is missing the label (eg. generic-all) Oops! http://cr.openjdk.java.net/~sjiang/JDK-8068591/01/ Hm, shouldn't javax/management/remote/mandatory/connection/ReconnectTest.java be quarantined too? -JB- Thanks, Shanliang -JB- Thanks, Shanliang
Re: [9] RFR (M) 8054888: Runtime: Add Diagnostic Command that prints the class hierarchy
Hi Stefan,
Comments inline below:
On 1/8/15 2:50 AM, Stefan Karlsson wrote:
Hi
Chris,
On 2015-01-08 00:29, Chris Plummer wrote:
Hi,
Please review the following changes for the addition of the
VM.class_hierarchy DCMD. Please read the bug first for some
background information.
Webrev: http://cr.openjdk.java.net/~cjplummer/8054888/webrev.00/
Bug: https://bugs.openjdk.java.net/browse/JDK-8054888
This looks like a nice feature. I think your suggestion about
supporting a class name argument as the root of the hierarchy
would be a nice addition.
Some comments:
Why do you need / use the super_stack? To me it seems like you
could simplify the could if you get rid of the super_stack and
walk the Klass::super() chain instead.
The initial implementation actually printed the superclass index as
the indentation, which required the super_stack, and I just ended up
keeping it. It looks like I could get rid of all the superclass
maintenance done in
print_class_hierarchy() if I walk Klass:super() in print_class().
I'll make that change.
Why did you add this side-effect to
KlassInfoHisto::print_class_state?:
- super_index = super_e-index();
+ e-set_super_index(super_e-index());
AFAICT, you are not using KlassInfoHisto::print_class_stats to
implement the VM.class_hierarchy DCMD, right? Or am I missing
something in your patch?
Originally I did overload print_class_stats to also handle
VM.class_hierarchy, but in the end decided not to due to the
overhead of it causing the java heap to be walked. So the above is a
remnant, but is also consistent with how print_class_hierarchy()
sets the super_index field of the KlassInfoEntry. However, it may
not be necessary anymore to maintain the super_index if I make the
change above to no longer maintain the super_stack. I'll look into
that.
A side-note, if it were not for the AnonymousClasses (created by
Unsafe_DefineAnonymousClass), then this could have be implemented
with less resources by just walking the Klass::subclass() and
Klass::next_sibling() links. Which means that you wouldn't have to
use any of the classes or functionality in heapInspection.hpp/cpp.
But the anonymous classes is unfortunately not present in the
subclass/next_sibling hierarchy.
I didn't not realize subclass info was being maintained by Klass.
Still had CVM stuck in my head, which does not do that. I'm not sure
what the AnonymousClasses issue is. Can you explain more?
And some style comments:
http://cr.openjdk.java.net/~cjplummer/8054888/webrev.00/src/share/vm/services/diagnosticCommand.cpp.frames.html
Maybe it would be nice to move:
66 #if INCLUDE_SERVICES
67 DCmdFactory::register_DCmdFactory(new
DCmdFactoryImplClassHierarchyDCmd(full_export, true,
false));
68 #endif
into the already existing INCLUDE_SERVICE block:
55 #if INCLUDE_SERVICES // Heap dumping/inspection supported
56 DCmdFactory::register_DCmdFactory(new
DCmdFactoryImplHeapDumpDCmd(DCmd_Source_Internal |
DCmd_Source_AttachAPI, true, false));
57 DCmdFactory::register_DCmdFactory(new
DCmdFactoryImplClassHistogramDCmd(full_export, true,
false));
58 DCmdFactory::register_DCmdFactory(new
DCmdFactoryImplClassStatsDCmd(full_export, true, false));
59 #endif // INCLUDE_SERVICES
Ok.
http://cr.openjdk.java.net/~cjplummer/8054888/webrev.00/src/share/vm/memory/heapInspection.hpp.frames.html
I would prefer if you moved the following implementation to the
cpp file, so that we can keep our includes in our hpp files to a
minimal. That helps lowering our include complexity.
218 inline void KlassInfoEntry::add_subclass(KlassInfoEntry* cie)
{
219 if (_subclasses == NULL) {
220 _subclasses = new (ResourceObj::C_HEAP, mtInternal)
GrowableArrayKlassInfoEntry*(4, true);
221 }
222 _subclasses-append(cie);
223 }
224
225 inline KlassInfoEntry::~KlassInfoEntry() {
226 if (_subclasses != NULL) {
227 delete _subclasses;
228 }
229 }
I guess I'm a bit unclear on this,
Re: [9] RFR (M) 8054888: Runtime: Add Diagnostic Command that prints the class hierarchy
Hi Karen, Comments inline. On 1/8/15 8:07 AM, Karen Kinnear wrote: Chris, Thank you for doing this. I had a couple of questions/comments. I like your idea of being able to start with a specific class to show all subclasses of. Ok. I'll add that. I think the way I read the code this shows the superclass hierarchy, not the superinterfaces. With the addition of default methods in interfaces, I think we have increased the value in seeing superinterfaces. It does include interfaces in the output, but not as part of any class hierarchy. Interfaces are just shown as regular classes that inherit from Object. This is the case if one interface extends another, I suppose because extends is just interpreted as implements in this case. So what I personally would find useful would be to be able to start with a specific class and find the superclasses and superinterfaces of that class - for the debugging I do, I usually am trying to look up and need both sets of information. Today if you run -XX:+TraceDefaultMethods there is one sample way to display all the supertypes of a single type, all the way up. I don't know the best way to make that consistent with the current output approach, e.g. using the |- syntax. e.g. Class java.util.Arrays$ArrayList requires default method processing java/util/Arrays$ArrayList java/util/AbstractList java/util/AbstractCollection java/lang/Object java/util/Collection java/lang/Object java/lang/Iterable java/lang/Object java/util/List java/lang/Object java/util/Collection java/lang/Object java/lang/Iterable java/lang/Object java/util/RandomAccess java/lang/Object java/io/Serializable java/lang/Object Do you think there could be value to others in the ability to walk up the hierarchy as well as to see superclasses and superinterfaces at least from that perspective? This is a inverted from how my dcmd prints the hierarchy, plus also includes interfaces. Inverting the hierarchy means a class is listed with every subclass of the class, which I don't think is desirable. Including interfaces has the same issue, but introduces a new issue even when not inverting the hierarchy. The same interface can be in more than one location in the hierarchy, so there is no avoiding printing it more than once, so we need to decide how to best include interfaces in the output. The could just be a simple list right after the class that implements them: java.lang.Object | ... |--MyBaseClass | | implements - MyInterface1 | | implements - MyInterface2 | |--MySubClass | implements - MyInterface1 | implements - MyInterface2 | ... |--MyInterface1 |--MyInterface2 The implements lines could be optional. I know cvm would distinguish between interfaces the Class declared it implemented, and those it inherited from the interfaces it declared it implemented. This was necessary for reflection, and I think also to properly build up interfaces tables. I assume hotspot does something similar. Is there any need for this information to be conveyed in the above output, or just list out every interface implemented, and not worry about how the class acquired it. Is there value in printing the defining class loader for each class - maybe optionally? This is already available with GC.class_stats, although not in the default output. I suppose the reality is that it might be better handled by this DCMD. The main puprose of GC.class_stats is to print statistics (counts and sizes), so printing the ClassLoader name there is probably not appropriate, but then it's not really appropriate for VM.class_hierarchy either. I'm not sure how best to handle this. One or both DCMDs possibly should be re-purposed and more clearly define what type of data it displays. If so, I'm wondering if there might be value in future for the jigsaw project adding the module for each class - maybe optionally as well? This relates to my above statement. We need to figure out what type of data is useful, and which dcmds should handle them. Love opinions on that - especially from the serviceability folks thanks, Karen Thanks for the input. Chris On Jan 7, 2015, at 6:29 PM, Chris Plummer wrote: Hi, Please review the following changes for the addition of the VM.class_hierarchy DCMD. Please read the bug first for some background information. Webrev: http://cr.openjdk.java.net/~cjplummer/8054888/webrev.00/ Bug: https://bugs.openjdk.java.net/browse/JDK-8054888 I expect there will be further restructuring or additional feature work. More discussion on that below. I'm not sure if that additional work will be done later with a separately bug filed or with this initial commit. That's one thing I want to work out with this review. Currently the bulk of the DCMD is implemented in heapInspection.cpp. The main purpose of this file is to implement the GC.class_stats and
