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

[Coleen Phillimore]

David, Thanks - a comment in the middle.

On 3/2/16 11:13 PM, David Holmes wrote:
> On 3/03/2016 1:11 PM, Coleen Phillimore wrote:
>>
>>
>> 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.
>
> I'm not sure how it can be faster to allocate in Java when the VM does 
> the allocation anyway ?? It is certainly easier to write the array 
> initialization code in the Java. The ugly part of doing it in Java is 
> that the VM has to poke the right length into the depth field. From an 
> API design perspective it is cleaner for the called function to 
> allocate exactly the size and type of array needed rather than having 
> to communicate that information out, and then add checks to make sure 
> a different sized (or typed) array is not passed in.
>
> Just my 2c.
>
>>>
>>>> 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?
>
> ClassCastException would be the closest thing to signify an incorrect 
> type. But as this is a private internal API that should never be 
> called incorrectly then asserts regarding the array type should be fine.

I like the idea of an assert better myself.  Nobody will expect 
Throwable.<init> to throw this exception.

And it's a private API.

Thanks!
Coleen
>
>>> test/runtime/Throwable/TestThrowable.java
>>>
>>> Does that reflection hack still work in a modular VM?
>>
>> Good thing Mandy answered that.
>
> :) Yeah I haven't caught up with Jigsaw yet.
>
> Cheers,
> David
>
>> 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
>>>>>>
>>>>>
>>>>
>>