Re: RFR 8066708: JMXStartStopTest fails to connect to port 38112
Stuart,
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?
This range is assigned by IANA so it's a standard.
-Dmitry
On 2015-01-09 05:50, Stuart Marks wrote:
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
Re: [9] RFR (M) 8054888: Runtime: Add Diagnostic Command that prints the class hierarchy
On 01/08/2015 10:29 PM, Chris Plummer wrote: 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. It seems to me that we have two very different use cases here, each one best served with a different output format: 1 - Listing of all classes/interfaces hierarchy when the dcmd is invoked without arguments: - Chris' output format as described below (with interfaces) 2 - Investigation on a particular class or interface when a class or interface is passed in argument to the dcmd - Karen's output format, much easier to work with to track default methods. Because the output is limited to the hierarchy from a single class, there's no class duplication in output (single parent class inheritance) and limited interfaces duplication. If the implementations of the two features are too different, we could consider having two different dcmds. My 2 cents, Fred 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
RE: RFR(XXS): 8068584: Compiler attach tests should be quarantined
Looks good. Thanks, Christian -Original Message- From: serviceability-dev [mailto:serviceability-dev-boun...@openjdk.java.net] On Behalf Of Mattias Tobiasson Sent: Thursday, January 8, 2015 7:45 AM To: Mikael Auno; serviceability-dev@openjdk.java.net Subject: Re: RFR(XXS): 8068584: Compiler attach tests should be quarantined Thanks for the explanation. I have moved the @ignore tags to directly before the first @run tag. webrev: http://cr.openjdk.java.net/~ykantser/8068584/webrev.01/ http://cr.openjdk.java.net/%7Eykantser/8068584/webrev.01/ Mattias On 01/08/2015 11:21 AM, Mikael Auno wrote: On 2015-01-08 11:04, Mattias Tobiasson wrote: 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 Mattias, In the two **/Launcher.java tests, you must place @ignore after the @library line, otherwise there will be a Parse Exception: `@library' must appear before first `@run'. (The error message is, to say the least, a bit unintuitive until you figure out that @ignore is just an alias for @run ignore.) Mikael
Re: [9] RFR (M) 8054888: Runtime: Add Diagnostic Command that prints the class hierarchy
Staffan, On Jan 9, 2015, at 7:38 AM, Staffan Larsen wrote: It’s getting difficult to get all the information into the same output: hierarchy, interfaces, class loaders and modules. I took a stab at it and it could look like this: java.lang.Object |--java.io.Serializable (java.base, 0x0007c00375f8, iface) |--java.util.RandomAccess (java.base, 0x0007c00375f8, iface) |--java.lang.Iterable (java.base, 0x0007c00375f8, iface) | |--java.util.Collection (java.base, 0x0007c00375f8, iface) | | |--java.util.List (java.base, 0x0007c00375f8, iface) |--java.util.AbstractCollection (java.base, 0x0007c00375f8) | | implements java.util.Collection | | implements java.lang.Iterable | |--java.util.AbstractList (java.base, 0x0007c00375f8) | |implements java.util.List | | |--java.util.Arrays$ArrayList (java.base, 0x0007c00375f8) | | |implements java.io.Serializable | | |implements java.util.RandomAccess But this is pushing what can be visualized in one place. We will need to add module and class loader information somehow to all the Diagnostic Commands that list classes. In addition we will need a way to see how modules relate to one another. Perhaps it isn’t possible to have all the information in one place, but instead make it possible to cross reference between different diagnostic commands. For example, GC.class_stats could have the module and class loader information, and GC.class_hierarchy would not have to include it. What is missing from making that possible is a unique way of identifying a class (since the name is not unique). All output would need to include that unique identifier and it would be possible to cross reference. The identifier has to be stable during a JVM run, but not between runs. I like where you are going with this. Since a given module is within a class loader, putting the the loader/module/class information in class_stats would be helpful and then having other dcmds just need the class/class loader pair for uniqueness. Not sure what you are using below for unique identifier. I would be tempted to use the class loader hex value since otherwise you are introducing hex values that have no meaning other than as cross-references. And the class/class loader pair is guaranteed uniqueness. For any class loader you could list its hex value thereby giving the information in a single command that gives meaning to the loader value. java.lang.Object/null ... |-sun.misc.Launcher$AppClassLoader/null (0xyyy) |-myapp/0xyyy Just a thought. If you are not enthused - I am ok with your proposal. thanks, Karen The above would then become: java.lang.Object/0x12345600 |--java.io.Serializable/0x12345601 |--java.util.RandomAccess/0x12345602 |--java.lang.Iterable/0x12345603 | |--java.util.Collection/0x12345604 | | |--java.util.List/0x12345605 |--java.util.AbstractCollection/0x12345606 | | implements java.util.Collection/0x12345604 | | implements java.lang.Iterable/0x12345603 | |--java.util.AbstractList/0x12345607 | |implements java.util.List/0x12345605 | | |--java.util.Arrays$ArrayList/0x12345608 | | |implements java.io.Serializable/0x12345601 | | |implements java.util.RandomAccess/0x12345602 With additions to GC.class_stats: Index Super ClassLoaderClassName 1-1 0x0007c0034c48 java.lang.Object/0x12345600 2 1 0x0007c0034c48 java.util.List/0x12345605 331 0x0007c0034c48 java.util.AbstractList/0x12345607 And GC.class_histogram: num #instances #bytes class name -- 1: 945 117736 java.lang.Object/0x12345600 2: 442 50352 java.util.List/0x12345605 3: 499 25288 java.util.AbstractList/0x12345607 /Staffan On 9 jan 2015, at 09:53, Frederic Parain frederic.par...@oracle.com wrote: On 01/08/2015 10:29 PM, Chris Plummer wrote: 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
Re: [9] RFR (M) 8054888: Runtime: Add Diagnostic Command that prints the class hierarchy
On 9 jan 2015, at 18:49, Karen Kinnear karen.kinn...@oracle.com wrote:
Staffan,
On Jan 9, 2015, at 7:38 AM, Staffan Larsen wrote:
It’s getting difficult to get all the information into the same output:
hierarchy, interfaces, class loaders and modules. I took a stab at it and it
could look like this:
java.lang.Object
|--java.io.Serializable (java.base, 0x0007c00375f8, iface)
|--java.util.RandomAccess (java.base, 0x0007c00375f8, iface)
|--java.lang.Iterable (java.base, 0x0007c00375f8, iface)
| |--java.util.Collection (java.base, 0x0007c00375f8, iface)
| | |--java.util.List (java.base, 0x0007c00375f8, iface)
|--java.util.AbstractCollection (java.base, 0x0007c00375f8)
| | implements java.util.Collection
| | implements java.lang.Iterable
| |--java.util.AbstractList (java.base, 0x0007c00375f8)
| |implements java.util.List
| | |--java.util.Arrays$ArrayList (java.base, 0x0007c00375f8)
| | |implements java.io.Serializable
| | |implements java.util.RandomAccess
But this is pushing what can be visualized in one place.
We will need to add module and class loader information somehow to all the
Diagnostic Commands that list classes. In addition we will need a way to see
how modules relate to one another. Perhaps it isn’t possible to have all the
information in one place, but instead make it possible to cross reference
between different diagnostic commands. For example, GC.class_stats could
have the module and class loader information, and GC.class_hierarchy would
not have to include it. What is missing from making that possible is a
unique way of identifying a class (since the name is not unique). All output
would need to include that unique identifier and it would be possible to
cross reference. The identifier has to be stable during a JVM run, but not
between runs.
I like where you are going with this. Since a given module is within a class
loader, putting the the loader/module/class information in class_stats would
be helpful and then having
other dcmds just need the class/class loader pair for uniqueness.
Not sure what you are using below for unique identifier.
I would be tempted to use the class loader hex value since otherwise you are
introducing hex values that have no meaning other than as
cross-references. And the class/class loader pair is guaranteed uniqueness.
I was actually thinking of a unique identifier for a class (the Klass* for
example), not a {class name, class loader} combo, but as you say, the {class
name, class loader} combo is also unique and gives more useful information.
For any class loader you could list its hex value thereby giving the
information in a single command that gives meaning to the loader value.
java.lang.Object/null
...
|-sun.misc.Launcher$AppClassLoader/null (0xyyy)
|-myapp/0xyyy
Just a thought. If you are not enthused - I am ok with your proposal.
This looks good to me.
/Staffan
thanks,
Karen
The above would then become:
java.lang.Object/0x12345600
|--java.io.Serializable/0x12345601
|--java.util.RandomAccess/0x12345602
|--java.lang.Iterable/0x12345603
| |--java.util.Collection/0x12345604
| | |--java.util.List/0x12345605
|--java.util.AbstractCollection/0x12345606
| | implements java.util.Collection/0x12345604
| | implements java.lang.Iterable/0x12345603
| |--java.util.AbstractList/0x12345607
| |implements java.util.List/0x12345605
| | |--java.util.Arrays$ArrayList/0x12345608
| | |implements java.io.Serializable/0x12345601
| | |implements java.util.RandomAccess/0x12345602
With additions to GC.class_stats:
Index Super ClassLoaderClassName
1-1 0x0007c0034c48 java.lang.Object/0x12345600
2 1 0x0007c0034c48 java.util.List/0x12345605
331 0x0007c0034c48 java.util.AbstractList/0x12345607
And GC.class_histogram:
num #instances #bytes class name
--
1: 945 117736 java.lang.Object/0x12345600
2: 442 50352 java.util.List/0x12345605
3: 499 25288 java.util.AbstractList/0x12345607
/Staffan
On 9 jan 2015, at 09:53, Frederic Parain frederic.par...@oracle.com
mailto:frederic.par...@oracle.com wrote:
On 01/08/2015 10:29 PM, Chris Plummer wrote:
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
Re: [9] RFR (M) 8054888: Runtime: Add Diagnostic Command that prints the class hierarchy
Thanks Frederic for suggesting two different dcmds - they could share a lot of the code logic. If folks generally prefer these as separate dcmds - I can file an rfe to add the inverted one - i.e. start at a given class/interface and tell me its supertypes. thanks, Karen On Jan 9, 2015, at 3:53 AM, Frederic Parain wrote: On 01/08/2015 10:29 PM, Chris Plummer wrote: 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. It seems to me that we have two very different use cases here, each one best served with a different output format: 1 - Listing of all classes/interfaces hierarchy when the dcmd is invoked without arguments: - Chris' output format as described below (with interfaces) 2 - Investigation on a particular class or interface when a class or interface is passed in argument to the dcmd - Karen's output format, much easier to work with to track default methods. Because the output is limited to the hierarchy from a single class, there's no class duplication in output (single parent class inheritance) and limited interfaces duplication. If the implementations of the two features are too different, we could consider having two different dcmds. My 2 cents, Fred 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
Re: [9] RFR (M) 8054888: Runtime: Add Diagnostic Command that prints the class hierarchy
On 1/9/15 4:38 AM, Staffan Larsen wrote: It�s getting difficult to get all the information into the same output: hierarchy, interfaces, class loaders and modules. I took a stab at it and it could look like this: java.lang.Object |--java.io.Serializable (java.base, 0x0007c00375f8, iface) |--java.util.RandomAccess (java.base, 0x0007c00375f8, iface) |--java.lang.Iterable (java.base, 0x0007c00375f8, iface) | |--java.util.Collection (java.base, 0x0007c00375f8, iface) | | |--java.util.List (java.base, 0x0007c00375f8, iface) |--java.util.AbstractCollection (java.base, 0x0007c00375f8) | | implements java.util.Collection | | implements java.lang.Iterable | |--java.util.AbstractList (java.base, 0x0007c00375f8) | | implements java.util.List | | |--java.util.Arrays$ArrayList (java.base, 0x0007c00375f8) | | | implements java.io.Serializable | | | implements java.util.RandomAccess For the most part I like this. The one part I'm not too fond of is hex long which I assume represents the ClassLoader. Maybe we could just use a simple index, and at the end of the dump include a table of the referenced ClassLoaders. Possibly just hijack what VM.classloader_stats outputs, but include the simple index in the first column. But this is pushing what can be visualized in one place. We will need to add module and class loader information somehow to all the Diagnostic Commands that list classes. In addition we will need a way to see how modules relate to one another. Perhaps it isn�t possible to have all the information in one place, but instead make it possible to cross reference between different diagnostic commands. For example, GC.class_stats could have the module and class loader information, and GC.class_hierarchy would not have to include it. What is missing from making that possible is a unique way of identifying a class (since the name is not unique). All output would need to include that unique identifier and it would be possible to cross reference. The identifier has to be stable during a JVM run, but not between runs. I don't know if there is some sort of simple stable ID for a class other than the address of the Klass instance. And of course this assumes there isn't any class unloading going on that could result in reuse of the address. Is that acceptable? The above would then become: java.lang.Object/0x12345600 |--java.io.Serializable/0x12345601 |--java.util.RandomAccess/0x12345602 |--java.lang.Iterable/0x12345603 | |--java.util.Collection/0x12345604 | | |--java.util.List/0x12345605 |--java.util.AbstractCollection/0x12345606 | | implements java.util.Collection/0x12345604 | | implements java.lang.Iterable/0x12345603 | |--java.util.AbstractList/0x12345607 | | implements java.util.List/0x12345605 | | |--java.util.Arrays$ArrayList/0x12345608 | | | implements java.io.Serializable/0x12345601 | | | implements java.util.RandomAccess/0x12345602 With additions to GC.class_stats: Index Super ClassLoader ClassName 1 -1 0x0007c0034c48 java.lang.Object/0x12345600 2 1 0x0007c0034c48 java.util.List/0x12345605 3 31 0x0007c0034c48 java.util.AbstractList/0x12345607 And GC.class_histogram: num #instances #bytes class name -- 1: 945 117736 java.lang.Object/0x12345600 2: 442 50352 java.util.List/0x12345605 3: 499 25288 java.util.AbstractList/0x12345607 Would it be better for the class identifier to be in a separate column from the class name, or is combining them intentional? In any
Re: [9] RFR (M) 8054888: Runtime: Add Diagnostic Command that prints the class hierarchy
On 1/9/15 4:38 AM, Staffan Larsen wrote: It�s getting difficult to get all the information into the same output: hierarchy, interfaces, class loaders and modules. I took a stab at it and it could look like this: java.lang.Object |--java.io.Serializable (java.base, 0x0007c00375f8, iface) |--java.util.RandomAccess (java.base, 0x0007c00375f8, iface) |--java.lang.Iterable (java.base, 0x0007c00375f8, iface) | |--java.util.Collection (java.base, 0x0007c00375f8, iface) | | |--java.util.List (java.base, 0x0007c00375f8, iface) |--java.util.AbstractCollection (java.base, 0x0007c00375f8) | | implements java.util.Collection | | implements java.lang.Iterable | |--java.util.AbstractList (java.base, 0x0007c00375f8) | | implements java.util.List | | |--java.util.Arrays$ArrayList (java.base, 0x0007c00375f8) | | | implements java.io.Serializable | | | implements java.util.RandomAccess For the most part I like this. The one part I'm not too fond of is hex long which I assume represents the ClassLoader. Maybe we could just use a simple index, and at the end of the dump include a table of the referenced ClassLoaders. Possibly just hijack what VM.classloader_stats outputs, but include the simple index in the first column. But this is pushing what can be visualized in one place. We will need to add module and class loader information somehow to all the Diagnostic Commands that list classes. In addition we will need a way to see how modules relate to one another. Perhaps it isn�t possible to have all the information in one place, but instead make it possible to cross reference between different diagnostic commands. For example, GC.class_stats could have the module and class loader information, and GC.class_hierarchy would not have to include it. What is missing from making that possible is a unique way of identifying a class (since the name is not unique). All output would need to include that unique identifier and it would be possible to cross reference. The identifier has to be stable during a JVM run, but not between runs. I don't know if there is some sort of simple stable ID for a class other than the address of the Klass instance. And of course this assumes there isn't any class unloading going on that could result in reuse of the address. Is that acceptable? The above would then become: java.lang.Object/0x12345600 |--java.io.Serializable/0x12345601 |--java.util.RandomAccess/0x12345602 |--java.lang.Iterable/0x12345603 | |--java.util.Collection/0x12345604 | | |--java.util.List/0x12345605 |--java.util.AbstractCollection/0x12345606 | | implements java.util.Collection/0x12345604 | | implements java.lang.Iterable/0x12345603 | |--java.util.AbstractList/0x12345607 | | implements java.util.List/0x12345605 | | |--java.util.Arrays$ArrayList/0x12345608 | | | implements java.io.Serializable/0x12345601 | | | implements java.util.RandomAccess/0x12345602 With additions to GC.class_stats: Index Super ClassLoader ClassName 1 -1 0x0007c0034c48 java.lang.Object/0x12345600 2 1 0x0007c0034c48 java.util.List/0x12345605 3 31 0x0007c0034c48 java.util.AbstractList/0x12345607 And GC.class_histogram: num #instances #bytes class name -- 1: 945 117736 java.lang.Object/0x12345600 2: 442 50352 java.util.List/0x12345605 3: 499 25288 java.util.AbstractList/0x12345607 Would it be better for the class identifier to be in a separate column from the class name, or is combining them intentional? In any
Re: [9] RFR (M) 8054888: Runtime: Add Diagnostic Command that prints the class hierarchy
On 2015-01-08 20:15, Chris Plummer wrote:
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?
The AnonymousClasses are supposed to be lightweight classes used by
JSR292. They have a different lifecycle than ordinary Klasses, and are
not registered in some of the data structures where the ordinary Klasses
are registered, and one example is the subclass/next_sibling tree.
Because these classes treated differently, they are a constant source of
bugs. If you visit Klasses by iterating over a data structure in the
JVM, you need to know if the AnonymousClasses are present our not. Your
current code is safe because you walk the ClassLoaderDataGraph, which
contains the AnonymousKlasses. But a simplified version relying on the
subclass/next_sibling tree would unfortunately be broken. The
SystemDictionary is another place where the AnonymousClasses are not
registered.
Note, that the term anonymous class is an overloaded term and I'm not
referring to this kind of anonymous classes:
http://docs.oracle.com/javase/tutorial/java/javaOO/anonymousclasses.html
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
Re: RFR(XXS): 8068584: Compiler attach tests should be quarantined
Looks good to me. I'm not a reviewer though. Mikael On 2015-01-08 13:45, Mattias Tobiasson wrote: Thanks for the explanation. I have moved the @ignore tags to directly before the first @run tag. webrev: http://cr.openjdk.java.net/~ykantser/8068584/webrev.01/ http://cr.openjdk.java.net/%7Eykantser/8068584/webrev.01/ Mattias On 01/08/2015 11:21 AM, Mikael Auno wrote: On 2015-01-08 11:04, Mattias Tobiasson wrote: 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 Mattias, In the two **/Launcher.java tests, you must place @ignore after the @library line, otherwise there will be a Parse Exception: `@library' must appear before first `@run'. (The error message is, to say the least, a bit unintuitive until you figure out that @ignore is just an alias for @run ignore.) Mikael
Re: RFR 8066708: JMXStartStopTest fails to connect to port 38112
Thank you all for the valuable input!
On 9.1.2015 03:50, Stuart Marks wrote:
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.)
I've changed the PortAllocator to allocate an array of unique random
ports instead of letting ServerSocket to take care of it.
I ran the test 200x in a tight loop without a failure.
I hope this will resolve this test's intermittent failures due to port
conflicts once and for all.
Update: http://cr.openjdk.java.net/~jbachorik/8066708/webrev.03
Thanks,
-JB-
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:
RFR(XXS): 8068718: com/sun/jdi/CatchPatternTest.sh should be quarantined
Hi, Could I please have a review of addition to ProblemList. bug: https://bugs.openjdk.java.net/browse/JDK-8068718 webrev: http://cr.openjdk.java.net/~miauno/8068718/webrev.00/ Thanks, Mikael
Re: RFR(XXS): 8068718: com/sun/jdi/CatchPatternTest.sh should be quarantined
Looks good! -JB- On 9.1.2015 13:54, Mikael Auno wrote: Hi, Could I please have a review of addition to ProblemList. bug: https://bugs.openjdk.java.net/browse/JDK-8068718 webrev: http://cr.openjdk.java.net/~miauno/8068718/webrev.00/ Thanks, Mikael
Re: [9] RFR (M) 8054888: Runtime: Add Diagnostic Command that prints the class hierarchy
It’s getting difficult to get all the information into the same output: hierarchy, interfaces, class loaders and modules. I took a stab at it and it could look like this: java.lang.Object |--java.io.Serializable (java.base, 0x0007c00375f8, iface) |--java.util.RandomAccess (java.base, 0x0007c00375f8, iface) |--java.lang.Iterable (java.base, 0x0007c00375f8, iface) | |--java.util.Collection (java.base, 0x0007c00375f8, iface) | | |--java.util.List (java.base, 0x0007c00375f8, iface) |--java.util.AbstractCollection (java.base, 0x0007c00375f8) | | implements java.util.Collection | | implements java.lang.Iterable | |--java.util.AbstractList (java.base, 0x0007c00375f8) | |implements java.util.List | | |--java.util.Arrays$ArrayList (java.base, 0x0007c00375f8) | | |implements java.io.Serializable | | |implements java.util.RandomAccess But this is pushing what can be visualized in one place. We will need to add module and class loader information somehow to all the Diagnostic Commands that list classes. In addition we will need a way to see how modules relate to one another. Perhaps it isn’t possible to have all the information in one place, but instead make it possible to cross reference between different diagnostic commands. For example, GC.class_stats could have the module and class loader information, and GC.class_hierarchy would not have to include it. What is missing from making that possible is a unique way of identifying a class (since the name is not unique). All output would need to include that unique identifier and it would be possible to cross reference. The identifier has to be stable during a JVM run, but not between runs. The above would then become: java.lang.Object/0x12345600 |--java.io.Serializable/0x12345601 |--java.util.RandomAccess/0x12345602 |--java.lang.Iterable/0x12345603 | |--java.util.Collection/0x12345604 | | |--java.util.List/0x12345605 |--java.util.AbstractCollection/0x12345606 | | implements java.util.Collection/0x12345604 | | implements java.lang.Iterable/0x12345603 | |--java.util.AbstractList/0x12345607 | |implements java.util.List/0x12345605 | | |--java.util.Arrays$ArrayList/0x12345608 | | |implements java.io.Serializable/0x12345601 | | |implements java.util.RandomAccess/0x12345602 With additions to GC.class_stats: Index Super ClassLoaderClassName 1-1 0x0007c0034c48 java.lang.Object/0x12345600 2 1 0x0007c0034c48 java.util.List/0x12345605 331 0x0007c0034c48 java.util.AbstractList/0x12345607 And GC.class_histogram: num #instances #bytes class name -- 1: 945 117736 java.lang.Object/0x12345600 2: 442 50352 java.util.List/0x12345605 3: 499 25288 java.util.AbstractList/0x12345607 /Staffan On 9 jan 2015, at 09:53, Frederic Parain frederic.par...@oracle.com wrote: On 01/08/2015 10:29 PM, Chris Plummer wrote: 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
Re: RFR(XXS): 8068718: com/sun/jdi/CatchPatternTest.sh should be quarantined
Thanks for the review! Mikael On 2015-01-09 13:59, Jaroslav Bachorik wrote: Looks good! -JB- On 9.1.2015 13:54, Mikael Auno wrote: Hi, Could I please have a review of addition to ProblemList. bug: https://bugs.openjdk.java.net/browse/JDK-8068718 webrev: http://cr.openjdk.java.net/~miauno/8068718/webrev.00/ Thanks, Mikael
