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

[Chris Plummer]

Oops. Sorry, that testing comment was for another changeset. I didn't 
test your changes. If you think they could use some additional testing 
on some more platforms, let me know.

thanks,

Chris

On 7/20/18 1:07 PM, Chris Plummer wrote:
> 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
>
>
>