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

Thu, 05 Jul 2018 19:37:24 -0700

On 7/5/18 19:17, serguei.spit...@oracle.com wrote:
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.

Please, skip the entire last comment as incorrect.
(Thanks to David Holmes for the catch!)
I've overlooked the error check in the loop header.
The part about lines 303-304 is incorrect too.

Thanks,
Serguei


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




Reply via email to