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

[Coleen Phillimore]

Thank you, Harold!
Coleen

On 3/10/16 9:56 AM, harold seigel wrote:
> Hi Coleen,
>
> The changes look good.  Thanks for adding the new test.
>
> Harold
>
> On 3/9/2016 4:51 PM, Coleen Phillimore wrote:
>>
>>
>> On 3/9/16 4:12 PM, 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.
>>
>> I reverted this last change because it used CATCH to exit if there's 
>> an exception, ie didn't properly handle errors.  To pass a name to 
>> compute_offset() you need to add the string to vmSymbols.hpp for now.
>>
>> Also hg added the test.
>>
>> Thanks,
>> Coleen
>>>
>>> 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
>>>>>>>
>>>>>>
>>>>>
>>>>
>>>
>>
>