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

[David Holmes]

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.

>> 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
>>>>>
>>>>
>>>
>