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

[Coleen Phillimore]

On 3/2/16 9:29 PM, David Holmes 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.

I see Mandy's reply afterward, but it's really an opinion question and I 
don't think there's a strong technical or correctness reason to do it 
one way rather than the other.  The preference is to write most code in 
Java and it is faster to allocate these in Java.  But this isn't a 
critical path for performance, it just helps.
>
>> 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 ?? :)
>

No idea!
> 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.)

I was being more cautious here but the one above is fine to use.
>
> 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.
>
I was being overly cautious even though the only caller is the code in 
the jdk in Throwable.  I could just check for NULL and if it's an array, 
but then I decided to check the element type.  Actually, there's an 
assert later for element type.  I can take it out. You're right, we 
don't check if throwable is a Throwable.

If I leave it in, what else should I throw?

> test/runtime/Throwable/TestThrowable.java
>
> Does that reflection hack still work in a modular VM?

Good thing Mandy answered that.

Thanks!
Coleen
>
> 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
>>>>
>>>
>>