RFR 8150778: Reduce Throwable.getStackTrace() calls to the JVM

[Mandy Chung]

> On Mar 2, 2016, at 6:29 PM, David Holmes <david.holmes at oracle.com> wrote:
> 
> Hi Coleen,
> 
> On 3/03/2016 10:03 AM, Coleen Phillimore wrote:
>> Freshly tested changes with jck tests, with missing checks and other
>> changes to use the depth field, as pointed out by Aleksey.  I've kept
>> the StackTraceElement[] allocation in Java to match the new API that was
>> approved.
> 
> I would have argued for returning a constructed array instead but if that decision has already been made then it has already been made.

Correct me if I’m wrong, AFAIK, allocating objects in Java is faster than done in VM.  This JVM_GetStackTraceElements function is only used by Throwable. Generally I prefer to keep things done in Java rather than in VM. I don’t have a strong opinion on this case as VM has to fill in StackTraceElement fields anyway.

> 
>> open webrev at http://cr.openjdk.java.net/~coleenp/8150778.02_hotspot/
>> open webrev at http://cr.openjdk.java.net/~coleenp/8150778.02_jdk/
> 
> Overall this looks good - makes me wonder why we didn't do it this way earlier ?? :)
> 
> javaClasses.cpp:
> 
> 2044   {
> 2045     ResourceMark rm;
> 2046     log_info(stacktrace)("%s, %d", throwable->klass()->external_name(), chunk_count);
> 2047   }
> 
> There is already a ResourceMark earlier in the method. (Otherwise it should be inside a "log is enabled" guard.)
> 
> 2063   if (throwable.is_null() || stack_trace_array_h.is_null() ||
> 2064       !is_StackTraceElementArray(stack_trace_array_h)) {
> 2065     THROW(vmSymbols::java_lang_NullPointerException());
> 2066   }
> 
> Not sure the type check for the array is needed (we don't check the throwable object really is a Throwable) - but if it is then throwing NPE seems misleading.
> 
> test/runtime/Throwable/TestThrowable.java
> 
> Does that reflection hack still work in a modular VM?
> 

java.lang.Throwable::depth is in an exported package and so it will work when module access check is enforced.

If a test accesses a non-exported package, jtreg has added @modules tag for tests to specify what non-exported packages a test needs to access and jtreg will run the test with -XaddExports to set up the access.

Mandy

> Thanks,
> David
> 
>> Thanks,
>> Coleen
>> 
>> On 3/2/16 2:57 PM, Coleen Phillimore wrote:
>>> 
>>> 
>>> On 3/2/16 1:58 PM, Aleksey Shipilev wrote:
>>>> Hi Coleen,
>>>> 
>>>> On 03/02/2016 09:44 PM, Coleen Phillimore wrote:
>>>>> Summary: replace JVM_GetStackTraceDepth and JVM_GetStackTraceElement,
>>>>> with JVM_GetStackTraceElements that gets all the elements in the
>>>>> StackTraceElement[]
>>>>> 
>>>>> These improvements were found during the investigation for replacing
>>>>> Throwable with the StackWalkAPI.   This change also adds iterator for
>>>>> BacktraceBuilder to make changing format of backtrace easier.
>>>>> 
>>>>> Tested with -testset core, RBT nightly hotspot nightly tests on all
>>>>> platforms, and jck tests on linux x64.  Compatibility request is
>>>>> approved.
>>>>> 
>>>>> open webrev at http://cr.openjdk.java.net/~coleenp/8150778_jdk/
>>>>> open webrev at http://cr.openjdk.java.net/~coleenp/8150778_hotspot
>>>>> bug link https://bugs.openjdk.java.net/browse/JDK-8150778
>>>> Looks interesting!
>>>> 
>>>> Is there an underlying reason why we can't return the pre-filled
>>>> StackTraceElements[] array from the JVM_GetStackTraceElements to begin
>>>> with? This will avoid leaking StackTraceElement constructor into
>>>> standard library, *and* allows to make StackTraceElement fields final.
>>>> Taking stuff back from the standard library is hard, if not impossible,
>>>> so we better expose as little as possible.
>>> 
>>> We measured that it's faster to allocate the StackTraceElement array
>>> in Java and it seems cleaner to the Java guys.
>>> It came from similar code we've been prototyping for StackFrameInfo.
>>>> 
>>>> Other minor nits:
>>>> 
>>>>  * Initializing fields to their default values is a code smell in Java:
>>>>      private transient int depth = 0;
>>> 
>>> ok, not sure what "code smell" means but it doesn't have to be
>>> initialized like this.  It's set in the native code.
>>>> 
>>>>  * Passing a null array to getStackTraceElement probably SEGVs? I don't
>>>> see the null checks in native parts.
>>> 
>>> Yes, it would SEGV.  I'll add some checks for null and make sure it's
>>> an array of StackTraceElement.
>>> 
>>> Thanks,
>>> Coleen
>>>> 
>>>> Thanks,
>>>> -Aleksey
>>>> 
>>> 
>>