Re: JVM/TI code review request (XS and M) (7182152)

2013-02-13 Thread Alan Bateman
On 12/02/2013 00:41, Daniel D. Daugherty wrote: Greetings, Right after I pushed the changeset for 8007420, Alan B let me know that he had updated the JLI/JPLIS test infrastructure to support running the tests with a JRE instead of a JDK. Alan made his changes in early January, but I had

Re: JVM/TI code review request (XS and M) (7182152)

2013-02-11 Thread Daniel D. Daugherty
On 2/11/13 8:34 AM, Coleen Phillimore wrote: Looks good, although I was surprised you don't need -e. Thanks for the review! If you only have a single edit cmd, then '-e' is not needed. Dan Coleen On 02/11/2013 10:05 AM, Daniel D. Daugherty wrote: Greetings, I've revised one of the

Re: JVM/TI code review request (XS and M) (7182152)

2013-02-11 Thread serguei.spit...@oracle.com
Looks good. Thanks, Serguei On 2/11/13 7:05 AM, Daniel D. Daugherty wrote: Greetings, I've revised one of the tests to make it more portable based on JPRT test results for the Code Review Round 1 version. Here is the URL for the webrev for Code Review Round 2:

Re: JVM/TI code review request (XS and M) (7182152)

2013-02-11 Thread Daniel D. Daugherty
Thanks for another re-review! Dan On 2/11/13 10:28 AM, serguei.spit...@oracle.com wrote: Looks good. Thanks, Serguei On 2/11/13 7:05 AM, Daniel D. Daugherty wrote: Greetings, I've revised one of the tests to make it more portable based on JPRT test results for the Code Review Round 1

Re: JVM/TI code review request (XS and M) (7182152)

2013-02-11 Thread Daniel D. Daugherty
Greetings, Right after I pushed the changeset for 8007420, Alan B let me know that he had updated the JLI/JPLIS test infrastructure to support running the tests with a JRE instead of a JDK. Alan made his changes in early January, but I had developed this test back in December. I never checked to

Re: JVM/TI code review request (XS and M) (7182152)

2013-02-07 Thread serguei.spit...@oracle.com
I agree, the test is a piece of art - nice example to start from for future test development. Ship it! Thanks, Serguei On 2/6/13 12:08 PM, Daniel D. Daugherty wrote: Greetings, I've revised the tests based on Coleen's and Serguei's feedback in Code Review Round 0. Here is the URL for the

Re: JVM/TI code review request (XS and M) (7182152)

2013-02-07 Thread Daniel D. Daugherty
Thanks for the re-review! Dan On 2/7/13 1:30 AM, serguei.spit...@oracle.com wrote: I agree, the test is a piece of art - nice example to start from for future test development. Ship it! Thanks, Serguei On 2/6/13 12:08 PM, Daniel D. Daugherty wrote: Greetings, I've revised the tests

Re: JVM/TI code review request (XS and M) (7182152)

2013-02-06 Thread Daniel D. Daugherty
Adding other alias and people back onto the thread... Thanks for the re-review! On 2/6/13 6:41 AM, Coleen Phillimore wrote: This is good that you added the INCLUDE_JVMTI. I didn't think it'd add that much space, but it a good change. I didn't think it would add much space either, but...

Re: JVM/TI code review request (XS and M) (7182152)

2013-02-06 Thread Daniel D. Daugherty
Thanks Serguei! Dan On 2/6/13 1:22 PM, serguei.spit...@oracle.com wrote: Looks good. Thanks, Serguei On 2/6/13 11:54 AM, Karen Kinnear wrote: Thank you Dan - this is much better and sets a good model for the rest of us. thanks, Karen On Feb 6, 2013, at 9:05 AM, Daniel D. Daugherty

Re: JVM/TI code review request (XS and M) (7182152)

2013-02-05 Thread serguei.spit...@oracle.com
Both tests look good, I do not see any issues yet. What is missed is a comment explaining what happened when the bug is not fixed and what correct behavior is expected. Maybe it'd make sense to put bad and good output into the comment, not everything, but the most important fragments. It would

Re: JVM/TI code review request (XS and M) (7182152)

2013-02-05 Thread Daniel D. Daugherty
On 2/5/13 10:50 AM, serguei.spit...@oracle.com wrote: Both tests look good, I do not see any issues yet. What is missed is a comment explaining what happened when the bug is not fixed and what correct behavior is expected. Maybe it'd make sense to put bad and good output into the comment, not

Re: JVM/TI code review request (XS and M) (7182152)

2013-02-05 Thread Daniel D. Daugherty
On 2/5/13 12:56 PM, Karen Kinnear wrote: New versions look great. Thank you. Thanks! For jdk8 only - is redefineclasses in the MINIMALVM? I missed your reply. If so, I was assuming that the additional tracing would also be conditional in the builds. I believe in addition to ifdef's there is

Re: JVM/TI code review request (XS and M) (7182152)

2013-02-04 Thread Daniel D. Daugherty
Adding back the missing aliases and people... Coleen, Thanks for the review. Replies embedded below. On 2/4/13 7:26 AM, Coleen Phillimore wrote: On 2/1/2013 6:55 PM, Daniel D. Daugherty wrote: And here is the webrev for the new tests (relative to JDK8-TL):

Re: JVM/TI code review request (XS and M) (7182152)

