(original) (raw)

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,

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-T&L):



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

> 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


    https://jbs.oracle.com/bugs/browse/JDK-8007420




Of course, the tests cannot be pushed until the HSX
changes have made

it into a promoted build and thus available to
JDK8-T&L.



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/

In cpCache.cpp:

RC_TRACE_NO_CR(0x00004000, (""));

Can this "searchable prefix" be defined in jvmtiTracing with
the rest of the RC_TRACE macros as some descriptive RC_TRACE
name?  Doesn't have to be long but this is distracting
stuff.   Also, if you change this searchable prefix, you'd
only have to change it once.

In the cpCache.cpp case, the RC_TRACE_NO_CR(0x00004000, (""))
macro

calls adds a searchable prefix for each dump line when the
dump code

is called from JVM/TI RedefineClasses tracing. Other code that
calls

the cpCache dump code won't get the JVM/TI RedefineClasses
tracing

prefix. The purpose for the prefix is so that all tracing
output for

a particular tracing level, e.g., 0x00004000, is grep'able
together.

This function

void ConstantPoolCacheEntry::print(outputStream* st, int index) const {
}

Is general purpose code. It shouldn't have all these lines
specific to redefine classes. If it needs to, the noise should be
minimized.   You can add a define in that trace macros.hpp file:



#define RC_PREFIX <all that stuff>



This code shouldn't be present in the general purpose metadata
printing.    The dump_vtable and dump_itable functions are
arguable since they seem to only be used by redefine classes now,
but I can see a general purpose use for these too that shouldn't
include this noise.

It wouldn't be appropriate to add the "searchable prefix" to
the

jvmtiTraceRedefineClasses.hpp file. The HEX values in JVM/TI

RedefineClasses tracing are associated with the code being
traced.

In jvmtiRedefineClasses.cpp why can't dump_methods just print
the methods without these RC_TRACE macros?

Debug output style for JVM/TI RedefineClasses() is
supposed to be done

using the jvmtiTraceRedefineClasses mechanism. It was
inconsistent for

dump_methods() to do its output the way it was so I fixed it.

And

some is printed and some is not?

No, in this case, every line will have a
jvmtiTraceRedefineClasses

prefix on it when that tracing level is turned on. Example:



RedefineClasses-0x4000: _old_methods --

RedefineClasses-0x4000: 0 ( -2) public {old} -- virtual void
RedefineSubclassWithTwoInterfacesTarget.<init>()

RedefineClasses-0x4000: 1 ( 5) public {old} {obsolete} --
virtual jobject
RedefineSubclassWithTwoInterfacesTarget.echo(jobject)



Again, the prefix, "RedefineClasses-0x4000:", exists so that
everything

at that tracing level is grep'able together.

This

is really hard to read.   The indentation came out strange in
the webrev too.

Yes, the original dump_methods() code didn't follow hotspot

style and I reformatted it since I was changing much of the

code in the function.



I tend to use "frames" view in webrevs and I don't see any

issues with indentation.

I see what happened with dump_methods.  I think it was refactored
(to cut/paste places) and the indentation wasn't fixed.  You can
leave the noise in there since it's in redefine/classes, but I
really object to it in the general purpose code.



Coleen

It looks like the call to dump_methods() is covered by one of
these RC_TRACE macros.

Yes, I did that intentionally. If I didn't then I would have
to have

redone all the print code to match jvmtiTraceRedefineClasses
style.

It was easier to add RC_TRACE_NO_CR() calls where needed and
protect

the initial call with an RC_TRACE_ENABLED check.

Why

isn't that enough?

Because all RedefineClasses debug output is supposed to have a

prefix like this one:



RedefineClasses-0x4000: _old_methods --

This

is really distracting because I keep wondering why it's
RC_TRACE_NO_CR (don't file a CR??) rather than reading the
code.   Oh, it's no carriage return.   Ugh.

You mean like print_cr()? :-)

I
still would like dump_methods to always dump all the methods
so if you're debugging this you can temporarily paste this
call various places without trying to figure out which
RC_TRACE number to give it.

Sorry, that's not how debugging in RedefineClasses is
supposed to work.

dump_methods() was an outlier that needed to be fixed so that
it matched

the rest of the jvmtiTraceRedefineClasses infrastructure.



Of course, the Serviceability team is welcome to change this
debugging

code to suite their own style and tastes. I think that would
be an easier

task if it was all "the same".



Dan

Coleen

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