Re: RFR(S): 6545295: TEST BUG: The test HatHeapDump1Test uses wrong path in exec call.
Hi, could I get one more review please? Thanks, Katja On 06/13/2014 02:23 PM, Staffan Larsen wrote: Looks good! Thanks, /Staffan On 13 jun 2014, at 14:15, Yekaterina Kantserova yekaterina.kantser...@oracle.com mailto:yekaterina.kantser...@oracle.com wrote: Staffan, thanks for your review! The new webrev can be found here:http://cr.openjdk.java.net/~ykantser/6545295/webrev.01/ http://cr.openjdk.java.net/%7Eykantser/6545295/webrev.01/ // Katja On 06/13/2014 01:44 PM, Staffan Larsen wrote: Katja, Great to see this simplification! A couple of comments: - I don’t think you need to launch jhat with -XX:+UsePerfData - no one is attaching or reading data from the process. - The static processBuilder field seems to be mis-used. Why not use different ProcessBuilder objects for the HelloWorld and jhat? - For dumpFile.deleteOnExit() to be useful you should add it as early as possible (line 48 perhaps?). If it is the last line in the test you can just as well do dumpFile.delete(). Of cause it should be dumpFile.delete()! - dumpFile.deleteOnExit() has crept in by mistake. /S On 13 jun 2014, at 13:25, Yekaterina Kantserova yekaterina.kantser...@oracle.com mailto:yekaterina.kantser...@oracle.com wrote: Hi, Could I please have a review of this fix. webrev: http://cr.openjdk.java.net/~ykantser/6545295 http://cr.openjdk.java.net/%7Eykantser/6545295 bug: https://bugs.openjdk.java.net/browse/JDK-6545295 The test has been re-written using the testlibrary's functionality. It's verified internally on all basic platforms. Thanks, Katja
Re: RFR(S): 6545295: TEST BUG: The test HatHeapDump1Test uses wrong path in exec call.
Katja, The policy for reviews in the jdk repo is different from the hotspot repo. For the jdk it is ok with a single review. /Staffan On 16 jun 2014, at 09:59, Yekaterina Kantserova yekaterina.kantser...@oracle.com wrote: Hi, could I get one more review please? Thanks, Katja On 06/13/2014 02:23 PM, Staffan Larsen wrote: Looks good! Thanks, /Staffan On 13 jun 2014, at 14:15, Yekaterina Kantserova yekaterina.kantser...@oracle.com wrote: Staffan, thanks for your review! The new webrev can be found here: http://cr.openjdk.java.net/~ykantser/6545295/webrev.01/ // Katja On 06/13/2014 01:44 PM, Staffan Larsen wrote: Katja, Great to see this simplification! A couple of comments: - I don’t think you need to launch jhat with -XX:+UsePerfData - no one is attaching or reading data from the process. - The static processBuilder field seems to be mis-used. Why not use different ProcessBuilder objects for the HelloWorld and jhat? - For dumpFile.deleteOnExit() to be useful you should add it as early as possible (line 48 perhaps?). If it is the last line in the test you can just as well do dumpFile.delete(). Of cause it should be dumpFile.delete()! - dumpFile.deleteOnExit() has crept in by mistake. /S On 13 jun 2014, at 13:25, Yekaterina Kantserova yekaterina.kantser...@oracle.com wrote: Hi, Could I please have a review of this fix. webrev: http://cr.openjdk.java.net/~ykantser/6545295 bug: https://bugs.openjdk.java.net/browse/JDK-6545295 The test has been re-written using the testlibrary's functionality. It's verified internally on all basic platforms. Thanks, Katja
Re: RFR JDK-8046282: SA update
Hi Sundar,
I have updated the enum classes. The updated webrev is available here:
http://cr.openjdk.java.net/~poonam/8046282/webrev.01/
Please review the changes and let me know your feedback.
Thanks,
Poonam
On 6/12/2014 1:25 PM, Poonam Bajaj wrote:
Hi Sundar,
Is it okay with you if I keep the enum and class definitions as they
are now? And can I add you as the reviewer for these changes?
Thanks,
Poonam
On 6/9/2014 2:56 PM, A. Sundararajan wrote:
Since SA is java code, we could have it cleaner..
my 2 cents,
-Sundar
On Monday 09 June 2014 02:40 PM, Poonam Bajaj wrote:
Hi Sundar,
Yes, it is possible to do that. e.g. G1YCType can be defined like
this.
public enum G1YCType {
Normal (Normal),
InitialMark (Initial Mark),
DuringMark (During Mark),
Mixed (Mixed),
G1YCTypeEndSentinel (Unknown)
private final String value;
G1YCType(String val) {
this.value = val;
}
public String value() {
return value;
}
}
But, my purpose of having a class and an enum being used in that
class was to have the similar kind of code structure on the SA side
as is present on the hotspot side. e.g the above is defined as the
following on the hotspot side:
030 enum G1YCType {
031 Normal,
032 InitialMark,
033 DuringMark,
034 Mixed,
035 G1YCTypeEndSentinel
036 };
037
038 class G1YCTypeHelper {
039 public:
040 static const char* to_string(G1YCType type) {
041 switch(type) {
042 case Normal: return Normal;
043 case InitialMark: return Initial Mark;
044 case DuringMark: return During Mark;
045 case Mixed: return Mixed;
046 default: ShouldNotReachHere(); return NULL;
047 }
048 }
049 };
And I have tried to replicate the same on the SA side so that one
can easily understand and map the definitions on both the sides.
27 public class G1YCTypeHelper {
28
29 public enum G1YCType {
30 Normal,
31 InitialMark,
32 DuringMark,
33 Mixed,
34 G1YCTypeEndSentinel
35 }
36
37 public static String toString(G1YCType type) {
38 switch (type) {
39 case Normal:
40 return Normal;
41 case InitialMark:
42 return Initial Mark;
43 case DuringMark:
44 return During Mark;
45 case Mixed:
46 return Mixed;
47 default:
48 return null;
49 }
50 }
51 }
Please let me know if this is still a concern for you.
Thanks,
Poonam
On 6/9/2014 10:38 AM, A. Sundararajan wrote:
The pattern of enum within a class and toString helper to return
string description of it -- is used for many cases.
Why not use enums with String accepting constructor and toString
(or new method called toDescription()) override? You could have
add Unknown in these enums to map to an option that is available
in VM -- but not in SA.
Since it is a debugger it is expected to work with incomplete
info.. so whereever you map a VM data structure element to java
enum, you can map unknown enum constants to Unknown on Java
side. Of course, if there is a sentinel element defined in VM
side, you could use that itself - no need for Unknown in such cases.
It'd nice to simplify that class-within-enum pattern...
-Sundar
On Saturday 07 June 2014 02:48 PM, Poonam Bajaj wrote:
Hi,
Please review these changes for the bug 8046282 for JDK 9. These
changes add some definitions on the SA side and the supporting
code on the hotspot side.
Webrev: http://cr.openjdk.java.net/~poonam/8046282/webrev.00/
Bug: https://bugs.openjdk.java.net/browse/JDK-8046282
Thanks,
Poonam
Re: RFR JDK-8046282: SA update
Hi Poonam,
Looks good to me.
-Sundar
On Monday 16 June 2014 02:45 PM, Poonam Bajaj wrote:
Hi Sundar,
I have updated the enum classes. The updated webrev is available here:
http://cr.openjdk.java.net/~poonam/8046282/webrev.01/
Please review the changes and let me know your feedback.
Thanks,
Poonam
On 6/12/2014 1:25 PM, Poonam Bajaj wrote:
Hi Sundar,
Is it okay with you if I keep the enum and class definitions as they
are now? And can I add you as the reviewer for these changes?
Thanks,
Poonam
On 6/9/2014 2:56 PM, A. Sundararajan wrote:
Since SA is java code, we could have it cleaner..
my 2 cents,
-Sundar
On Monday 09 June 2014 02:40 PM, Poonam Bajaj wrote:
Hi Sundar,
Yes, it is possible to do that. e.g. G1YCType can be defined like
this.
public enum G1YCType {
Normal (Normal),
InitialMark (Initial Mark),
DuringMark (During Mark),
Mixed (Mixed),
G1YCTypeEndSentinel (Unknown)
private final String value;
G1YCType(String val) {
this.value = val;
}
public String value() {
return value;
}
}
But, my purpose of having a class and an enum being used in that
class was to have the similar kind of code structure on the SA side
as is present on the hotspot side. e.g the above is defined as the
following on the hotspot side:
030 enum G1YCType {
031 Normal,
032 InitialMark,
033 DuringMark,
034 Mixed,
035 G1YCTypeEndSentinel
036 };
037
038 class G1YCTypeHelper {
039 public:
040 static const char* to_string(G1YCType type) {
041 switch(type) {
042 case Normal: return Normal;
043 case InitialMark: return Initial Mark;
044 case DuringMark: return During Mark;
045 case Mixed: return Mixed;
046 default: ShouldNotReachHere(); return NULL;
047 }
048 }
049 };
And I have tried to replicate the same on the SA side so that one
can easily understand and map the definitions on both the sides.
27 public class G1YCTypeHelper {
28
29 public enum G1YCType {
30 Normal,
31 InitialMark,
32 DuringMark,
33 Mixed,
34 G1YCTypeEndSentinel
35 }
36
37 public static String toString(G1YCType type) {
38 switch (type) {
39 case Normal:
40 return Normal;
41 case InitialMark:
42 return Initial Mark;
43 case DuringMark:
44 return During Mark;
45 case Mixed:
46 return Mixed;
47 default:
48 return null;
49 }
50 }
51 }
Please let me know if this is still a concern for you.
Thanks,
Poonam
On 6/9/2014 10:38 AM, A. Sundararajan wrote:
The pattern of enum within a class and toString helper to return
string description of it -- is used for many cases.
Why not use enums with String accepting constructor and toString
(or new method called toDescription()) override? You could have
add Unknown in these enums to map to an option that is available
in VM -- but not in SA.
Since it is a debugger it is expected to work with incomplete
info.. so whereever you map a VM data structure element to java
enum, you can map unknown enum constants to Unknown on Java
side. Of course, if there is a sentinel element defined in VM
side, you could use that itself - no need for Unknown in such
cases.
It'd nice to simplify that class-within-enum pattern...
-Sundar
On Saturday 07 June 2014 02:48 PM, Poonam Bajaj wrote:
Hi,
Please review these changes for the bug 8046282 for JDK 9. These
changes add some definitions on the SA side and the supporting
code on the hotspot side.
Webrev: http://cr.openjdk.java.net/~poonam/8046282/webrev.00/
Bug: https://bugs.openjdk.java.net/browse/JDK-8046282
Thanks,
Poonam
Re: RFR JDK-8046282: SA update
Thanks for the review, Sundar.
regards,
Poonam
On 6/16/2014 3:54 PM, A. Sundararajan wrote:
Hi Poonam,
Looks good to me.
-Sundar
On Monday 16 June 2014 02:45 PM, Poonam Bajaj wrote:
Hi Sundar,
I have updated the enum classes. The updated webrev is available here:
http://cr.openjdk.java.net/~poonam/8046282/webrev.01/
Please review the changes and let me know your feedback.
Thanks,
Poonam
On 6/12/2014 1:25 PM, Poonam Bajaj wrote:
Hi Sundar,
Is it okay with you if I keep the enum and class definitions as they
are now? And can I add you as the reviewer for these changes?
Thanks,
Poonam
On 6/9/2014 2:56 PM, A. Sundararajan wrote:
Since SA is java code, we could have it cleaner..
my 2 cents,
-Sundar
On Monday 09 June 2014 02:40 PM, Poonam Bajaj wrote:
Hi Sundar,
Yes, it is possible to do that. e.g. G1YCType can be defined like
this.
public enum G1YCType {
Normal (Normal),
InitialMark (Initial Mark),
DuringMark (During Mark),
Mixed (Mixed),
G1YCTypeEndSentinel (Unknown)
private final String value;
G1YCType(String val) {
this.value = val;
}
public String value() {
return value;
}
}
But, my purpose of having a class and an enum being used in that
class was to have the similar kind of code structure on the SA
side as is present on the hotspot side. e.g the above is defined
as the following on the hotspot side:
030 enum G1YCType {
031 Normal,
032 InitialMark,
033 DuringMark,
034 Mixed,
035 G1YCTypeEndSentinel
036 };
037
038 class G1YCTypeHelper {
039 public:
040 static const char* to_string(G1YCType type) {
041 switch(type) {
042 case Normal: return Normal;
043 case InitialMark: return Initial Mark;
044 case DuringMark: return During Mark;
045 case Mixed: return Mixed;
046 default: ShouldNotReachHere(); return NULL;
047 }
048 }
049 };
And I have tried to replicate the same on the SA side so that one
can easily understand and map the definitions on both the sides.
27 public class G1YCTypeHelper {
28
29 public enum G1YCType {
30 Normal,
31 InitialMark,
32 DuringMark,
33 Mixed,
34 G1YCTypeEndSentinel
35 }
36
37 public static String toString(G1YCType type) {
38 switch (type) {
39 case Normal:
40 return Normal;
41 case InitialMark:
42 return Initial Mark;
43 case DuringMark:
44 return During Mark;
45 case Mixed:
46 return Mixed;
47 default:
48 return null;
49 }
50 }
51 }
Please let me know if this is still a concern for you.
Thanks,
Poonam
On 6/9/2014 10:38 AM, A. Sundararajan wrote:
The pattern of enum within a class and toString helper to return
string description of it -- is used for many cases.
Why not use enums with String accepting constructor and toString
(or new method called toDescription()) override? You could have
add Unknown in these enums to map to an option that is
available in VM -- but not in SA.
Since it is a debugger it is expected to work with incomplete
info.. so whereever you map a VM data structure element to java
enum, you can map unknown enum constants to Unknown on Java
side. Of course, if there is a sentinel element defined in VM
side, you could use that itself - no need for Unknown in such
cases.
It'd nice to simplify that class-within-enum pattern...
-Sundar
On Saturday 07 June 2014 02:48 PM, Poonam Bajaj wrote:
Hi,
Please review these changes for the bug 8046282 for JDK 9. These
changes add some definitions on the SA side and the supporting
code on the hotspot side.
Webrev: http://cr.openjdk.java.net/~poonam/8046282/webrev.00/
Bug: https://bugs.openjdk.java.net/browse/JDK-8046282
Thanks,
Poonam
Re: RFR(M): 8028474: sun/jvmstat/monitor/MonitoredVm/MonitorVmStartTerminate.sh timeout, leaves looping process behind
Erik,
It should work see e.g.
hotspot/test/runtime/RedefineFinalizer
you might need to add
-Xbootclasspath/a:tools.jar to @run
and
-XDignore.symbol.file to @compile
-Dmitry
On 2014-06-16 14:32, Erik Gahlin wrote:
Yekaterina Kantserova skrev 13/06/14 12:59:
Erik,
is there some reason why we need to keep MonitorVmStartTerminate.sh?
I've moved the JTreg header to MonitorVmStartTerminate.java
Hi Katja,
That's how I did the test initially, and it works locally, but I could
never get it to work in JPRT without the shell script. I believe the
tools.jar is not on the class path.
Erik
/*
* @test
* @bug 4990825
* @summary attach to external but local JVM processes
* @library /lib/testlibrary
* @build jdk.testlibrary.*
* @run main MonitorVmStartTerminate
*/
and the test works just fine.
The JTreg run contains all pathes and system properties
MonitorVmStartTerminate.sh tries to construct:
${JAVA} ${TESTVMOPTS} -Dtest.jdk=${TESTJAVA}
-Dtest.classes=${TESTCLASSES} -classpath ${CP} MonitorVmStartTerminate
See the log attached.
Note *@build jdk.testlibrary.** instead of *@build
jdk.testlibrary.ProcessTools* to make sure all testlibrary classes are
compiled
to the right place when running tests concurrently.
Thanks,
Katja (Not a Reviewer)
On 06/12/2014 12:37 AM, Erik Gahlin wrote:
Hi,
Could I have a review of a test that has been failing
intermittently. The test now uses files for synchronization
instead of waiting for a process that sleeps.
Webrev:
http://cr.openjdk.java.net/~egahlin/8028474/
Bug:
https://bugs.openjdk.java.net/browse/JDK-8028474
Description:
The test starts ten Java processes, each with a unique id.
Each process creates a file named after the id and then it waits for
the test to remove the file, at which the Java process exits.
The processes are monitored by the test to make sure notifications
are sent when processes are started/terminated.
To avoid Java processes being left behind, in case of an unexpected
failure, shutdown hooks are registered that remove files when the test
exits. If files are not removed, i.e. due to a JVM crash,
the Java processes will exit themselves after 1000 s.
Thanks
Erik
--
Dmitry Samersoff
Oracle Java development team, Saint Petersburg, Russia
* I would love to change the world, but they won't give me the sources.
Re: RFR: 6622468 TEST_BUG: Time to retire the @debuggeeVMOptions mechanism used in the com.sun.jdi infrastructure
Thanks David!
On 16 jun 2014, at 14:22, David Holmes david.hol...@oracle.com wrote:
Looks okay to me.
Thanks,
David
On 16/06/2014 9:59 PM, Staffan Larsen wrote:
I would appreciate a Review of this change.
Thanks,
/Staffan
On 11 jun 2014, at 10:00, Staffan Larsen staffan.lar...@oracle.com
mailto:staffan.lar...@oracle.com wrote:
I realized that the code in VMConnection does not take into account
the test.java.opts property as it should.
updated webrev: http://cr.openjdk.java.net/~sla/6622468/webrev.2.01/
(only VMConnection changed)
Thanks,
/Staffan
On 10 jun 2014, at 13:59, Staffan Larsen staffan.lar...@oracle.com
mailto:staffan.lar...@oracle.com wrote:
On 10 jun 2014, at 11:44,serguei.spit...@oracle.com
mailto:serguei.spit...@oracle.comwrote:
Staffan,
It looks good, just one comment.
test/com/sun/jdi/VMConnection.java
61 String vmOpts = System.getProperty(test.vm.opts);
62 if (vmOpts != null) {
63 retVal += System.getProperty(test.vm.opts);
I wonder why not this:
63 retVal += vmOpts;
Uh. Yeah, I wonder that, too. Fixed. :-)
Thanks,
/Staffan
Thanks,
Serguei
On 6/10/14 12:58 AM, Staffan Larsen wrote:
This is a new take on this old bug. Since my previous attempt [0], jtreg
has been update with a “driver” feature and this is exactly what these
tests need. Specifying “@run driver” (instead of “@run main”) will
launch the test with no vm arguments. Whatever arguments were specified
in -vmoptions to jtreg will be available in the System property
test.vm.opts and the test code can use those arguments when launching
other processes.
For the JDI tests this is a very good match. The tests run two
processes: one debugger and one debuggee. It is really the debuggee that
is being tested, the the debugger is just driving the testing. So it is
the debuggee that should be invoked with the specified -vmoptions.
In this change I’ve changed all debuggers to be launched with “@run
driver” and all debuggees to be launched using the test.vm.opts options.
This will remove the need to the esoteric @debuggeeVMOptions file that
was previously used to pass arguments to the debuggee.
webrev:http://cr.openjdk.java.net/~sla/6622468/webrev.2.00/
bug:https://bugs.openjdk.java.net/browse/JDK-6622468
The webrev is very boring to read, probably best to read the diff file
directly. test/com/sun/jdi/VMConnection.java has the only substantial
change.
I have run this through JPRT with no failures.
Thanks,
/Staffan
[0]http://mail.openjdk.java.net/pipermail/serviceability-dev/2013-August/011325.html
Re: RFR: 6622468 TEST_BUG: Time to retire the @debuggeeVMOptions mechanism used in the com.sun.jdi infrastructure
updated webrev: http://cr.openjdk.java.net/~sla/6622468/webrev.2.01/
test/com/sun/jdi/VMConnection.java
line 62: System.out.println(vmOpts: +vmOpts);
line 67: System.out.println(javaOpts: +javaOpts);
line 68: if (javaOpts != null) {
Indent off by one in patch view; looks like there are tabs.
test/com/sun/jdi/connect/spi/DebugUsingCustomConnector.java
line 62: List launchers = vmm.launchingConnectors();
Indent off by one in patch view; looks like there is a tab.
test/com/sun/jdi/sde/MangleStepTest.java
old line 85: waitForInput();
Why was this line deleted?
No comments on the other files.
Dan
On 6/16/14 5:59 AM, Staffan Larsen wrote:
I would appreciate a Review of this change.
Thanks,
/Staffan
On 11 jun 2014, at 10:00, Staffan Larsen staffan.lar...@oracle.com
mailto:staffan.lar...@oracle.com wrote:
I realized that the code in VMConnection does not take into account
the test.java.opts property as it should.
updated webrev: http://cr.openjdk.java.net/~sla/6622468/webrev.2.01/
http://cr.openjdk.java.net/%7Esla/6622468/webrev.2.01/ (only
VMConnection changed)
Thanks,
/Staffan
On 10 jun 2014, at 13:59, Staffan Larsen staffan.lar...@oracle.com
mailto:staffan.lar...@oracle.com wrote:
On 10 jun 2014, at 11:44,serguei.spit...@oracle.com
mailto:serguei.spit...@oracle.comwrote:
Staffan,
It looks good, just one comment.
test/com/sun/jdi/VMConnection.java
61 String vmOpts = System.getProperty(test.vm.opts);
62 if (vmOpts != null) {
63 retVal += System.getProperty(test.vm.opts);
I wonder why not this:
63 retVal += vmOpts;
Uh. Yeah, I wonder that, too. Fixed. :-)
Thanks,
/Staffan
Thanks,
Serguei
On 6/10/14 12:58 AM, Staffan Larsen wrote:
This is a new take on this old bug. Since my previous attempt [0], jtreg has
been update with a “driver” feature and this is exactly what these tests need.
Specifying “@run driver” (instead of “@run main”) will launch the test with no
vm arguments. Whatever arguments were specified in -vmoptions to jtreg will be
available in the System property test.vm.opts and the test code can use those
arguments when launching other processes.
For the JDI tests this is a very good match. The tests run two processes: one
debugger and one debuggee. It is really the debuggee that is being tested, the
the debugger is just driving the testing. So it is the debuggee that should be
invoked with the specified -vmoptions.
In this change I’ve changed all debuggers to be launched with “@run driver” and
all debuggees to be launched using the test.vm.opts options. This will remove
the need to the esoteric @debuggeeVMOptions file that was previously used to
pass arguments to the debuggee.
webrev:http://cr.openjdk.java.net/~sla/6622468/webrev.2.00/
bug:https://bugs.openjdk.java.net/browse/JDK-6622468
The webrev is very boring to read, probably best to read the diff file
directly. test/com/sun/jdi/VMConnection.java has the only substantial change.
I have run this through JPRT with no failures.
Thanks,
/Staffan
[0]http://mail.openjdk.java.net/pipermail/serviceability-dev/2013-August/011325.html
