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

[David Holmes]

A belated "looks okay to me " :)

Thanks,
David

On 10/03/2016 7:12 AM, Coleen Phillimore wrote:
>
> Hi Harold,
> Thank you for the code review!
>
> I added a test for the stack trace logging.  I also made some other
> minor changes that you pointed out.  I also added a function to find the
> field offset from a char* which is used by "depth", but that can be used
> by other things in javaClasses.cpp that need to know field offsets
> without adding to vmSymbols.hpp.
>
> Also, I made the assertion changes discussed with David in this
> version.   Reran some jck tests.
>
> open webrev at http://cr.openjdk.java.net/~coleenp/8150778.03_hotspot/
> open webrev at http://cr.openjdk.java.net/~coleenp/8150778.03_jdk/
>
> I didn't change jdk since 02.
>
> Thanks,
> Coleen
>
>
> On 3/9/16 2:35 PM, harold seigel wrote:
>> Hi Coleen,
>>
>> The JVM changes look good.  Could you add a test or test case for the
>> new 'stacktrace' logging tag?
>>
>> Thanks, Harold
>>
>> On 3/2/2016 7:03 PM, 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.
>>>
>>> open webrev at http://cr.openjdk.java.net/~coleenp/8150778.02_hotspot/
>>> open webrev at http://cr.openjdk.java.net/~coleenp/8150778.02_jdk/
>>>
>>> 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
>>>>>
>>>>
>>>
>>
>