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

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

Hi Ralf, I have some questions. http://cr.openjdk.java.net/~simonis/webrevs/2018/8205608/src/jdk.jdwp.agent/share/native/libjdwp/ThreadReferenceImpl.c.frames.html 288 if (length != count) { 289 error = JVMTI_ERROR_INTERNAL; 290 }   The above looks as incorrect because it does not account for the startIndex value.   Also, if the condition is true then it should skip the loop at the lines 292-303. 292 for (fnum = 0; (fnum < count) && (error == JVMTI_ERROR_NONE); ++fnum) { 293 WITH_LOCAL_REFS(env, 1) { 294 jclass clazz; 295 error = methodClass(frames[fnum].method, &clazz); 296 297 if (error == JVMTI_ERROR_NONE) { 298 FrameID frame = createFrameID(thread, fnum + startIndex); 299 outStream_writeFrameID(out, frame); 300 writeCodeLocation(out, clazz, frames[fnum].method, frames[fnum].location); 301 } 302 } END_WITH_LOCAL_REFS(env); 303 }    It seems, there has to be a break in the loop at lines 292-303 above    if the condition at the line 297 is not true.    Please, find a similarity in the original code lines 303-304: 303 if (error != JVMTI_ERROR_NONE) 304 break; Also, it looks as incorrect to send the packages if the such an error was identified. But this issue exists in the original implementation as well. On 7/5/18 15:37, Chris Plummer wrote: Hi Ralf, Overall looks good, but I do have a few comments and questions. Please update the copyright. What testing have you done? +1 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. The test has to be the TestScaffold based. You can find many examples in the jdk/com/sun/jdi folder. Thanks, Serguei 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