Review Request: 8006506: Add test for redefining methods in backtraces to java/lang/instrument tests (original) (raw)

David Holmes david.holmes at oracle.com
Fri Feb 1 03:48:15 PST 2013

Hi Stefan,

This part of the test:

86 private static void doClassUnloading() { 87 // This will clean out old, unused redefined methods. 88 System.gc(); 89 }

seems to make assumptions about System.gc() and class unloading. Are we relying on knowledge of hotspot internals here?

I don't have a good suggestion to avoid this. This topic came up on another thread today regarding weak references. For that case we can typically add a loop and sleep until we see the reference clear, but here ... how can you check that class unloading occurred? Actually is that really what is happening - I don't see how any class unloading will occur here. I can imagine that GC might cleanup unreferenced methods.

David

On 1/02/2013 9:27 PM, Stefan Karlsson wrote:

On 2013-02-01 12:17, Stefan Karlsson wrote:

On 2013-02-01 12:11, serguei.spitsyn at oracle.com wrote:

On 2/1/13 1:57 AM, Stefan Karlsson wrote:

On 2013-02-01 10:22, serguei.spitsyn at oracle.com wrote:

Nice test! It looks good. Thanks for reviewing!

As the original bug and the test are non-trivial, it'd make sense to add a comment to the class RedefineMethodInBacktraceApp and explain a little bit what the test is doing, and what behavior is expected. http://cr.openjdk.java.net/~stefank/8006506/webrev.04/ Tell me if you think this is good enough. It is good. Nit: it'd be enough if it is more specific. :) This method is a key point: 90 private static void touchRedefinedMethodInBacktrace(Throwable throwable) { 91 throwable.getStackTrace(); 92 } Is it true that the test expects the getStackTrace() does not crash nor throw an exception which would happen if the old/obsolete method is gc'ed? I see. I'll add a comment that we shouldn't crash. http://cr.openjdk.java.net/~stefank/8006506/webrev.05/ StefanK Thanks, StefanK Thanks, Serguei

thanks, StefanK

Thanks, Sergueri On 2/1/13 12:13 AM, Stefan Karlsson wrote: http://cr.openjdk.java.net/~stefank/8006506/webrev.03/ 1) Reverted the ProblemList change, since the fix has already propagaged to jdk8/tl 2) Renamed doredefine -> doRedefine 3) Updated the .sh file with the bug number of the original CR instead of the test CR. thanks, StefanK On 2013-01-22 14:11, Stefan Karlsson wrote: http://cr.openjdk.java.net/~stefank/8006506/webrev.00/ This test provokes the JVM crash described in bug: JDK-7174978. I intend to push this to: http://hg.openjdk.java.net/jdk8/tl/jdk thanks, StefanK

More information about the serviceability-dev mailing list