2013-02-04 Thread Karen Kinnear
Dan, All 3 versions of the code looks good. Thank you for enabling the printing for product since this type of problem is so hard to duplicate. A small note, I think it would have been easier for the internal code logic for the CPCE::check_no_old_or_obsolete_entries to reverse the true/false,

Re: JVM/TI code review request (XS and M) (7182152)

2013-02-04 Thread Coleen Phillimore
On 2/4/2013 10:15 AM, Daniel D. Daugherty wrote: Adding back the missing aliases and people... Sorry, I meant to send this re-all. I missed something major in my earlier review. The metadata-is_valid() flag should only be enabled in debug mode. It adds a word to all metadata. Coleen,

Re: JVM/TI code review request (XS and M) (7182152)

2013-02-04 Thread Daniel D. Daugherty
Karen, Thanks for the reviews! Replies embedded below. On 2/4/13 8:19 AM, Karen Kinnear wrote: Dan, All 3 versions of the code looks good. Thank you for enabling the printing for product since this type of problem is so hard to duplicate. You're welcome. A small note, I think it would

Re: JVM/TI code review request (XS and M) (7182152)

2013-02-04 Thread Daniel D. Daugherty
Thanks for the additional comment. Reply below. On 2/4/13 8:34 AM, Coleen Phillimore wrote: On 2/4/2013 10:15 AM, Daniel D. Daugherty wrote: Adding back the missing aliases and people... Sorry, I meant to send this re-all. I missed something major in my earlier review. The

Re: JVM/TI code review request (XS and M) (7182152)

2013-02-04 Thread Daniel D. Daugherty
Adding back dropped aliases and people... Thanks for reviewing the first test! Replies embedded below. On 2/4/13 8:09 AM, Coleen Phillimore wrote: On 2/1/2013 6:55 PM, Daniel D. Daugherty wrote: And here is the webrev for the new tests (relative to JDK8-TL):

Re: JVM/TI code review request (XS and M) (7182152)

2013-02-04 Thread Daniel D. Daugherty
On 2/4/13 10:16 AM, Coleen Phillimore wrote: On 2/4/2013 11:50 AM, Daniel D. Daugherty wrote: Thanks for the additional comment. Reply below. On 2/4/13 8:34 AM, Coleen Phillimore wrote: On 2/4/2013 10:15 AM, Daniel D. Daugherty wrote: Adding back the missing aliases and people... Sorry, I

Re: JVM/TI code review request (XS and M) (7182152)

2013-02-04 Thread serguei.spit...@oracle.com
Dan, The fixes look good for all 3 HS versions (modulo discussions with Coleen and Karen). Great discovery, thank you for doing this! Thanks, Serguei On 2/1/13 11:55 AM, Daniel D. Daugherty wrote: Greetings, I have a fix for the following JVM/TI bug: 7182152 Instrumentation hot

Re: JVM/TI code review request (XS and M) (7182152)

2013-02-04 Thread Daniel D. Daugherty
On 2/4/13 12:21 PM, serguei.spit...@oracle.com wrote: Dan, The fixes look good for all 3 HS versions (modulo discussions with Coleen and Karen). Great discovery, thank you for doing this! Thanks Serguei! I'm working on addressing Coleen's and Karen's comments and will be rolling out three

Re: JVM/TI code review request (XS and M) (7182152)

2013-02-04 Thread Coleen Phillimore
On 2/4/2013 12:19 PM, Daniel D. Daugherty wrote: Adding back dropped aliases and people... I think I have all the aliases on this, this time. Thanks for reviewing the first test! Replies embedded below. Ok, now I see about the scaffolding framework for the first test. I can say it looks

Re: JVM/TI code review request (XS and M) (7182152)

2013-02-04 Thread Daniel D. Daugherty
Greetings, I've updated the fix due to comments in Code Review Round 0. Here is a summary of changes made to the various files: src/share/vm/oops/cpCacheOop.cpp (HSX-23.6, HSX-24) src/share/vm/oops/cpCache.cpp (HSX-25) src/share/vm/oops/klassVtable.cpp - removed the new RC_TRACE_NO_CR()

JVM/TI code review request (XS and M) (7182152)

2013-02-01 Thread Daniel D. Daugherty
Greetings, I have a fix for the following JVM/TI bug: 7182152 Instrumentation hot swap test incorrect monitor count http://bugs.sun.com/bugdatabase/view_bug.do?bug_id=7182152 https://jbs.oracle.com/bugs/browse/JDK-7182152 The fix for the bug in the product code is one line:

Re: JVM/TI code review request (XS and M) (7182152)

2013-02-01 Thread Daniel D. Daugherty
There are two new tests that will be pushed to the JDK repos using a different bug ID (not yet filed): New bug is now filed: 8007420 add test for 6805864 to com/sun/jdi, add test for 7182152 to java/lang/instrument http://bugs.sun.com/bugdatabase/view_bug.do?bug_id=8007420

Re: JVM/TI code review request (XS and M) (7182152)

2013-02-01 Thread Daniel D. Daugherty
And here is the webrev for the new tests (relative to JDK8-TL): http://cr.openjdk.java.net/~dcubed/8007420-webrev/0-jdk8-tl/ As always, comments and suggestions are welcome. Dan On 2/1/13 4:39 PM, Daniel D. Daugherty wrote: There are two new tests that will be pushed to the JDK repos using