RFR 8153123 : Streamline StackWalker code

[Brent Christian]

Hi, Daniel
On 04/07/2016 04:24 AM, Daniel Fuchs wrote:
>
> In
> http://cr.openjdk.java.net/~bchristi/8153123/webrev.00/hotspot/src/share/vm/prims/jvm.cpp.frames.html
>
>   548   objArrayOop fa = objArrayOop(JNIHandles::resolve_non_null(frames));
>   549   objArrayHandle frames_array_h(THREAD, fa);
>   550
>   551   int limit = start_index + frame_count;
>   552   if (frames_array_h.is_null()) {
>   553     THROW_MSG_(vmSymbols::java_lang_IllegalArgumentException(),
> "parameters and mode mismatch", NULL);
>   554   }
>
> Can frames_array_h.is_null() ever be true, given that we used
> JNIHandles::resolve_non_null(frames) at line 548?

No!  As you point out, it will assert out at 548.

> I wonder if lines 552-554 are a remnant of the previous
> implementation and could be removed now...

You're absolutely right.

>   589   Handle stack_frame_info(THREAD, JNIHandles::resolve(frame));
>   590   Handle stack_trace_element(THREAD, JNIHandles::resolve(stack));
>
> Should these call JNIHandles::resolve_non_null instead?

Yes!

> http://cr.openjdk.java.net/~bchristi/8153123/webrev.00/jdk/src/java.base/share/classes/java/lang/StackFrameInfo.java.frames.html
>
> I'd be very tempted to make 'ste' private volatile.

Sounds good to me.


Thank you for having a look.  I will send:
   http://cr.openjdk.java.net/~bchristi/8153123/webrev.02/
to hs-rt shortly.

-Brent

> On 05/04/16 01:45, Brent Christian wrote:
>> Hi,
>>
>> I'd like to check in some footprint and code reduction changes to the
>> java.lang.StackWalker implementation.
>>
>> Webrev:
>> http://cr.openjdk.java.net/~bchristi/8153123/webrev.00/
>> Bug:
>> https://bugs.openjdk.java.net/browse/JDK-8153123
>>
>> A summary of the changes:
>>
>> * remove the "stackwalk.newThrowable" system property and
>> "MemberNameInStackFrame" VM flag, originally left in to aid benchmarking
>>
>> * Streamline StackFrameInfo fields
>>
>> * Refactor/streamline StackStreamFactory (no more separate
>> classes[]/StackFrame[] arrays, remove unneeded (for now)
>> StackStreamFactory.StackTrace class)
>>
>>
>> Given the hotspot changes, I plan to push this through hs-rt.
>>
>> Thanks,
>> -Brent
>>
>