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

[harold seigel]

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