Re: RFR (M) 8067662: "java.lang.NullPointerException: Method name is null" from StackTraceElement.
On 3/19/15 11:11 AM, Daniel D. Daugherty wrote: On 3/12/15 11:27 PM, serguei.spit...@oracle.com wrote: Please, review the jdk 9 fix for: https://bugs.openjdk.java.net/browse/JDK-8067662 Open hotspot webrev: http://cr.openjdk.java.net/~sspitsyn/webrevs/2015/hotspot/8067662-JVMTI-trace.1/ Thumbs up. None of these comments are critical. Thanks a lot for the review, Dan! src/share/vm/classfile/javaClasses.hpp No comments. src/share/vm/classfile/javaClasses.cpp line 1316: return method != NULL && (method->constants()->version() == version line 1317: && version < MAX_VERSION); Don't really need the parens and the indenting implies a logical grouping that isn't really there. Perhaps: return method != NULL && method->constants()->version() == version && version < MAX_VERSION; Although I also wonder why the caller would pass a 'version' value that is >= MAX_VERSION. Perhaps this instead: assert(version < MAX_VERSION, "version is too big"); return method != NULL && method->constants()->version() == version line 1347: typeArrayOop_cprefs; Perhaps add a comment: // needed to insulate method name against redefinition line 1486: holder = holder->get_klass_version(version); // Use specific ik version as a holder Perhaps the following comment instead: // Use specific ik version as a holder since the mirror might // refer to version that is now obsolete. I'm not sure "obsolete" is the right term here, but say something about why the mirror version is not good enough. Update: 'obsolete' is the right term now that I've re-read the code review invite! Perhaps this is better: // Use specific ik version as a holder since the mirror might // refer to version that is now obsolete and no longer accessible // via the previous versions list. line 1854: // Get method id, cpref, bci, version and mirror from chunk Perhaps: // Get method id, bci, version, mirror and cpref from chunk since that's the order you do the work... line 1909: holder = holder->get_klass_version(version); // Use specific ik version as a holder Similar comment as on line 1486. src/share/vm/oops/instanceKlass.hpp No comments. src/share/vm/oops/instanceKlass.cpp No comments. Open webrev for unit test update: http://cr.openjdk.java.net/~sspitsyn/webrevs/2015/jdk/8067662-JVMTI-trace-test.1/ test/java/lang/instrument/RedefineMethodInBacktrace.sh No comments. test/java/lang/instrument/RedefineMethodInBacktraceApp.java line 175: System.exit(1); A comment to explain why this test needs to exit on failure would be good. When the work to get rid of shell script driven tests gets here, it would be useful for understanding why this test must be standalone. All of the above suggestions are good. Will update before the push. Thanks! test/java/lang/instrument/RedefineMethodInBacktraceTarget.java No comments. test/java/lang/instrument/RedefineMethodInBacktraceTargetB.java No comments. test/java/lang/instrument/RedefineMethodInBacktraceTargetB_2.java No comments. test/java/lang/instrument/RedefineMethodInBacktraceTarget_2.java No commetns. Thanks! Serguei Dan Summary: An NPE is trown in a thread dumping via JMX when doing CPU profiling in NetBeans Profiler and VisualVM. The issue is that the methods and related klass versions are not kept alive if a class redefinition takes place between catching a Throwable and taking a thread dump. It can result in loosing an obsolete method, and so, the reconstruction of method name becomes impossible. In such a case, the null reference is returned instead of a valid method name. The solution is to use current klass version and method orig_idnum instead of ordinary method idnum to find required method pointer. In the worst case when the related klass version is gone (was not included to or was removed from the previous_versions linked list), a saved method name CP index of the latest klass version can be used to restore the method name. The footprint extra overhead for this approach is u2 per stack frame. The plan is also to backport the fix to 8u60. Testing: In progress: nsk redefine classes tests, JTREG java/lang/instrument Thanks, Serguei
Re: RFR (M) 8067662: "java.lang.NullPointerException: Method name is null" from StackTraceElement.
On 3/13/15, 2:37 PM, Coleen Phillimore wrote: On 3/13/15, 2:36 PM, serguei.spit...@oracle.com wrote: Coleen, Thank you for review and great idea to use the method name cp index! However, this approach is going stop working if we get rid of the CP merge in the future. Yes, it will. I was just thinking about how to resolve that. We'll have to brainstorm this in the summer. There's still a huge snowbank outside my window, but I've been thinking about this. I have been working on the enhanced class redefinition sources. This is how we can resolve the stack trace problem, which is better than what we have today: When we do a class redefinition with enhanced class redefinition, we walk the heap to replace oop->_klass fields with the new classes. We replace a few other things too. We can change backtraces to have the Method* again and when we walk the heap during redefinition, we can also build a GrowableArray of all Method* found in the backtraces. This GrowableArray of Method* can be used to mark the Method* on_stack so that they are not deallocated during class unloading. If the method isn't deallocated, neither is the old class where it came from. This is good because we can get the line number at bci and source file name also from the old Method*. Coleen Coleen Thanks, Serguei On 3/13/15 10:40 AM, Coleen Phillimore wrote: Serguei, This looks good. This works a lot harder to find the original method than the code used to. Thanks, Coleen On 3/13/15, 1:27 AM, serguei.spit...@oracle.com wrote: Please, review the jdk 9 fix for: https://bugs.openjdk.java.net/browse/JDK-8067662 Open hotspot webrev: http://cr.openjdk.java.net/~sspitsyn/webrevs/2015/hotspot/8067662-JVMTI-trace.1/ Open webrev for unit test update: http://cr.openjdk.java.net/~sspitsyn/webrevs/2015/jdk/8067662-JVMTI-trace-test.1/ Summary: An NPE is trown in a thread dumping via JMX when doing CPU profiling in NetBeans Profiler and VisualVM. The issue is that the methods and related klass versions are not kept alive if a class redefinition takes place between catching a Throwable and taking a thread dump. It can result in loosing an obsolete method, and so, the reconstruction of method name becomes impossible. In such a case, the null reference is returned instead of a valid method name. The solution is to use current klass version and method orig_idnum instead of ordinary method idnum to find required method pointer. In the worst case when the related klass version is gone (was not included to or was removed from the previous_versions linked list), a saved method name CP index of the latest klass version can be used to restore the method name. The footprint extra overhead for this approach is u2 per stack frame. The plan is also to backport the fix to 8u60. Testing: In progress: nsk redefine classes tests, JTREG java/lang/instrument Thanks, Serguei
Re: RFR (M) 8067662: "java.lang.NullPointerException: Method name is null" from StackTraceElement.
On 3/12/15 11:27 PM, serguei.spit...@oracle.com wrote: Please, review the jdk 9 fix for: https://bugs.openjdk.java.net/browse/JDK-8067662 Open hotspot webrev: http://cr.openjdk.java.net/~sspitsyn/webrevs/2015/hotspot/8067662-JVMTI-trace.1/ Thumbs up. None of these comments are critical. src/share/vm/classfile/javaClasses.hpp No comments. src/share/vm/classfile/javaClasses.cpp line 1316: return method != NULL && (method->constants()->version() == version line 1317: && version < MAX_VERSION); Don't really need the parens and the indenting implies a logical grouping that isn't really there. Perhaps: return method != NULL && method->constants()->version() == version && version < MAX_VERSION; Although I also wonder why the caller would pass a 'version' value that is >= MAX_VERSION. Perhaps this instead: assert(version < MAX_VERSION, "version is too big"); return method != NULL && method->constants()->version() == version line 1347: typeArrayOop_cprefs; Perhaps add a comment: // needed to insulate method name against redefinition line 1486: holder = holder->get_klass_version(version); // Use specific ik version as a holder Perhaps the following comment instead: // Use specific ik version as a holder since the mirror might // refer to version that is now obsolete. I'm not sure "obsolete" is the right term here, but say something about why the mirror version is not good enough. Update: 'obsolete' is the right term now that I've re-read the code review invite! Perhaps this is better: // Use specific ik version as a holder since the mirror might // refer to version that is now obsolete and no longer accessible // via the previous versions list. line 1854: // Get method id, cpref, bci, version and mirror from chunk Perhaps: // Get method id, bci, version, mirror and cpref from chunk since that's the order you do the work... line 1909: holder = holder->get_klass_version(version); // Use specific ik version as a holder Similar comment as on line 1486. src/share/vm/oops/instanceKlass.hpp No comments. src/share/vm/oops/instanceKlass.cpp No comments. Open webrev for unit test update: http://cr.openjdk.java.net/~sspitsyn/webrevs/2015/jdk/8067662-JVMTI-trace-test.1/ test/java/lang/instrument/RedefineMethodInBacktrace.sh No comments. test/java/lang/instrument/RedefineMethodInBacktraceApp.java line 175: System.exit(1); A comment to explain why this test needs to exit on failure would be good. When the work to get rid of shell script driven tests gets here, it would be useful for understanding why this test must be standalone. test/java/lang/instrument/RedefineMethodInBacktraceTarget.java No comments. test/java/lang/instrument/RedefineMethodInBacktraceTargetB.java No comments. test/java/lang/instrument/RedefineMethodInBacktraceTargetB_2.java No comments. test/java/lang/instrument/RedefineMethodInBacktraceTarget_2.java No commetns. Dan Summary: An NPE is trown in a thread dumping via JMX when doing CPU profiling in NetBeans Profiler and VisualVM. The issue is that the methods and related klass versions are not kept alive if a class redefinition takes place between catching a Throwable and taking a thread dump. It can result in loosing an obsolete method, and so, the reconstruction of method name becomes impossible. In such a case, the null reference is returned instead of a valid method name. The solution is to use current klass version and method orig_idnum instead of ordinary method idnum to find required method pointer. In the worst case when the related klass version is gone (was not included to or was removed from the previous_versions linked list), a saved method name CP index of the latest klass version can be used to restore the method name. The footprint extra overhead for this approach is u2 per stack frame. The plan is also to backport the fix to 8u60. Testing: In progress: nsk redefine classes tests, JTREG java/lang/instrument Thanks, Serguei
Re: RFR (M) 8067662: "java.lang.NullPointerException: Method name is null" from StackTraceElement.
I need another reviewer for this. Thanks, Serguei On 3/13/15 10:40 AM, Coleen Phillimore wrote: Serguei, This looks good. This works a lot harder to find the original method than the code used to. Thanks, Coleen On 3/13/15, 1:27 AM, serguei.spit...@oracle.com wrote: Please, review the jdk 9 fix for: https://bugs.openjdk.java.net/browse/JDK-8067662 Open hotspot webrev: http://cr.openjdk.java.net/~sspitsyn/webrevs/2015/hotspot/8067662-JVMTI-trace.1/ Open webrev for unit test update: http://cr.openjdk.java.net/~sspitsyn/webrevs/2015/jdk/8067662-JVMTI-trace-test.1/ Summary: An NPE is trown in a thread dumping via JMX when doing CPU profiling in NetBeans Profiler and VisualVM. The issue is that the methods and related klass versions are not kept alive if a class redefinition takes place between catching a Throwable and taking a thread dump. It can result in loosing an obsolete method, and so, the reconstruction of method name becomes impossible. In such a case, the null reference is returned instead of a valid method name. The solution is to use current klass version and method orig_idnum instead of ordinary method idnum to find required method pointer. In the worst case when the related klass version is gone (was not included to or was removed from the previous_versions linked list), a saved method name CP index of the latest klass version can be used to restore the method name. The footprint extra overhead for this approach is u2 per stack frame. The plan is also to backport the fix to 8u60. Testing: In progress: nsk redefine classes tests, JTREG java/lang/instrument Thanks, Serguei
Re: RFR 8054890: Serviceability: New diagnostic commands 'VM.set_flag' and 'JVMTI.data_dump'
Jaroslav, src/share/vm/services/diagnosticCommand.cpp No comments src/share/vm/services/diagnosticCommand.hpp Why adding #include "services/attachListener.hpp" ? test/serviceability/dcmd/jvmti/DataDumpDcmdTest.java test/serviceability/dcmd/vm/SetVMFlagTest.java I don't know the new test framework, so I cannot comment on these files. Regards, Fred On 03/19/2015 10:59 AM, Jaroslav Bachorik wrote: Please, review the following change Issue : https://bugs.openjdk.java.net/browse/JDK-8054890 Webrev: http://cr.openjdk.java.net/~jbachorik/8054890/webrev.00 This patch is about adding 2 new diagnostic commands - VM.set_flag and JVMTI.data_dump. VM.set_flag allows to set any writeable flag. It takes the flag name and the flag value in textual form. The mutability of the flag and the value format checks are forwarded to the shared vm management code. JVMTI.data_dump will send the data dump request to JVMTI. Both of these commands are covered by the corresponding tests. Thanks, -JB- -- Frederic Parain - Oracle Grenoble Engineering Center - France Phone: +33 4 76 18 81 17 Email: frederic.par...@oracle.com
RFR 8054890: Serviceability: New diagnostic commands 'VM.set_flag' and 'JVMTI.data_dump'
Please, review the following change Issue : https://bugs.openjdk.java.net/browse/JDK-8054890 Webrev: http://cr.openjdk.java.net/~jbachorik/8054890/webrev.00 This patch is about adding 2 new diagnostic commands - VM.set_flag and JVMTI.data_dump. VM.set_flag allows to set any writeable flag. It takes the flag name and the flag value in textual form. The mutability of the flag and the value format checks are forwarded to the shared vm management code. JVMTI.data_dump will send the data dump request to JVMTI. Both of these commands are covered by the corresponding tests. Thanks, -JB-
Re: RFR(XXS): 8064923: [TESTBUG] jps doesn't display anything on embedded platforms and it causes some tests to fail
Erik, Jaroslav, thanks for your reviews! // Katja On 03/18/2015 06:25 PM, Erik Gahlin wrote: Looks good. Erik Yekaterina Kantserova skrev den 18/03/15 17:37: Hi, Could I please have a review of this very small fix. bug: https://bugs.openjdk.java.net/browse/JDK-8064923 webrev: http://cr.openjdk.java.net/~ykantser/8064923/webrev.00/ Thanks, Katja
