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

[Chris Plummer]

Hi Ralf,

Changes look good and pass all the testing I did. You can push once 
Serguei approves.

thanks,

Chris

On 7/20/18 7:28 AM, Schmelter, Ralf wrote:
> Hi Sergue,
>
> I’ve updated the webref: http://cr.openjdk.java.net/~simonis/webrevs/2018/8205608.v4/
>
> JVMTI_ERROR_OPAQUE_FRAME was never returned from GetFrameLocation(). If it would have, the old code would have removed all native methods from the call stack. The original JVMDI call did indeed return JVMDI_ERROR_OPAQUE_FRAME, so maybe it was a leftover from the JVMDI->JVMTI transition.
>
> I’ve tried to make the test more readable and added some comments to explain why it is done the way it is.
>
> Best regards,
> Ralf
>
>
>
>
> From: serguei.spitsyn at oracle.com [mailto:serguei.spitsyn at oracle.com]
> Sent: Mittwoch, 18. Juli 2018 22:57
> To: Chris Plummer <chris.plummer at oracle.com>; Schmelter, Ralf <ralf.schmelter at sap.com>; serviceability-dev at openjdk.java.net; Stuefe, Thomas <thomas.stuefe at sap.com>
> Subject: Re: RFR (S) 8205608: Fix 'frames()' in ThreadReferenceImpl.c to prevent quadratic runtime behavior
>
> 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