Code Review for JEP 259: Stack-Walking API

[Coleen Phillimore]

On 11/18/15 5:06 PM, Mandy Chung wrote:
>> On Nov 18, 2015, at 1:01 PM, Coleen Phillimore <coleen.phillimore at oracle.com> wrote:
>>
>>
>> One of the things that I'm struggling with is that StackFrameInfo contains both the collected information from walking the stack frames, method id, bci, mirror, version and cpref, and the digested information: interned string for class name, method name, line number and source file name.
>>
> method id, mirror, version and cpref are injected in StackFrameInfo.  I hope there is a way to make it conditional only if -XX:-MemberNameInThrowable is set (is it possible?)

That's really hard to do with the nice macros we have now in javaClasses.
>
> -XX:-MemberNameInThrowable is temporary and disabled by default.  It is used for follow-on performance improvement as we discussed previously.   I still believe that there may be some low hanging fruit to reduce the initialization of MemberName for an already-linked method.    We should aim to remove this flag in JDK 9 so that StackFrameInfo will have only MemberName and bci.

Given that that we were trying to stick to the original feature freeze 
date for 9, I don't think the performance of the MethodHandles code 
would make it in time.  There are some big problems with it, notably 
that it creates weak references for each MemberName and the GC people 
are not going to like that.   We have not to-date found a better 
solution for this to support redefinition.

I think if StackFrameInfo was trimmed to just what was needed for 
collecting the information during stack traces, it would be possible to 
make the new implementation performant enough to be low risk for 9 *and* 
would allow for removing the duplicated code in BacktraceBuilder.  This 
would be a very promising improvement!
>
> The interned string for class name, method name, line number and source file name are requested lazily when StackFrame::getMethodName or other methods are called.  They are not eagerly allocated.

But the fields in StackFrameInfo are present for each element in the 
stack trace.  We had problems with GC scavenge times when we increased 
the size of the backtrace that we collect today.   The StackFrameInfo 
struct would be similarly sized if you didn't all the fields from 
StackTraceElement, so it would be good.
>
>> If this is to replace stack walking for exceptions, this will more than double the footprint of the information collected but rarely used. I don't understand why the digested information isn't still StackFrameElement?
>>
> If Throwable uses StackWalker, I expect it to use MemberName and -XX:-MemberNameInThrowable should be removed by that time.   Also VM no longer needs to fill in StackTraceElement as it should fill in StackFrameInfo.   java_lang_StackTraceElement in javaClasses.[hc]pp can be removed at an appropriate time :)

I don't know why StackTraceElement should be removed.  What's wrong with 
StackTraceElement?
>
> Throwable backtrace will keep an array of StackFrameInfo, one element per frame.  StackFrameInfo only captures the MemberName + bci.

Right (or the combination of things that we save now in the backtraces 
for efficiency).


> When Throwable::getStackTraceElements() or printStackTrace() is called, the VM will allocate the intern strings for those names and fill in StackFrameInfo.  Then convert them to StackTraceElement[] and throw away StackFrameInfo[].   This is just the current implementation.   I expect further optimization can be done in the JDK side about StackTraceElement and StackFrameInfo.

This sounds really inefficient!  Why not fill in StackTraceElement 
directly?  And keep it?

Even in Java, having one class represent two different things isn't very 
object oriented.

>> That's just a high level comment.  I haven't read the java code yet for the rationale why this type is used for two different things.
>>
> The way I implement it is to prepare Throwable backtrace + StackTraceElement be replaced by StackFrameInfo in the VM.
>
> The VM fills in StackFrameInfo for StackWalker.   When Throwable switches to use StackWalker, VM doesn’t need to know anything about StackTraceElement.

I don't see the advantage of this. 
java_lang_StackFrameInfo::fill_methodInfo() could just fill in a Java 
allocated array of StackTraceElement instead.  Again, making 
StackFrameInfo so large will have footprint and GC performance 
implications when it's almost always thrown away.  I included GC in the 
mailing list.  Hopefully with enough context if they want to comment.

thanks,
Coleen

>
> Mandy
>
>