Re: RFR 8153123 : Streamline StackWalker code

[Brent Christian]

On 04/07/2016 04:33 PM, Brent Christian wrote:

I will send:
   http://cr.openjdk.java.net/~bchristi/8153123/webrev.02/
to hs-rt shortly.


...after adding the following :)

diff -r f628b87a6067 makefiles/symbols/symbols-unix
--- a/makefiles/symbols/symbols-unixFri Apr 08 13:14:23 2016 +0200
+++ b/makefiles/symbols/symbols-unixFri Apr 08 12:22:14 2016 -0700
@@ -58,7 +58,6 @@
 JVM_DumpAllStacks
 JVM_DumpThreads
 JVM_FillInStackTrace
-JVM_FillStackFrames
 JVM_FindClassFromCaller
 JVM_FindClassFromClass
 JVM_FindLibraryEntry
@@ -169,7 +168,6 @@
 JVM_ResumeThread
 JVM_SetArrayElement
 JVM_SetClassSigners
-JVM_SetMethodInfo
 JVM_SetNativeThreadName
 JVM_SetPrimitiveArrayElement
 JVM_SetThreadPriority
@@ -178,6 +176,7 @@
 JVM_StopThread
 JVM_SupportsCX8
 JVM_SuspendThread
+JVM_ToStackTraceElement
 JVM_TotalMemory
 JVM_UnloadLibrary
 JVM_Yield

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

Re: RFR 8153123 : Streamline StackWalker code

[Daniel Fuchs]

Hi Brent,

This looks good! Thanks for taking care of this one.

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?
I wonder if lines 552-554 are a remnant of the previous
implementation and could be removed now...


 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?


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.


That's all for me :-)

best regards,

-- daniel


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

Re: RFR 8153123 : Streamline StackWalker code

[Brent Christian]

On 04/05/2016 10:36 PM, Mandy Chung wrote:


