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

[Thomas Stüfe]

Hi Ralf,

patch looks good and makes sense.


Some remarks:

+    /* Should not usually happen. */
+    if (length != count) {
+        error = JVMTI_ERROR_INTERNAL;
+    }

Cosmetics: I would also probably explicitly return:

/* Should not usually happen. */
if (length != count) {
  jvmtiDeallocate(frames);
  outStream_setError(out, JDWP_ERROR(INTERNAL));
  return JNI_TRUE;
}

.. makes the code clearer and should someone change the loop cancel
condition it will still work.

======

+    for (fnum = 0; (fnum < count) && (error == JVMTI_ERROR_NONE); ++fnum) {

you could loose the inner brackets.

--

Cosmetics: you changed meaning of fnum. Before it was really the frame
number. Now, fnum is a zero based index into your array. So I would
probably have renamed the variable too, maybe index? or somesuch.

======

Do we not have to handle opaque frames like the code before did? Or
does GetStackTrace already filter out opaque frames? Would that not
mean that GetStackTrace returns fewer frames than expected, and then
count could be smaller than length?

-- oh wait I see GetFrameLocation never really returned
JVMTI_ERROR_OPAQUE_FRAME? So it is probably fine.

=====

How large can the depth get? In stack overflow scenarios?

To limit memory usage and to make it more predictable, I would not
retrieve all frames in one go but in a loop, in bulks a n frames. E.g.
4086 frames would mean your buffer never exceeds 64K on 64bit
platforms. You would sacrifice a tiny bit of performance (again
needless walking up to starting position) but would not choke out when
stacks are ridiculously large.

======

I cannot comment on the jtreg test. Looks fine to me, but I wonder
whether there is a better way to script jdb, is this how we are
supposed to do this?

Maybe someone from the Oracle serviceability group can comment.


Thanks & Best Regards, Thomas


On Tue, Jul 3, 2018 at 12:43 PM, Schmelter, Ralf <ralf.schmelter at sap.com> wrote:
> Hi All,
>
> Please review the fix for the bug https://bugs.openjdk.java.net/browse/JDK-8205608 . The webref is at http://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