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

[Chris Plummer]

Hi Ralf,

Looks good.

thanks,

Chris

On 7/18/18 8:44 AM, Schmelter, Ralf wrote:
> Hi Chris,
>
> here is an updated webref http://cr.openjdk.java.net/~simonis/webrevs/2018/8205608.v3/
>
> I've changed the summary text and the comment according to your suggestion.
>
> The 100M frames is surely overkill for this test. I had seen that the JIT compiler started to inline, leading to less memory needed per frame. But I've never got more than 1M frames even for very big stacks. Therefore I've reduced it in the test to 1M.
>
> When the stack overflow never occurs and callEnded() thus never gets called, the test will fail, because
> bpe = resumeTo("Frames2Targ", "callEnded", "()V");
> will fail since the VM will exit and never reach the breakpoint. In addition, a message will be written about the missing SOE.
>
> Best regards,
> Ralf
>
>
> -----Original Message-----
> From: Chris Plummer [mailto:chris.plummer at oracle.com]
> Sent: Mittwoch, 18. Juli 2018 07:02
> To: Schmelter, Ralf <ralf.schmelter at sap.com>; serviceability-dev at openjdk.java.net; serguei.spitsyn at oracle.com; Stuefe, Thomas <thomas.stuefe at sap.com>
> Subject: Re: RFR (S) 8205608: Fix 'frames()' in ThreadReferenceImpl.c to prevent quadratic runtime behavior
>
> 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
>