Looks good.  Nit: can you add a space after “synchronized” in 
StackFrameInfo.java line 109:
  109 synchronized(this) {


Yep - changed locally.

Thanks,
-Brent

Re: RFR 8153123 : Streamline StackWalker code

[Coleen Phillimore]

On 4/5/16 7:48 PM, Brent Christian wrote:
Thanks, Coleen.  Coordinating method/function names on "to stack trace 
element" is a fine thing.  I've done so in the updated webrev, and 
also implemented Claes's suggestion.


http://cr.openjdk.java.net/~bchristi/8153123/webrev.01/index.html


Thank you for making this change.  It looks good!  I've reviewed this.

Coleen



-Brent
On 04/05/2016 11:25 AM, Coleen Phillimore wrote:


A correction below.

On 4/5/16 1:29 PM, Coleen Phillimore wrote:


Also meant to include core-libs-dev in the email.
Thanks,
Coleen

On 4/5/16 1:27 PM, Coleen Phillimore wrote:


Hi, I've reviewed the hotspot changes and some of the jdk changes.
This looks really good.

One comment about the jvm function names:

I think FillInStackTraceElement is too close of a name to
Throwable::fill_in_stack_trace().

-JVM_ENTRY(void, JVM_SetMethodInfo(JNIEnv *env, jobject frame))
+JVM_ENTRY(void, JVM_FillInStackTraceElement(JNIEnv *env, jobject
frame, jobject stack))
   JVMWrapper("JVM_SetMethodInfo");
- Handle stackFrame(THREAD, JNIHandles::resolve(frame));
- java_lang_StackFrameInfo::fill_methodInfo(stackFrame, THREAD);
+ Handle stack_frame_info(THREAD, JNIHandles::resolve(frame));
+ Handle stack_trace_element(THREAD, JNIHandles::resolve(stack));
+ java_lang_StackFrameInfo::fill_methodInfo(stack_frame_info,
stack_trace_element, THREAD); JVM_END


And the function is called fill_methodInfo in the javaClasses 
function.


I think the JVM and the java_lang_StackFrameInfo function names
should be closer.

I wonder if the name JVM_ToStackFrameElement() and
java_lang_StackFrameInfo::to_stack_frame_element() would be better
and then it'd match the Java name.



I meant JVM_ToStackTraceElement() and
java_lang_StackFrameInfo::to_stack_trace_element(), since it's producing
a StackTraceElement.

thanks,
Coleen

Thanks!
Coleen

On 4/4/16 9:29 PM, Mandy Chung wrote:

On Apr 4, 2016, at 4:45 PM, 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


This looks good to me.

One thing to mention is that this patch is a follow-up work from the
investigation on what it takes to enable Throwable to use
StackWalker (JDK-8141239). The current built-in VM backtrace is very
compact and performant.  We have identified and prototypes the
performance improvements if Throwable backtrace is generated using
stack walker.  There are some performance gaps that we agree to
defer JDK-8141239 to a future release and improve the footprint
performance and GC throughput concerns when MemberNames are stored
in the throwable backtrace.

Mandy

Re: RFR 8153123 : Streamline StackWalker code

[Mandy Chung]

> On Apr 5, 2016, at 4:48 PM, Brent Christian  
> wrote:
> 
> Thanks, Coleen.  Coordinating method/function names on "to stack trace 
> element" is a fine thing.  I've done so in the updated webrev, and also 
> implemented Claes's suggestion.
> 
> http://cr.openjdk.java.net/~bchristi/8153123/webrev.01/index.html
> 

Looks good.  Nit: can you add a space after “synchronized” in 
StackFrameInfo.java line 109:
 109 synchronized(this) {

Mandy

Re: RFR 8153123 : Streamline StackWalker code

[Brent Christian]

Thanks, Coleen.  Coordinating method/function names on "to stack trace 
element" is a fine thing.  I've done so in the updated webrev, and also 
implemented Claes's suggestion.


http://cr.openjdk.java.net/~bchristi/8153123/webrev.01/index.html

-Brent
On 04/05/2016 11:25 AM, Coleen Phillimore wrote:


A correction below.

On 4/5/16 1:29 PM, Coleen Phillimore wrote:


Also meant to include core-libs-dev in the email.
Thanks,
Coleen

On 4/5/16 1:27 PM, Coleen Phillimore wrote:


Hi, I've reviewed the hotspot changes and some of the jdk changes.
This looks really good.

One comment about the jvm function names:

I think FillInStackTraceElement is too close of a name to
Throwable::fill_in_stack_trace().

-JVM_ENTRY(void, JVM_SetMethodInfo(JNIEnv *env, jobject frame))
+JVM_ENTRY(void, JVM_FillInStackTraceElement(JNIEnv *env, jobject
frame, jobject stack))
   JVMWrapper("JVM_SetMethodInfo");
- Handle stackFrame(THREAD, JNIHandles::resolve(frame));
- java_lang_StackFrameInfo::fill_methodInfo(stackFrame, THREAD);
+ Handle stack_frame_info(THREAD, JNIHandles::resolve(frame));
+ Handle stack_trace_element(THREAD, JNIHandles::resolve(stack));
+ java_lang_StackFrameInfo::fill_methodInfo(stack_frame_info,
stack_trace_element, THREAD); JVM_END


And the function is called fill_methodInfo in the javaClasses function.

I think the JVM and the java_lang_StackFrameInfo function names
should be closer.

I wonder if the name JVM_ToStackFrameElement() and
java_lang_StackFrameInfo::to_stack_frame_element() would be better
and then it'd match the Java name.



I meant JVM_ToStackTraceElement() and
java_lang_StackFrameInfo::to_stack_trace_element(), since it's producing
a StackTraceElement.

thanks,
Coleen

Thanks!
Coleen

On 4/4/16 9:29 PM, Mandy Chung wrote:

On Apr 4, 2016, at 4:45 PM, 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


This looks good to me.

One thing to mention is that this patch is a follow-up work from the
investigation on what it takes to enable Throwable to use
StackWalker (JDK-8141239). The current built-in VM backtrace is very
compact and performant.  We have identified and prototypes the
performance improvements if Throwable backtrace is generated using
stack walker.  There are some performance gaps that we agree to
defer JDK-8141239 to a future release and improve the footprint
performance and GC throughput concerns when MemberNames are stored
in the throwable backtrace.

Mandy

Re: RFR 8153123 : Streamline StackWalker code

[Coleen Phillimore]

A correction below.

On 4/5/16 1:29 PM, Coleen Phillimore wrote:


Also meant to include core-libs-dev in the email.
Thanks,
Coleen

On 4/5/16 1:27 PM, Coleen Phillimore wrote:


Hi, I've reviewed the hotspot changes and some of the jdk changes. 
This looks really good.


One comment about the jvm function names:

I think FillInStackTraceElement is too close of a name to 
Throwable::fill_in_stack_trace().


-JVM_ENTRY(void, JVM_SetMethodInfo(JNIEnv *env, jobject frame))
+JVM_ENTRY(void, JVM_FillInStackTraceElement(JNIEnv *env, jobject 
frame, jobject stack))

   JVMWrapper("JVM_SetMethodInfo");
- Handle stackFrame(THREAD, JNIHandles::resolve(frame));
- java_lang_StackFrameInfo::fill_methodInfo(stackFrame, THREAD);
+ Handle stack_frame_info(THREAD, JNIHandles::resolve(frame));
+ Handle stack_trace_element(THREAD, JNIHandles::resolve(stack));
+ java_lang_StackFrameInfo::fill_methodInfo(stack_frame_info, 
stack_trace_element, THREAD); JVM_END



And the function is called fill_methodInfo in the javaClasses function.

I think the JVM and the java_lang_StackFrameInfo function names 
should be closer.


I wonder if the name JVM_ToStackFrameElement() and 
java_lang_StackFrameInfo::to_stack_frame_element() would be better 
and then it'd match the Java name.




I meant JVM_ToStackTraceElement() and 
java_lang_StackFrameInfo::to_stack_trace_element(), since it's producing 
a StackTraceElement.


thanks,
Coleen

Thanks!
Coleen

On 4/4/16 9:29 PM, Mandy Chung wrote:
On Apr 4, 2016, at 4:45 PM, 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


This looks good to me.

One thing to mention is that this patch is a follow-up work from the 
investigation on what it takes to enable Throwable to use 
StackWalker (JDK-8141239). The current built-in VM backtrace is very 
compact and performant.  We have identified and prototypes the 
performance improvements if Throwable backtrace is generated using 
stack walker.  There are some performance gaps that we agree to 
defer JDK-8141239 to a future release and improve the footprint 
performance and GC throughput concerns when MemberNames are stored 
in the throwable backtrace.


Mandy

Re: RFR 8153123 : Streamline StackWalker code

[Claes Redestad]

Hi,

On 04/05/2016 01:45 AM, 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/


this looks really good to me.

It seems the new implementation of StackFrameInfo::toStackTraceElement 
reads the volatile field ste twice on the fast path, though, so perhaps 
consider something like this:


+ StackTraceElement s = ste; + if (s == null) {
+ synchronized(this) { + s = ste; + if (s == null) {
+ s = new StackTraceElement();
+ toStackTraceElement0(s);
+ ste = s;
+ }
+ }
+ }
+ return s;


Thanks!

/Claes


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

Re: RFR 8153123 : Streamline StackWalker code

[Coleen Phillimore]

Also meant to include core-libs-dev in the email.
Thanks,
Coleen

On 4/5/16 1:27 PM, Coleen Phillimore wrote:


Hi, I've reviewed the hotspot changes and some of the jdk changes. 
This looks really good.


One comment about the jvm function names:

I think FillInStackTraceElement is too close of a name to 
Throwable::fill_in_stack_trace().


-JVM_ENTRY(void, JVM_SetMethodInfo(JNIEnv *env, jobject frame))
+JVM_ENTRY(void, JVM_FillInStackTraceElement(JNIEnv *env, jobject 
frame, jobject stack))

   JVMWrapper("JVM_SetMethodInfo");
- Handle stackFrame(THREAD, JNIHandles::resolve(frame));
- java_lang_StackFrameInfo::fill_methodInfo(stackFrame, THREAD);
+ Handle stack_frame_info(THREAD, JNIHandles::resolve(frame));
+ Handle stack_trace_element(THREAD, JNIHandles::resolve(stack));
+ java_lang_StackFrameInfo::fill_methodInfo(stack_frame_info, 
stack_trace_element, THREAD); JVM_END



And the function is called fill_methodInfo in the javaClasses function.

I think the JVM and the java_lang_StackFrameInfo function names should 
be closer.


I wonder if the name JVM_ToStackFrameElement() and 
java_lang_StackFrameInfo::to_stack_frame_element() would be better and 
then it'd match the Java name.


Thanks!
Coleen

On 4/4/16 9:29 PM, Mandy Chung wrote:
On Apr 4, 2016, at 4:45 PM, 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


This looks good to me.

One thing to mention is that this patch is a follow-up work from the 
investigation on what it takes to enable Throwable to use StackWalker 
(JDK-8141239). The current built-in VM backtrace is very compact and 
performant.  We have identified and prototypes the performance 
improvements if Throwable backtrace is generated using stack walker.  
There are some performance gaps that we agree to defer JDK-8141239 to 
a future release and improve the footprint performance and GC 
throughput concerns when MemberNames are stored in the throwable 
backtrace.


Mandy

Re: RFR 8153123 : Streamline StackWalker code

[Mandy Chung]

> On Apr 4, 2016, at 4:45 PM, 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
> 

This looks good to me.

One thing to mention is that this patch is a follow-up work from the 
investigation on what it takes to enable Throwable to use StackWalker 
(JDK-8141239). The current built-in VM backtrace is very compact and 
performant.  We have identified and prototypes the performance improvements if 
Throwable backtrace is generated using stack walker.  There are some 
performance gaps that we agree to defer JDK-8141239 to a future release and 
improve the footprint performance and GC throughput concerns when MemberNames 
are stored in the throwable backtrace.

Mandy

RFR 8153123 : Streamline StackWalker code

[Brent Christian]

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