On 7/20/18 1:40 PM, serguei.spit...@oracle.com wrote:
Hi Ralf,
On 7/20/18 07:28, Schmelter, Ralf wrote:
Hi Sergue,
I’ve updated the webref:
http://cr.openjdk.java.net/~simonis/webrevs/2018/8205608.v4/
The copyright year in ThreadReferenceImpl.c still has to be 2018, not
2008.
http://cr.openjdk.java.net/~simonis/webrevs/2018/8205608.v4/test/jdk/com/sun/jdi/Frames2Test.java.html
72 if (newDepth == -1_000) {
73 // Pop some frames so there is room on the stack
for the
74 // call (including println()).
75 notifyRecursionEnded();
76 }
I have a concern on potential issue mentioned in the comment above.
Should a StackOverflowError be expected here?
79 } catch (StackOverflowError e) {
80 // Use negative depth to indicate the recursion has
ended.
81 return -1;
82 }
What is going to happen if the StackOverflowError was really caught
above?
The SOE is really caught in the above code. I returns -1, and starts the
unwinding of the stack. After 1000 frames have been popped via returns,
notifyRecursionEnded() will be called. The pops are so
notifyRecursionEnded() can be called without worry of another SOE.
Chris
If I understand it correctly, the notifyRecursionEnded() call will
be missed then.
This breakpoint will be missed as well:
107 bpe = resumeTo("Frames2Targ", "notifyRecursionEnded",
"()V");
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.
Agreed.
I’ve tried to make the test more readable and added some comments to
explain why it is done the way it is.
Thank you for the update!
Thanks,
Serguei
Best regards,
Ralf
From: serguei.spit...@oracle.com [mailto:serguei.spit...@oracle.com]
Sent: Mittwoch, 18. Juli 2018 22:57
To: Chris Plummer <chris.plum...@oracle.com>; Schmelter, Ralf
<ralf.schmel...@sap.com>; serviceability-dev@openjdk.java.net;
Stuefe, Thomas <thomas.stu...@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
