> open webrev at http://cr.openjdk.java.net/~coleenp/8005056/
Thumbs up. Minor nits/questions below.
Very nicely done. Adding a test to retransform java.lang.Object
will do a nice job of detecting potential breakages in this area.
src/share/vm/classfile/dictionary.cpp
No comments.
src/share/vm/classfile/dictionary.hpp
No comments.
src/share/vm/classfile/systemDictionary.cpp
No comments.
src/share/vm/classfile/systemDictionary.hpp
No comments.
src/share/vm/oops/klass.hpp
No comments.
src/share/vm/prims/jvmtiRedefineClasses.cpp
line 3547: // print error if old methods are found.
We fail a guarantee here also so this is more than "print error".
line 3551 } else {
Indent is off by one space (not your change, but...)
src/share/vm/prims/jvmtiRedefineClasses.hpp
No comments.
test/testlibrary/ClassFileInstaller.java
line 43: String pathName = arg.replace('.', '/').concat(".class");
line 48: if (pathName.contains("/")) {
Should these be System.getProperty("path.separator")? Or maybe
this is something already queried and cached in this testing
harness?
test/runtime/RedefineObject/Agent.java
line 56: for (int i = 0; i< 1000 ; i++) {
Space before '<'.
test/runtime/RedefineObject/MANIFEST.MF
No comments (agree with Christian that this can be deleted)
test/runtime/RedefineObject/Test.java
No comments (agree with Christian that this should be renamed).
Dan
On 5/9/13 8:22 AM, Coleen Phillimore wrote:
Dan,
Thank you for adding serviceability. I'll provide more detail on the
change if needed. Here's some additional detail.
What I did was added a closure to pass to
ClassLoaderDataGraph::classes_do() function. This function walks all
the loaded classes, which includes the array classes. The
SystemDicitonary walk only walks loaded InstanceKlasses. The array
classes created are linked from the InstanceKlass in _array_klasses
and the code used to walk them separately. The code for switching to
the new methods from the old methods didn't change very much.
The reason we have to change the vtables in array classes is because
array classes are inherited from java/lang/Object class and have this
vtable. We had missed the ones for arrays of basic types created in
universe. The old code never fixed these entries but it didn't crash
because the methodOops were followed with the basic type array classes
in their vtable so wouldn't go away. With permgen removal, we
explicitly delete unreferenced Method objects so these ones have to be
replaced. And it's more correct because you don't want to call the
old Method.
Coleen
On 05/09/2013 09:55 AM, Daniel D. Daugherty wrote:
Adding Serviceability to this review thread since this concerns
JVM/TI RedefineClasses().
Coleen, this will take a bit of time to review.
Dan
On 5/8/13 8:51 PM, Coleen Phillimore wrote:
Summary: Need to walk array class vtables replacing old methods too
if j.l.o is redefined
Array methods aren't in the SystemDictionary and the code that was
there didn't walk the basic type array classes defined in
universe. It also walked the same classes more than once. Use the
ClassLoaderDataGraph class walking instead.
open webrev at http://cr.openjdk.java.net/~coleenp/8005056/
bug link at http://bugs.sun.com/view_bug.do?bug_id=8005056
Tested with all redefine classes tests, jdk java/lang/instrument
tests, hotspot jtreg tests.
Thanks,
Coleen
