Code Review for JEP 259: Stack-Walking API

[Mandy Chung]

I want to make sure the key point is getting across.

This change does not change anything in Throwable, backtrace and StackTraceElement.  StackFrameInfo is used by the new StackWalker API.  JDK Thread::dumpStack, Thread::getStackTrace and LogRecord/PlatformLogger are updated to use the stack walker API.   There is no impact to the runtime performance of existing code in terms of exceptions.   -Dstackwalk.newThrowable system property is set to false that indicates whether Throwable is not using the new StackWalker API.

It’d be good if the GC team can take a look at this change.

Your comment below is concerning the follow-on work where we need to work together to determine if Throwable can switch to use StackWalker/StackFrameInfo API.  If it’s determined that there is no benefit using StackWalker API for exceptions, we will clean up the implementation for that.  However this requires more performance investigation and data to determine that.

Hope this clarifies it.

Mandy

> On Nov 18, 2015, at 3:42 PM, Coleen Phillimore <coleen.phillimore at oracle.com> wrote:
> 
> 
> 
> 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
>> 
>> 
>