Already reviewed. :)
It is good.

Thanks,
Serguei


On 2/6/13 12:09 PM, Daniel D. Daugherty wrote:
Karen,

Thanks for the re-review!

Serguei, just waiting for you to chime in!!

Dan


On 2/6/13 12:54 PM, 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 wrote:

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...

It gave me a chance to check out the MinimalVM stuff a bit and
it serves to identify those pieces of code as being associated
with JVM/TI.

Karen and Serguei, when you get the chance, please re-review.

Again, thanks for the re-review!

Dan


Thumbs up!
Coleen

On 2/6/2013 12:59 AM, Daniel D. Daugherty wrote:
Greetings,

The JPDA stack testing finished with no new regressions on HSX-23.6,
HSX-24 or HSX-25. The HSX-24 version of the fix has been pushed.

I've updated the HSX-25 version of the fix due to Karen's comments
about the MinimalVM configuration in Code Review Round 1. In this
latest round for HSX-25, I've made use of the INCLUDE_JVMTI define
to limit the exposure of new tracing code to configurations that
include JVM/TI support. The MinimalVM does not support JVM/TI so
none of the new code needs to be included there. While I was at it,
I also excluded some other JVM/TI RedefineClasses() support code
from the MinimalVM config that I hadn't previously touched.

In short, none of the functionality has been changed in this round.
Just the way it is built or not built has been changed.

Here is the URL for the latest HSX-25 webrev:

http://cr.openjdk.java.net/~dcubed/7182152-webrev/2-hsx25/

Thanks, in advance, for more comments, suggestions or questions.

Dan


On 2/4/13 2:08 PM, Daniel D. Daugherty wrote:
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() macro calls at Coleen's request;
    these files are outside of JVM/TI RedefineClasses proper so the
tracing/debug style should not be dictated by JVM/TI RedefineClasses
    style that is currently in use
- did not touch the existing RC_TRACE... macros in the file; removing
    the existing macro calls would be outside the scope of this bug

src/share/vm/oops/cpCacheOop.cpp (HSX-23.6, HSX-24)
src/share/vm/oops/cpCache.cpp (HSX-25)
- copy a comment from ConstantPoolCacheEntry::is_interesting_method_entry()
    to ConstantPoolCacheEntry::check_no_old_or_obsolete_entries()

src/share/vm/oops/klassVtable.cpp
  - Copied the new comment intended to prevent the "break" from
    re-materializing again from klassItable::adjust_method_entries()
    to klassVtable::adjust_method_entries(); yes, I'm paranoid.

In the HSX-25 version only:
src/share/vm/oops/metadata.hpp
  - revert changes

src/share/vm/oops/cpCacheOop.cpp
src/share/vm/oops/klassVtable.cpp
  - wrap uses of is_valid() in NOT_PRODUCT macro as appropriate

Here are the URLs for the updated webrevs:

http://cr.openjdk.java.net/~dcubed/7182152-webrev/1-hsx23.6/
http://cr.openjdk.java.net/~dcubed/7182152-webrev/1-hsx24/
http://cr.openjdk.java.net/~dcubed/7182152-webrev/1-hsx25/

The JPDA stack testing that I started on Friday is still running on
WinXP so I'm blocked for checking the recompile due to these changes.
I'll start a recompile on Solaris X86, but that will take some time.

Thanks, in advance, for more comments, suggestions or questions.

Dan



On 2/1/13 12:55 PM, Daniel D. Daugherty wrote:
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:

src/share/vm/oops/klassVtable.cpp:

@@ -992,18 +1020,50 @@
           // RC_TRACE macro has an embedded ResourceMark
           RC_TRACE(0x00200000, ("itable method update: %s(%s)",
             new_method->name()->as_C_string(),
new_method->signature()->as_C_string()));
         }
-        break;
+        // cannot 'break' here; see for-loop comment above.
       }
       ime++;
     }
   }
}

and is applicable to JDK7u10/HSX-23.6 and JDK7u14/HSX-24. Coleen
already fixed the bug as part of the Perm Gen Removal (PGR) project
in HSX-25. Yes, we found a 1-line bug fix buried in the monster PGR
changeset. Many thanks to Coleen for her help in this bug hunt!

The rest of the code in the webrevs are:

- additional JVM/TI tracing code backported from Coleen's PGR changeset - additional JVM/TI tracing code added by me and forward ported to HSX-25
- a new -XX:TraceRedefineClasses=16384 flag value for finding these
  elusive old or obsolete methods
- exposure of some printing code to the PRODUCT build so that the new
  tracing is available in a PRODUCT build

You might be wondering why the new tracing code is exposed in a PRODUCT build. Well, it appears that more and more PRODUCT bits deployments are using JVM/TI RedefineClasses() and/or RetransformClasses() at run-time to instrument their systems. This bug (7182152) was only intermittently reproducible in the WLS environment in which it occurred so I made the
tracing available in a PRODUCT build to assist in the hunt.

Raj from the WLS team has also verified that the HSX-23.6 version of
fix resolves the issue in his environment. Thanks Raj!

Here are the URLs for the three webrevs:

http://cr.openjdk.java.net/~dcubed/7182152-webrev/0-hsx23.6/
http://cr.openjdk.java.net/~dcubed/7182152-webrev/0-hsx24/
http://cr.openjdk.java.net/~dcubed/7182152-webrev/0-hsx25/

I have run the following test suites from the JPDA stack on the
JDK7u10/HSX-23.6 version of the fix with -XX:TraceRedefineClasses=16384
specified:

    sdk-jdi
    sdk-jdi_closed
    sdk-jli
    vm-heapdump
    vm-hprof
    vm-jdb
    vm-jdi
    vm-jdwp
    vm-jvmti
    vm-sajdi

The tested configs are:

    {Solaris-X86, WinXP}
      X {Client VM, Server VM}
      X {-Xmixed, -Xcomp}
      X {product, fastdebug}

With the 1-liner fix in place, the new tracing code does not find any instances of this failure mode in any of the above test suites. Without the the 1-liner fix in place, the new tracing code finds one instance
of this failure mode in the above test suites:

test/java/lang/instrument/IsModifiableClassAgent.java

There are two new tests that will be pushed to the JDK repos using
a different bug ID (not yet filed):

    test/com/sun/jdi/RedefineAbstractClass.sh
test/java/lang/instrument/RedefineSubclassWithTwoInterfaces.sh

There will be a separate review request for the new tests.

I'm currently running the JPDA stack of tests on the JDK7u14/HSX-24
and JDK8-B75/HSX-25 versions of the fix. That testing will likely
take all weekend to complete.

Thanks, in advance, for any comments and/or suggestions.

Dan







Reply via email to