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

[Chris Plummer]

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-bounces at openjdk.java.net] On Behalf Of Schmelter, Ralf
> Sent: Montag, 9. Juli 2018 16:05
> To: Chris Plummer <chris.plummer at oracle.com>; serviceability-dev at openjdk.java.net; serguei.spitsyn at 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.plummer at oracle.com]
> Sent: Freitag, 6. Juli 2018 00:37
> To: Schmelter, Ralf <ralf.schmelter at sap.com>; serviceability-dev at openjdk.java.net; serguei.spitsyn at 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
>