Re: RFR (S) 8205608: Fix 'frames()' in ThreadReferenceImpl.c to prevent quadratic runtime behavior

[serguei.spit...@oracle.com]

Hi Ralf, The fix itself looks pretty good to me. Some minor comments. The copyright year needs an update. 218 jint count, filledIn; Could you, please, split the declarations above into different lines to follow the local style? Ii is interesting that the original implementation checked the error code returned from the JVMTI GetFrameLocation for being equal to JVMTI_ERROR_OPAQUE_FRAME. However, the GetFrameLocation spec does not list this error code as possible. Some comments about the test. 52 static void callEnded() { 53 System.out.println("SOE occurred as expected"); 54 } 55 56 static int call(int depth) { 57 if (depth == 0) { 58 // Should have seen a stack overflow by now. 59 System.out.println("Exited without creating SOE"); 60 System.exit(0); 61 } 62 63 try { 64 int newDepth = call(depth - 1); 65 66 if (newDepth == -1_000) { 67 // Pop some frames so there is room on the stack for the 68 // println() 69 callEnded(); 70 } 71 72 return newDepth - 1; 73 } catch (StackOverflowError e) { 74 return -1; 75 } 76 } 77 }   I'd suggest to rename the methods call() and callEnded() to something like   recursiveMethod() and recursionEnd().   Also, the manipulations with SOE create a complexity and are confusing.   Could it be more simple to let it propagated and then catch in main()?   What is the point for all these checks at the lines 104-119?   In general, I'm looking for some ways to make it more clear, simple and stable. Thanks, Serguei On 7/18/18 10:10, Chris Plummer wrote: Hi Ralf, Looks good. thanks, Chris On 7/18/18 8:44 AM, Schmelter, Ralf wrote: Hi Chris, here is an updated webref http://cr.openjdk.java.net/~simonis/webrevs/2018/8205608.v3/ I've changed the summary text and the comment according to your suggestion. The 100M frames is surely overkill for this test. I had seen that the JIT compiler started to inline, leading to less memory needed per frame. But I've never got more than 1M frames even for very big stacks. Therefore I've reduced it in the test to 1M. When the stack overflow never occurs and callEnded() thus never gets called, the test will fail, because bpe = resumeTo("Frames2Targ", "callEnded", "()V"); will fail since the VM will exit and never reach the breakpoint. In addition, a message will be written about the missing SOE. Best regards, Ralf -----Original Message----- From: Chris Plummer [mailto:chris.plum...@oracle.com] Sent: Mittwoch, 18. Juli 2018 07:02 To: Schmelter, Ralf <ralf.schmel...@sap.com>; serviceability-dev@openjdk.java.net; serguei.spit...@oracle.com; Stuefe, Thomas <thomas.stu...@sap.com> Subject: Re: RFR (S) 8205608: Fix 'frames()' in ThreadReferenceImpl.c to prevent quadratic runtime behavior Hi Ralf, A few comments below, but overall looks good:     27  * @summary get stack trace for large stacks took too long. How about "Test that getting the stack trace for a very large stack does not take too long". The max number of frames you'll test for is 100M, but the stack size is set to 4m, assuming -Xss works (and I think on some platforms it may not). 100M frames seems like overkill for a 4M stack. If the stack was nothing more than a frame link pointer on a 32-bit system, you'd only have 1M frames, but lets be more realistic than that and say you should never have more than 256k frames. Lowering the max number of frames will prevent this test from taking a very long time on platforms where -Xss has failed.     65                 // Have some frames be removed before we call again. Should this be: "Pop some frames so there is room on the stack for the println()"     96         bpe = resumeTo("Frames2Targ", "callEnded", "()V"); What happens if we never get to callEnded()? thanks, Chris On 7/17/18 7:08 AM, Schmelter, Ralf wrote: Hi all, here is an updated webref at http://cr.openjdk.java.net/~simonis/webrevs/2018/8205608.v2/ I've converted the shell based test to a Java one, which had the nice side effect of speeding it up (now takes ~1 second runtime). The fix itself is mainly unchanged, but I've added the variable 'filledIn' to store the number of frames actually filled in by the GetStackTrace call. Formerly I've reused the count variable, but this can lead to misunderstandings. Best regards, Ralf -----Original Message----- From: serviceability-dev [mailto:serviceability-dev-boun...@openjdk.java.net] On Behalf Of Schmelter, Ralf Sent: Montag, 9. Juli 2018 16:05 To: Chris Plummer <chris.plum...@oracle.com>; serviceability-dev@openjdk.java.net; serguei.spit...@oracle.com Subject: [CAUTION] RE: RFR (S) 8205608: Fix 'frames()' in ThreadReferenceImpl.c to prevent quadratic runtime behavior Hi Chris, thanks for the review. What testing have you done? I've tested the change by debugging by hand in eclipse and jdb, running the com/sun/jdi rtreg tests and the jdwp jck tests. And analog code is running in the SAP JVM for many years. How long does this test take to run. 15 s according to jtreg. What happens if for some reason SOE is never thrown? It's not clear to me what the script would do in this case. It is treated as passed (which is not ideal). In answer to the ShellScaffold.sh question, there is already work underway to convert to pure java tests. See JDK-8201652. Ok, then I think it is better to convert the test to a Java TestScaffold test. I will update the webref when this is done. Best regards, Ralf -----Original Message----- From: Chris Plummer [mailto:chris.plum...@oracle.com] Sent: Freitag, 6. Juli 2018 00:37 To: Schmelter, Ralf <ralf.schmel...@sap.com>; serviceability-dev@openjdk.java.net; serguei.spit...@oracle.com Subject: Re: RFR (S) 8205608: Fix 'frames()' in ThreadReferenceImpl.c to prevent quadratic runtime behavior Hi Ralf, Overall looks good, but I do have a few comments and questions. Please update the copyright. What testing have you done? How long does this test take to run. What happens if for some reason SOE is never thrown? It's not clear to me what the script would do in this case. In answer to the ShellScaffold.sh question, there is already work underway to convert to pure java tests. See JDK-8201652. I'm not certain if it is ok for you to just submit this new shell script, or if should be rewritten in pure java. Most of the work to convert the scripts has already been done but was put on hold. Maybe Serguei can comment and guide you on how it would be done in java. thanks, Chris On 7/3/18 3:43 AM, Schmelter, Ralf wrote: Hi All, Please review the fix for the bughttps://bugs.openjdk.java.net/browse/JDK-8205608  . The webref is athttp://cr.openjdk.java.net/~simonis/webrevs/2018/8205608/  . This fixes the quadratic runtime (in the number of frames) of the frames() method, making it linear instead. It uses additional memory proportional to the number of frames on the stack. I've included a jtreg test, which would time out in the old implementation (since it takes minutes to get the stack frames). I'm not sure how useful this is. Best regards, Ralf Schmelter