Re: RFR: JDK-8152690: main thread does not have native thread name

[David Holmes]

On 27/04/2016 12:38 AM, Yasumasa Suenaga wrote:

Hi Kumar, David,


simply to set a name which seems to be useful only for folks trying to
debug
the VM ???  besides ps and few other OS related utils it
has no visibility of value to the JRE or the JDK APIs.


I want to check CPU time through ps command on Linux.
Usually, I get "ps -eLf" and nid in threaddump to check
which thread spends much CPU time.

If I can get thread name from output of ps command,
I can check it rapidly.


If it is the only unnamed thread in the java process then you can infer 
that it is the "main" thread in one of its two forms. The tid may also 
give you some indication it is the first thread created by the launcher.



So I want to set "main" name at least.


Sorry, but as previously discussed leaving it at "main" after it becomes 
the DestroyJavaVM thread will be a source of confusion (and bug reports).


David
-



Thanks,

Yasumasa


On 2016/04/26 23:21, Kumar Srinivasan wrote:


On 4/26/2016 6:08 AM, David Holmes wrote:

On 26/04/2016 10:54 PM, Yasumasa Suenaga wrote:

Hi David,

For this issue, I think we can approach as following:


   1. Export new JVM function to set native thread name
It can avoid JNI call and can handle char* value.
However this plan has been rejected.

   2. Call uncaught exception handler from Launcher
We have to clear (process) any pending exception before
DetachCurrentThread() call. (not documented?)
--

so note that we are potentially calling DetachCurrentThread with an
exception pending - which is prohibited by JNI**



**JNI spec needs to be modified here.

--
So we can process pending exception through uncaught
exception handler.
However, this plan has been rejected.

   3. Do not set DestroyJavaVM name when exception is occurred
It disrupts consistency for native thread name.

   4. Attach to JVM to set native thread name for DestroyJavaVM
It does not look good.


If all of them are not accepted, I guess that it is difficult.
Do you have any other idea?


Sorry I don't. The basic idea of having the launcher set the native
thread name is fine, but the mechanism to do that has been
problematic. The "real" solution (ie one that other applications
hosting the jvm would need to use) is for the launcher to duplicate
the per-platform logic for os::set_native_thread_name - but that was
undesirable. Instead we have tried to avoid that by finding a way to
use whatever is already in the JVM - but adding a new JVM interface
to expose it directly is not ideal; and hooking into the
java.lang.Thread code to avoid that has run into these other
problems. I really don't want to take the logic for uncaught
exception handling that is in Thread::exit and duplicate it in the
launcher.

The effort and disruption here really does not justify the "it would
be nice to set the native thread name for the main thread" objective.

I never expected this to be as problematic as it has turned out.


I agree with Holmes-san, I think this needlessly complicates
the launcher, more than I would like!, ie  simply to set a name
which seems to be useful only for folks trying to debug
the VM ???  besides ps and few other OS related utils it
has no visibility of value to the JRE or the JDK APIs.

Thanks

Kumar



Sorry.

David
-




Thanks,

Yasumasa


On 2016/04/26 18:35, David Holmes wrote:

On 26/04/2016 7:22 PM, Yasumasa Suenaga wrote:

Hi David,


I thought about being able to save/restore the original pending
exception but could not see a simple way to restore it - ie just by
poking it back into the thread's pending exception field. The
problem
with using env->Throw is that it acts like the initial throwing
of the
exception and will have a number of side-effects that then get
doubled
up:
- logging statements (UL and Event logging)
- OOM counting


Thanks, I understood.


so note that we are potentially calling DetachCurrentThread
with an
exception pending - which is prohibited by JNI**, but which we
actually rely on for desired operation as DetachCurrentThread
calls
thread->exit() which performs uncaught exception handling (ie it
prints the exception message and stacktrace) and then clears the
exception! (Hence DestroyJavaVM is not called with any pending
exceptions.)


I think we can call uncaught exception handler before calling
DestroyJavaVM().
I added it in new webrev for jdk:

http://cr.openjdk.java.net/~ysuenaga/JDK-8152690/webrev.08/hotspot/
http://cr.openjdk.java.net/~ysuenaga/JDK-8152690/webrev.08/jdk/

DispatchUncaughtException() in java.c emulates uncaught exception
handler
call in JavaThread::exit().
I think this patch can provide the solution for our issue.

Could you check it?


Sorry - this is getting far too disruptive. I do not support changing
the way the main thread behaves upon completion, just because we want
to set a native thread name.

David
-



Thanks,

Yasumasa


On 2016/04/26 14:16, David Holmes wrote:

On 

Re: RFR: JDK-8152690: main thread does not have native thread name

[Yasumasa Suenaga]

Hi Kumar, David,


simply to set a name which seems to be useful only for folks trying to debug
the VM ???  besides ps and few other OS related utils it
has no visibility of value to the JRE or the JDK APIs.


I want to check CPU time through ps command on Linux.
Usually, I get "ps -eLf" and nid in threaddump to check
which thread spends much CPU time.

If I can get thread name from output of ps command,
I can check it rapidly.

So I want to set "main" name at least.


Thanks,

Yasumasa


On 2016/04/26 23:21, Kumar Srinivasan wrote:


On 4/26/2016 6:08 AM, David Holmes wrote:

On 26/04/2016 10:54 PM, Yasumasa Suenaga wrote:

Hi David,

For this issue, I think we can approach as following:


   1. Export new JVM function to set native thread name
It can avoid JNI call and can handle char* value.
However this plan has been rejected.

   2. Call uncaught exception handler from Launcher
We have to clear (process) any pending exception before
DetachCurrentThread() call. (not documented?)
--

so note that we are potentially calling DetachCurrentThread with an
exception pending - which is prohibited by JNI**



**JNI spec needs to be modified here.

--
So we can process pending exception through uncaught
exception handler.
However, this plan has been rejected.

   3. Do not set DestroyJavaVM name when exception is occurred
It disrupts consistency for native thread name.

   4. Attach to JVM to set native thread name for DestroyJavaVM
It does not look good.


If all of them are not accepted, I guess that it is difficult.
Do you have any other idea?


Sorry I don't. The basic idea of having the launcher set the native thread name is fine, 
but the mechanism to do that has been problematic. The "real" solution (ie one 
that other applications hosting the jvm would need to use) is for the launcher to 
duplicate the per-platform logic for os::set_native_thread_name - but that was 
undesirable. Instead we have tried to avoid that by finding a way to use whatever is 
already in the JVM - but adding a new JVM interface to expose it directly is not ideal; 
and hooking into the java.lang.Thread code to avoid that has run into these other 
problems. I really don't want to take the logic for uncaught exception handling that is 
in Thread::exit and duplicate it in the launcher.

The effort and disruption here really does not justify the "it would be nice to set 
the native thread name for the main thread" objective.

I never expected this to be as problematic as it has turned out.


I agree with Holmes-san, I think this needlessly complicates
the launcher, more than I would like!, ie  simply to set a name
which seems to be useful only for folks trying to debug
the VM ???  besides ps and few other OS related utils it
has no visibility of value to the JRE or the JDK APIs.

Thanks

Kumar



Sorry.

David
-




Thanks,

Yasumasa


On 2016/04/26 18:35, David Holmes wrote:

On 26/04/2016 7:22 PM, Yasumasa Suenaga wrote:

Hi David,


I thought about being able to save/restore the original pending
exception but could not see a simple way to restore it - ie just by
poking it back into the thread's pending exception field. The problem
with using env->Throw is that it acts like the initial throwing of the
exception and will have a number of side-effects that then get doubled
up:
- logging statements (UL and Event logging)
- OOM counting


Thanks, I understood.


so note that we are potentially calling DetachCurrentThread with an
exception pending - which is prohibited by JNI**, but which we
actually rely on for desired operation as DetachCurrentThread calls
thread->exit() which performs uncaught exception handling (ie it
prints the exception message and stacktrace) and then clears the
exception! (Hence DestroyJavaVM is not called with any pending
exceptions.)


I think we can call uncaught exception handler before calling
DestroyJavaVM().
I added it in new webrev for jdk:

http://cr.openjdk.java.net/~ysuenaga/JDK-8152690/webrev.08/hotspot/
http://cr.openjdk.java.net/~ysuenaga/JDK-8152690/webrev.08/jdk/

DispatchUncaughtException() in java.c emulates uncaught exception
handler
call in JavaThread::exit().
I think this patch can provide the solution for our issue.

Could you check it?


Sorry - this is getting far too disruptive. I do not support changing
the way the main thread behaves upon completion, just because we want
to set a native thread name.

David
-



Thanks,

Yasumasa


On 2016/04/26 14:16, David Holmes wrote:

On 26/04/2016 1:11 PM, Yasumasa Suenaga wrote:

Hi David, Kumar,

I think that we should evacuate original exception before
DestroyJavaVM
thread name is set.

http://cr.openjdk.java.net/~ysuenaga/JDK-8152690/webrev.07/hotspot/
http://cr.openjdk.java.net/~ysuenaga/JDK-8152690/webrev.07/jdk/

I added it to LEAVE macro in new webrev.


BTW: in the LEAVE macro, stylistically all the code should be in the
do { } while(false); loop. I 

Re: RFR: JDK-8152690: main thread does not have native thread name

[Kumar Srinivasan]

On 4/26/2016 6:08 AM, David Holmes wrote:

On 26/04/2016 10:54 PM, Yasumasa Suenaga wrote:

Hi David,

For this issue, I think we can approach as following:


   1. Export new JVM function to set native thread name
It can avoid JNI call and can handle char* value.
However this plan has been rejected.

   2. Call uncaught exception handler from Launcher
We have to clear (process) any pending exception before
DetachCurrentThread() call. (not documented?)
--

so note that we are potentially calling DetachCurrentThread with an
exception pending - which is prohibited by JNI**



**JNI spec needs to be modified here.

--
So we can process pending exception through uncaught
exception handler.
However, this plan has been rejected.

   3. Do not set DestroyJavaVM name when exception is occurred
It disrupts consistency for native thread name.

   4. Attach to JVM to set native thread name for DestroyJavaVM
It does not look good.


If all of them are not accepted, I guess that it is difficult.
Do you have any other idea?


Sorry I don't. The basic idea of having the launcher set the native 
thread name is fine, but the mechanism to do that has been 
problematic. The "real" solution (ie one that other applications 
hosting the jvm would need to use) is for the launcher to duplicate 
the per-platform logic for os::set_native_thread_name - but that was 
undesirable. Instead we have tried to avoid that by finding a way to 
use whatever is already in the JVM - but adding a new JVM interface to 
expose it directly is not ideal; and hooking into the java.lang.Thread 
code to avoid that has run into these other problems. I really don't 
want to take the logic for uncaught exception handling that is in 
Thread::exit and duplicate it in the launcher.


The effort and disruption here really does not justify the "it would 
be nice to set the native thread name for the main thread" objective.


I never expected this to be as problematic as it has turned out.


I agree with Holmes-san, I think this needlessly complicates
the launcher, more than I would like!, ie  simply to set a name
which seems to be useful only for folks trying to debug
the VM ???  besides ps and few other OS related utils it
has no visibility of value to the JRE or the JDK APIs.

Thanks

Kumar



Sorry.

David
-




Thanks,

Yasumasa


On 2016/04/26 18:35, David Holmes wrote:

On 26/04/2016 7:22 PM, Yasumasa Suenaga wrote:

Hi David,


I thought about being able to save/restore the original pending
exception but could not see a simple way to restore it - ie just by
poking it back into the thread's pending exception field. The problem
with using env->Throw is that it acts like the initial throwing of 
the
exception and will have a number of side-effects that then get 
doubled

up:
- logging statements (UL and Event logging)
- OOM counting


Thanks, I understood.


so note that we are potentially calling DetachCurrentThread with an
exception pending - which is prohibited by JNI**, but which we
actually rely on for desired operation as DetachCurrentThread calls
thread->exit() which performs uncaught exception handling (ie it
prints the exception message and stacktrace) and then clears the
exception! (Hence DestroyJavaVM is not called with any pending
exceptions.)


I think we can call uncaught exception handler before calling
DestroyJavaVM().
I added it in new webrev for jdk:

http://cr.openjdk.java.net/~ysuenaga/JDK-8152690/webrev.08/hotspot/
http://cr.openjdk.java.net/~ysuenaga/JDK-8152690/webrev.08/jdk/

DispatchUncaughtException() in java.c emulates uncaught exception
handler
call in JavaThread::exit().
I think this patch can provide the solution for our issue.

Could you check it?


Sorry - this is getting far too disruptive. I do not support changing
the way the main thread behaves upon completion, just because we want
to set a native thread name.

David
-



Thanks,

Yasumasa


On 2016/04/26 14:16, David Holmes wrote:

On 26/04/2016 1:11 PM, Yasumasa Suenaga wrote:

Hi David, Kumar,

I think that we should evacuate original exception before
DestroyJavaVM
thread name is set.

http://cr.openjdk.java.net/~ysuenaga/JDK-8152690/webrev.07/hotspot/
http://cr.openjdk.java.net/~ysuenaga/JDK-8152690/webrev.07/jdk/

I added it to LEAVE macro in new webrev.


BTW: in the LEAVE macro, stylistically all the code should be in the
do { } while(false); loop. I overlooked that initially.


I tested it with following code. It works fine.

-
public void main(String[] args){
   throw new RuntimeException("test");
}
-

What do you think about it?


I thought about being able to save/restore the original pending
exception but could not see a simple way to restore it - ie just by
poking it back into the thread's pending exception field. The problem
with using env->Throw is that it acts like the initial throwing of 
the
exception and will have a number of side-effects 

Re: RFR: JDK-8152690: main thread does not have native thread name

[David Holmes]

On 26/04/2016 10:54 PM, Yasumasa Suenaga wrote:

Hi David,

For this issue, I think we can approach as following:


   1. Export new JVM function to set native thread name
It can avoid JNI call and can handle char* value.
However this plan has been rejected.

   2. Call uncaught exception handler from Launcher
We have to clear (process) any pending exception before
DetachCurrentThread() call. (not documented?)
--

so note that we are potentially calling DetachCurrentThread with an
exception pending - which is prohibited by JNI**



**JNI spec needs to be modified here.

--
So we can process pending exception through uncaught
exception handler.
However, this plan has been rejected.

   3. Do not set DestroyJavaVM name when exception is occurred
It disrupts consistency for native thread name.

   4. Attach to JVM to set native thread name for DestroyJavaVM
It does not look good.


If all of them are not accepted, I guess that it is difficult.
Do you have any other idea?


Sorry I don't. The basic idea of having the launcher set the native 
thread name is fine, but the mechanism to do that has been problematic. 
The "real" solution (ie one that other applications hosting the jvm 
would need to use) is for the launcher to duplicate the per-platform 
logic for os::set_native_thread_name - but that was undesirable. Instead 
we have tried to avoid that by finding a way to use whatever is already 
in the JVM - but adding a new JVM interface to expose it directly is not 
ideal; and hooking into the java.lang.Thread code to avoid that has run 
into these other problems. I really don't want to take the logic for 
uncaught exception handling that is in Thread::exit and duplicate it in 
the launcher.


The effort and disruption here really does not justify the "it would be 
nice to set the native thread name for the main thread" objective.


I never expected this to be as problematic as it has turned out.

Sorry.

David
-




Thanks,

Yasumasa


On 2016/04/26 18:35, David Holmes wrote:

On 26/04/2016 7:22 PM, Yasumasa Suenaga wrote:

Hi David,


I thought about being able to save/restore the original pending
exception but could not see a simple way to restore it - ie just by
poking it back into the thread's pending exception field. The problem
with using env->Throw is that it acts like the initial throwing of the
exception and will have a number of side-effects that then get doubled
up:
- logging statements (UL and Event logging)
- OOM counting


Thanks, I understood.


so note that we are potentially calling DetachCurrentThread with an
exception pending - which is prohibited by JNI**, but which we
actually rely on for desired operation as DetachCurrentThread calls
thread->exit() which performs uncaught exception handling (ie it
prints the exception message and stacktrace) and then clears the
exception! (Hence DestroyJavaVM is not called with any pending
exceptions.)


I think we can call uncaught exception handler before calling
DestroyJavaVM().
I added it in new webrev for jdk:

   http://cr.openjdk.java.net/~ysuenaga/JDK-8152690/webrev.08/hotspot/
   http://cr.openjdk.java.net/~ysuenaga/JDK-8152690/webrev.08/jdk/

DispatchUncaughtException() in java.c emulates uncaught exception
handler
call in JavaThread::exit().
I think this patch can provide the solution for our issue.

Could you check it?


Sorry - this is getting far too disruptive. I do not support changing
the way the main thread behaves upon completion, just because we want
to set a native thread name.

David
-



Thanks,

Yasumasa


On 2016/04/26 14:16, David Holmes wrote:

On 26/04/2016 1:11 PM, Yasumasa Suenaga wrote:

Hi David, Kumar,

I think that we should evacuate original exception before
DestroyJavaVM
thread name is set.

   http://cr.openjdk.java.net/~ysuenaga/JDK-8152690/webrev.07/hotspot/
   http://cr.openjdk.java.net/~ysuenaga/JDK-8152690/webrev.07/jdk/

I added it to LEAVE macro in new webrev.


BTW: in the LEAVE macro, stylistically all the code should be in the
do { } while(false); loop. I overlooked that initially.


I tested it with following code. It works fine.

-
public void main(String[] args){
   throw new RuntimeException("test");
}
-

What do you think about it?


I thought about being able to save/restore the original pending
exception but could not see a simple way to restore it - ie just by
poking it back into the thread's pending exception field. The problem
with using env->Throw is that it acts like the initial throwing of the
exception and will have a number of side-effects that then get doubled
up:
- logging statements (UL and Event logging)
- OOM counting

I'm not sure whether that is acceptable or not

That aside you should check if orig_throwable is non-null before
clearing to avoid an unnecessary JNI call.

Also "Resume original exception" -> "Restore any original exception"

Thanks,
David
-



Thanks,

Re: RFR: JDK-8152690: main thread does not have native thread name

[Yasumasa Suenaga]

Hi David,

For this issue, I think we can approach as following:


  1. Export new JVM function to set native thread name
   It can avoid JNI call and can handle char* value.
   However this plan has been rejected.

  2. Call uncaught exception handler from Launcher
   We have to clear (process) any pending exception before
   DetachCurrentThread() call. (not documented?)
--

so note that we are potentially calling DetachCurrentThread with an
exception pending - which is prohibited by JNI**



**JNI spec needs to be modified here.

--
   So we can process pending exception through uncaught
   exception handler.
   However, this plan has been rejected.

  3. Do not set DestroyJavaVM name when exception is occurred
   It disrupts consistency for native thread name.

  4. Attach to JVM to set native thread name for DestroyJavaVM
   It does not look good.


If all of them are not accepted, I guess that it is difficult.
Do you have any other idea?


Thanks,

Yasumasa


On 2016/04/26 18:35, David Holmes wrote:

On 26/04/2016 7:22 PM, Yasumasa Suenaga wrote:

Hi David,


I thought about being able to save/restore the original pending
exception but could not see a simple way to restore it - ie just by
poking it back into the thread's pending exception field. The problem
with using env->Throw is that it acts like the initial throwing of the
exception and will have a number of side-effects that then get doubled
up:
- logging statements (UL and Event logging)
- OOM counting


Thanks, I understood.


so note that we are potentially calling DetachCurrentThread with an
exception pending - which is prohibited by JNI**, but which we
actually rely on for desired operation as DetachCurrentThread calls
thread->exit() which performs uncaught exception handling (ie it
prints the exception message and stacktrace) and then clears the
exception! (Hence DestroyJavaVM is not called with any pending
exceptions.)


I think we can call uncaught exception handler before calling
DestroyJavaVM().
I added it in new webrev for jdk:

   http://cr.openjdk.java.net/~ysuenaga/JDK-8152690/webrev.08/hotspot/
   http://cr.openjdk.java.net/~ysuenaga/JDK-8152690/webrev.08/jdk/

DispatchUncaughtException() in java.c emulates uncaught exception handler
call in JavaThread::exit().
I think this patch can provide the solution for our issue.

Could you check it?


Sorry - this is getting far too disruptive. I do not support changing the way 
the main thread behaves upon completion, just because we want to set a native 
thread name.

David
-



Thanks,

Yasumasa


On 2016/04/26 14:16, David Holmes wrote:

On 26/04/2016 1:11 PM, Yasumasa Suenaga wrote:

Hi David, Kumar,

I think that we should evacuate original exception before DestroyJavaVM
thread name is set.

   http://cr.openjdk.java.net/~ysuenaga/JDK-8152690/webrev.07/hotspot/
   http://cr.openjdk.java.net/~ysuenaga/JDK-8152690/webrev.07/jdk/

I added it to LEAVE macro in new webrev.


BTW: in the LEAVE macro, stylistically all the code should be in the
do { } while(false); loop. I overlooked that initially.


I tested it with following code. It works fine.

-
public void main(String[] args){
   throw new RuntimeException("test");
}
-

What do you think about it?


I thought about being able to save/restore the original pending
exception but could not see a simple way to restore it - ie just by
poking it back into the thread's pending exception field. The problem
with using env->Throw is that it acts like the initial throwing of the
exception and will have a number of side-effects that then get doubled
up:
- logging statements (UL and Event logging)
- OOM counting

I'm not sure whether that is acceptable or not

That aside you should check if orig_throwable is non-null before
clearing to avoid an unnecessary JNI call.

Also "Resume original exception" -> "Restore any original exception"

Thanks,
David
-



Thanks,

Yasumasa


On 2016/04/26 11:16, David Holmes wrote:

Hi Yasumasa, Kumar,

Turns out this change breaks the behaviour of the launcher in the case
that main() completes by throwing an exception.

What we have in the launcher is:

(*env)->CallStaticVoidMethod(env, mainClass, mainID, mainArgs);
ret = (*env)->ExceptionOccurred(env) == NULL ? 0 : 1;
LEAVE();

where LEAVE would have expanded to:

if ((*vm)->DetachCurrentThread(vm) != JNI_OK) { \
JLI_ReportErrorMessage(JVM_ERROR2); \
ret = 1; \
} \
if (JNI_TRUE) { \
(*vm)->DestroyJavaVM(vm); \
return ret; \
} \

so note that we are potentially calling DetachCurrentThread with an
exception pending - which is prohibited by JNI**, but which we
actually rely on for desired operation as DetachCurrentThread calls
thread->exit() which performs uncaught exception handling (ie it
prints the exception message and stacktrace) and then clears the
exception! (Hence DestroyJavaVM is not called 

Re: RFR: JDK-8152690: main thread does not have native thread name

[David Holmes]

On 26/04/2016 7:22 PM, Yasumasa Suenaga wrote:

Hi David,


I thought about being able to save/restore the original pending
exception but could not see a simple way to restore it - ie just by
poking it back into the thread's pending exception field. The problem
with using env->Throw is that it acts like the initial throwing of the
exception and will have a number of side-effects that then get doubled
up:
- logging statements (UL and Event logging)
- OOM counting


Thanks, I understood.


so note that we are potentially calling DetachCurrentThread with an
exception pending - which is prohibited by JNI**, but which we
actually rely on for desired operation as DetachCurrentThread calls
thread->exit() which performs uncaught exception handling (ie it
prints the exception message and stacktrace) and then clears the
exception! (Hence DestroyJavaVM is not called with any pending
exceptions.)


I think we can call uncaught exception handler before calling
DestroyJavaVM().
I added it in new webrev for jdk:

   http://cr.openjdk.java.net/~ysuenaga/JDK-8152690/webrev.08/hotspot/
   http://cr.openjdk.java.net/~ysuenaga/JDK-8152690/webrev.08/jdk/

DispatchUncaughtException() in java.c emulates uncaught exception handler
call in JavaThread::exit().
I think this patch can provide the solution for our issue.

Could you check it?


Sorry - this is getting far too disruptive. I do not support changing 
the way the main thread behaves upon completion, just because we want to 
set a native thread name.


David
-



Thanks,

Yasumasa


On 2016/04/26 14:16, David Holmes wrote:

On 26/04/2016 1:11 PM, Yasumasa Suenaga wrote:

Hi David, Kumar,

I think that we should evacuate original exception before DestroyJavaVM
thread name is set.

   http://cr.openjdk.java.net/~ysuenaga/JDK-8152690/webrev.07/hotspot/
   http://cr.openjdk.java.net/~ysuenaga/JDK-8152690/webrev.07/jdk/

I added it to LEAVE macro in new webrev.


BTW: in the LEAVE macro, stylistically all the code should be in the
do { } while(false); loop. I overlooked that initially.


I tested it with following code. It works fine.

-
public void main(String[] args){
   throw new RuntimeException("test");
}
-

What do you think about it?


I thought about being able to save/restore the original pending
exception but could not see a simple way to restore it - ie just by
poking it back into the thread's pending exception field. The problem
with using env->Throw is that it acts like the initial throwing of the
exception and will have a number of side-effects that then get doubled
up:
- logging statements (UL and Event logging)
- OOM counting

I'm not sure whether that is acceptable or not

That aside you should check if orig_throwable is non-null before
clearing to avoid an unnecessary JNI call.

Also "Resume original exception" -> "Restore any original exception"

Thanks,
David
-



Thanks,

Yasumasa


On 2016/04/26 11:16, David Holmes wrote:

Hi Yasumasa, Kumar,

Turns out this change breaks the behaviour of the launcher in the case
that main() completes by throwing an exception.

What we have in the launcher is:

(*env)->CallStaticVoidMethod(env, mainClass, mainID, mainArgs);
ret = (*env)->ExceptionOccurred(env) == NULL ? 0 : 1;
LEAVE();

where LEAVE would have expanded to:

if ((*vm)->DetachCurrentThread(vm) != JNI_OK) { \
JLI_ReportErrorMessage(JVM_ERROR2); \
ret = 1; \
} \
if (JNI_TRUE) { \
(*vm)->DestroyJavaVM(vm); \
return ret; \
} \

so note that we are potentially calling DetachCurrentThread with an
exception pending - which is prohibited by JNI**, but which we
actually rely on for desired operation as DetachCurrentThread calls
thread->exit() which performs uncaught exception handling (ie it
prints the exception message and stacktrace) and then clears the
exception! (Hence DestroyJavaVM is not called with any pending
exceptions.)

**JNI spec needs to be modified here.

With the current change we have now inserted the following at the
start of LEAVE:

SetNativeThreadName(env, "DestroyJavaVM");  \
if ((*env)->ExceptionOccurred(env)) { \
(*env)->ExceptionClear(env);   \
} \

this has two unintended consequences:

1. SetNativeThreadName itself calls a number of JNI methods, with the
exception pending - which is not permitted. So straight away where we
have:

   NULL_CHECK(cls = FindBootStrapClass(env, "java/lang/Thread"));

FindBootStrapClass calls JVM_FindClassFromBootLoader, which make calls
using the VM's CHECK_NULL macro - which checks for a pending exception
(which it finds) and returns NULL. So the jli NULL_CHECK macro then
reports a JNI error:

Error: A JNI error has occurred, please check your installation and
try again


2. There is no longer an exception from main() for DetachCurrentThread
to report, so we exit with a return code of 1 as required, but don't
report the exception 

Re: RFR: JDK-8152690: main thread does not have native thread name

[Yasumasa Suenaga]

Hi David,


I thought about being able to save/restore the original pending exception but 
could not see a simple way to restore it - ie just by poking it back into the 
thread's pending exception field. The problem with using env->Throw is that it 
acts like the initial throwing of the exception and will have a number of 
side-effects that then get doubled up:
- logging statements (UL and Event logging)
- OOM counting


Thanks, I understood.


so note that we are potentially calling DetachCurrentThread with an
exception pending - which is prohibited by JNI**, but which we
actually rely on for desired operation as DetachCurrentThread calls
thread->exit() which performs uncaught exception handling (ie it
prints the exception message and stacktrace) and then clears the
exception! (Hence DestroyJavaVM is not called with any pending
exceptions.)


I think we can call uncaught exception handler before calling DestroyJavaVM().
I added it in new webrev for jdk:

  http://cr.openjdk.java.net/~ysuenaga/JDK-8152690/webrev.08/hotspot/
  http://cr.openjdk.java.net/~ysuenaga/JDK-8152690/webrev.08/jdk/

DispatchUncaughtException() in java.c emulates uncaught exception handler
call in JavaThread::exit().
I think this patch can provide the solution for our issue.

Could you check it?


Thanks,

Yasumasa


On 2016/04/26 14:16, David Holmes wrote:

On 26/04/2016 1:11 PM, Yasumasa Suenaga wrote:

Hi David, Kumar,

I think that we should evacuate original exception before DestroyJavaVM
thread name is set.

   http://cr.openjdk.java.net/~ysuenaga/JDK-8152690/webrev.07/hotspot/
   http://cr.openjdk.java.net/~ysuenaga/JDK-8152690/webrev.07/jdk/

I added it to LEAVE macro in new webrev.


BTW: in the LEAVE macro, stylistically all the code should be in the do { } 
while(false); loop. I overlooked that initially.


I tested it with following code. It works fine.

-
public void main(String[] args){
   throw new RuntimeException("test");
}
-

What do you think about it?


I thought about being able to save/restore the original pending exception but 
could not see a simple way to restore it - ie just by poking it back into the 
thread's pending exception field. The problem with using env->Throw is that it 
acts like the initial throwing of the exception and will have a number of 
side-effects that then get doubled up:
- logging statements (UL and Event logging)
- OOM counting

I'm not sure whether that is acceptable or not

That aside you should check if orig_throwable is non-null before clearing to 
avoid an unnecessary JNI call.

Also "Resume original exception" -> "Restore any original exception"

Thanks,
David
-



Thanks,

Yasumasa


On 2016/04/26 11:16, David Holmes wrote:

Hi Yasumasa, Kumar,

Turns out this change breaks the behaviour of the launcher in the case
that main() completes by throwing an exception.

What we have in the launcher is:

(*env)->CallStaticVoidMethod(env, mainClass, mainID, mainArgs);
ret = (*env)->ExceptionOccurred(env) == NULL ? 0 : 1;
LEAVE();

where LEAVE would have expanded to:

if ((*vm)->DetachCurrentThread(vm) != JNI_OK) { \
JLI_ReportErrorMessage(JVM_ERROR2); \
ret = 1; \
} \
if (JNI_TRUE) { \
(*vm)->DestroyJavaVM(vm); \
return ret; \
} \

so note that we are potentially calling DetachCurrentThread with an
exception pending - which is prohibited by JNI**, but which we
actually rely on for desired operation as DetachCurrentThread calls
thread->exit() which performs uncaught exception handling (ie it
prints the exception message and stacktrace) and then clears the
exception! (Hence DestroyJavaVM is not called with any pending
exceptions.)

**JNI spec needs to be modified here.

With the current change we have now inserted the following at the
start of LEAVE:

SetNativeThreadName(env, "DestroyJavaVM");  \
if ((*env)->ExceptionOccurred(env)) { \
(*env)->ExceptionClear(env);   \
} \

this has two unintended consequences:

1. SetNativeThreadName itself calls a number of JNI methods, with the
exception pending - which is not permitted. So straight away where we
have:

   NULL_CHECK(cls = FindBootStrapClass(env, "java/lang/Thread"));

FindBootStrapClass calls JVM_FindClassFromBootLoader, which make calls
using the VM's CHECK_NULL macro - which checks for a pending exception
(which it finds) and returns NULL. So the jli NULL_CHECK macro then
reports a JNI error:

Error: A JNI error has occurred, please check your installation and
try again


2. There is no longer an exception from main() for DetachCurrentThread
to report, so we exit with a return code of 1 as required, but don't
report the exception message/stacktrace.

I don't see a reasonable solution for this other than abandoning the
attempt to change the name from "main" to "DestroyJavaVM" - which if
we can't do, I question the utility of setting the name in the first

Re: RFR: JDK-8152690: main thread does not have native thread name

[David Holmes]

On 26/04/2016 1:11 PM, Yasumasa Suenaga wrote:

Hi David, Kumar,

I think that we should evacuate original exception before DestroyJavaVM
thread name is set.

   http://cr.openjdk.java.net/~ysuenaga/JDK-8152690/webrev.07/hotspot/
   http://cr.openjdk.java.net/~ysuenaga/JDK-8152690/webrev.07/jdk/

I added it to LEAVE macro in new webrev.


BTW: in the LEAVE macro, stylistically all the code should be in the do 
{ } while(false); loop. I overlooked that initially.



I tested it with following code. It works fine.

-
public void main(String[] args){
   throw new RuntimeException("test");
}
-

What do you think about it?


I thought about being able to save/restore the original pending 
exception but could not see a simple way to restore it - ie just by 
poking it back into the thread's pending exception field. The problem 
with using env->Throw is that it acts like the initial throwing of the 
exception and will have a number of side-effects that then get doubled up:

- logging statements (UL and Event logging)
- OOM counting

I'm not sure whether that is acceptable or not

That aside you should check if orig_throwable is non-null before 
clearing to avoid an unnecessary JNI call.


Also "Resume original exception" -> "Restore any original exception"

Thanks,
David
-



Thanks,

Yasumasa


On 2016/04/26 11:16, David Holmes wrote:

Hi Yasumasa, Kumar,

Turns out this change breaks the behaviour of the launcher in the case
that main() completes by throwing an exception.

What we have in the launcher is:

(*env)->CallStaticVoidMethod(env, mainClass, mainID, mainArgs);
ret = (*env)->ExceptionOccurred(env) == NULL ? 0 : 1;
LEAVE();

where LEAVE would have expanded to:

if ((*vm)->DetachCurrentThread(vm) != JNI_OK) { \
JLI_ReportErrorMessage(JVM_ERROR2); \
ret = 1; \
} \
if (JNI_TRUE) { \
(*vm)->DestroyJavaVM(vm); \
return ret; \
} \

so note that we are potentially calling DetachCurrentThread with an
exception pending - which is prohibited by JNI**, but which we
actually rely on for desired operation as DetachCurrentThread calls
thread->exit() which performs uncaught exception handling (ie it
prints the exception message and stacktrace) and then clears the
exception! (Hence DestroyJavaVM is not called with any pending
exceptions.)

**JNI spec needs to be modified here.

With the current change we have now inserted the following at the
start of LEAVE:

SetNativeThreadName(env, "DestroyJavaVM");  \
if ((*env)->ExceptionOccurred(env)) { \
(*env)->ExceptionClear(env);   \
} \

this has two unintended consequences:

1. SetNativeThreadName itself calls a number of JNI methods, with the
exception pending - which is not permitted. So straight away where we
have:

   NULL_CHECK(cls = FindBootStrapClass(env, "java/lang/Thread"));

FindBootStrapClass calls JVM_FindClassFromBootLoader, which make calls
using the VM's CHECK_NULL macro - which checks for a pending exception
(which it finds) and returns NULL. So the jli NULL_CHECK macro then
reports a JNI error:

Error: A JNI error has occurred, please check your installation and
try again


2. There is no longer an exception from main() for DetachCurrentThread
to report, so we exit with a return code of 1 as required, but don't
report the exception message/stacktrace.

I don't see a reasonable solution for this other than abandoning the
attempt to change the name from "main" to "DestroyJavaVM" - which if
we can't do, I question the utility of setting the name in the first
place (while it might be useful in some circumstances [when main() is
running] it will cause confusion in others [when main() has exited]).

David
-

On 25/04/2016 3:47 PM, David Holmes wrote:

Looks good to me.

I'll sponsor this "tomorrow".

Thanks,
David

On 23/04/2016 11:24 PM, Yasumasa Suenaga wrote:

Hi Kumar,

Thank you for your comment!
I've fixed them and uploaded new webrev. Could you review again?

   http://cr.openjdk.java.net/~ysuenaga/JDK-8152690/webrev.06/hotspot/
   http://cr.openjdk.java.net/~ysuenaga/JDK-8152690/webrev.06/jdk/


Thanks,

Yasumasa


On 2016/04/23 1:14, Kumar Srinivasan wrote:


Also a couple of minor suggestions:

- * Set native thread name as possible.
+ * Set native thread name if possible.


+  /*
-   * We can clear pending exception because exception at this
point
-   * is not critical.
+   */

+  /*
+   * Clear non critical pending exceptions at this point.
+   */

Thanks
Kumar


Hi,

This is in the wrong place:

1715 if ((*env)->ExceptionOccurred(env)) {
1716   /*
1717* We can clear pending exception because exception at
this point
1718* is not critical.
1719*/
1720   (*env)->ExceptionClear(env);
1721 }

This above needs to be after
 391 SetNativeThreadName(env, "main");
 392

Here is why, supposing 1704 through 

Re: RFR: JDK-8152690: main thread does not have native thread name

[Yasumasa Suenaga]

Hi David, Kumar,

I think that we should evacuate original exception before DestroyJavaVM
thread name is set.

  http://cr.openjdk.java.net/~ysuenaga/JDK-8152690/webrev.07/hotspot/
  http://cr.openjdk.java.net/~ysuenaga/JDK-8152690/webrev.07/jdk/

I added it to LEAVE macro in new webrev.
I tested it with following code. It works fine.

-
public void main(String[] args){
  throw new RuntimeException("test");
}
-

What do you think about it?


Thanks,

Yasumasa


On 2016/04/26 11:16, David Holmes wrote:

Hi Yasumasa, Kumar,

Turns out this change breaks the behaviour of the launcher in the case that 
main() completes by throwing an exception.

What we have in the launcher is:

(*env)->CallStaticVoidMethod(env, mainClass, mainID, mainArgs);
ret = (*env)->ExceptionOccurred(env) == NULL ? 0 : 1;
LEAVE();

where LEAVE would have expanded to:

if ((*vm)->DetachCurrentThread(vm) != JNI_OK) { \
JLI_ReportErrorMessage(JVM_ERROR2); \
ret = 1; \
} \
if (JNI_TRUE) { \
(*vm)->DestroyJavaVM(vm); \
return ret; \
} \

so note that we are potentially calling DetachCurrentThread with an exception 
pending - which is prohibited by JNI**, but which we actually rely on for desired 
operation as DetachCurrentThread calls thread->exit() which performs uncaught 
exception handling (ie it prints the exception message and stacktrace) and then 
clears the exception! (Hence DestroyJavaVM is not called with any pending 
exceptions.)

**JNI spec needs to be modified here.

With the current change we have now inserted the following at the start of 
LEAVE:

SetNativeThreadName(env, "DestroyJavaVM");  \
if ((*env)->ExceptionOccurred(env)) { \
(*env)->ExceptionClear(env);   \
} \

this has two unintended consequences:

1. SetNativeThreadName itself calls a number of JNI methods, with the exception 
pending - which is not permitted. So straight away where we have:

   NULL_CHECK(cls = FindBootStrapClass(env, "java/lang/Thread"));

FindBootStrapClass calls JVM_FindClassFromBootLoader, which make calls using 
the VM's CHECK_NULL macro - which checks for a pending exception (which it 
finds) and returns NULL. So the jli NULL_CHECK macro then reports a JNI error:

Error: A JNI error has occurred, please check your installation and try again


2. There is no longer an exception from main() for DetachCurrentThread to 
report, so we exit with a return code of 1 as required, but don't report the 
exception message/stacktrace.

I don't see a reasonable solution for this other than abandoning the attempt to change the name 
from "main" to "DestroyJavaVM" - which if we can't do, I question the utility 
of setting the name in the first place (while it might be useful in some circumstances [when main() 
is running] it will cause confusion in others [when main() has exited]).

David
-

On 25/04/2016 3:47 PM, David Holmes wrote:

Looks good to me.

I'll sponsor this "tomorrow".

Thanks,
David

On 23/04/2016 11:24 PM, Yasumasa Suenaga wrote:

Hi Kumar,

Thank you for your comment!
I've fixed them and uploaded new webrev. Could you review again?

   http://cr.openjdk.java.net/~ysuenaga/JDK-8152690/webrev.06/hotspot/
   http://cr.openjdk.java.net/~ysuenaga/JDK-8152690/webrev.06/jdk/


Thanks,

Yasumasa


On 2016/04/23 1:14, Kumar Srinivasan wrote:


Also a couple of minor suggestions:

- * Set native thread name as possible.
+ * Set native thread name if possible.


+  /*
-   * We can clear pending exception because exception at this point
-   * is not critical.
+   */

+  /*
+   * Clear non critical pending exceptions at this point.
+   */

Thanks
Kumar


Hi,

This is in the wrong place:

1715 if ((*env)->ExceptionOccurred(env)) {
1716   /*
1717* We can clear pending exception because exception at
this point
1718* is not critical.
1719*/
1720   (*env)->ExceptionClear(env);
1721 }

This above needs to be after
 391 SetNativeThreadName(env, "main");
 392

Here is why, supposing 1704 through 1711, returns a NULL,
but have also encountered an exception. In which case
the method SetNativeThreadName will return to the caller, as
if nothing has happened, because NULL_CHECK simply checks for NULL
and returns to the caller. This will cause the caller to enter
the VM with exceptions.

Kumar


On 4/22/2016 5:11 AM, Yasumasa Suenaga wrote:

Hi David,


I don't think we need to report the exception, but can just ignore
it. Either way we have to clear the exception before continuing.


I've fixed it in new webrev:

http://cr.openjdk.java.net/~ysuenaga/JDK-8152690/webrev.05/hotspot/
http://cr.openjdk.java.net/~ysuenaga/JDK-8152690/webrev.05/jdk/


Thanks,

Yasumasa


On 2016/04/22 15:33, David Holmes wrote:

On 22/04/2016 1:36 PM, Yasumasa Suenaga wrote:

Hi all,

I have uploaded webrev.04 as below.
Could you review again?

Re: RFR: JDK-8152690: main thread does not have native thread name

[David Holmes]

Hi Yasumasa, Kumar,

Turns out this change breaks the behaviour of the launcher in the case 
that main() completes by throwing an exception.


What we have in the launcher is:

(*env)->CallStaticVoidMethod(env, mainClass, mainID, mainArgs);
ret = (*env)->ExceptionOccurred(env) == NULL ? 0 : 1;
LEAVE();

where LEAVE would have expanded to:

if ((*vm)->DetachCurrentThread(vm) != JNI_OK) { \
JLI_ReportErrorMessage(JVM_ERROR2); \
ret = 1; \
} \
if (JNI_TRUE) { \
(*vm)->DestroyJavaVM(vm); \
return ret; \
} \

so note that we are potentially calling DetachCurrentThread with an 
exception pending - which is prohibited by JNI**, but which we actually 
rely on for desired operation as DetachCurrentThread calls 
thread->exit() which performs uncaught exception handling (ie it prints 
the exception message and stacktrace) and then clears the exception! 
(Hence DestroyJavaVM is not called with any pending exceptions.)


**JNI spec needs to be modified here.

With the current change we have now inserted the following at the start 
of LEAVE:


SetNativeThreadName(env, "DestroyJavaVM");  \
if ((*env)->ExceptionOccurred(env)) { \
(*env)->ExceptionClear(env);   \
} \

this has two unintended consequences:

1. SetNativeThreadName itself calls a number of JNI methods, with the 
exception pending - which is not permitted. So straight away where we have:


   NULL_CHECK(cls = FindBootStrapClass(env, "java/lang/Thread"));

FindBootStrapClass calls JVM_FindClassFromBootLoader, which make calls 
using the VM's CHECK_NULL macro - which checks for a pending exception 
(which it finds) and returns NULL. So the jli NULL_CHECK macro then 
reports a JNI error:


Error: A JNI error has occurred, please check your installation and try 
again



2. There is no longer an exception from main() for DetachCurrentThread 
to report, so we exit with a return code of 1 as required, but don't 
report the exception message/stacktrace.


I don't see a reasonable solution for this other than abandoning the 
attempt to change the name from "main" to "DestroyJavaVM" - which if we 
can't do, I question the utility of setting the name in the first place 
(while it might be useful in some circumstances [when main() is running] 
it will cause confusion in others [when main() has exited]).


David
-

On 25/04/2016 3:47 PM, David Holmes wrote:

Looks good to me.

I'll sponsor this "tomorrow".

Thanks,
David

On 23/04/2016 11:24 PM, Yasumasa Suenaga wrote:

Hi Kumar,

Thank you for your comment!
I've fixed them and uploaded new webrev. Could you review again?

   http://cr.openjdk.java.net/~ysuenaga/JDK-8152690/webrev.06/hotspot/
   http://cr.openjdk.java.net/~ysuenaga/JDK-8152690/webrev.06/jdk/


Thanks,

Yasumasa


On 2016/04/23 1:14, Kumar Srinivasan wrote:


Also a couple of minor suggestions:

- * Set native thread name as possible.
+ * Set native thread name if possible.


+  /*
-   * We can clear pending exception because exception at this point
-   * is not critical.
+   */

+  /*
+   * Clear non critical pending exceptions at this point.
+   */

Thanks
Kumar


Hi,

This is in the wrong place:

1715 if ((*env)->ExceptionOccurred(env)) {
1716   /*
1717* We can clear pending exception because exception at
this point
1718* is not critical.
1719*/
1720   (*env)->ExceptionClear(env);
1721 }

This above needs to be after
 391 SetNativeThreadName(env, "main");
 392

Here is why, supposing 1704 through 1711, returns a NULL,
but have also encountered an exception. In which case
the method SetNativeThreadName will return to the caller, as
if nothing has happened, because NULL_CHECK simply checks for NULL
and returns to the caller. This will cause the caller to enter
the VM with exceptions.

Kumar


On 4/22/2016 5:11 AM, Yasumasa Suenaga wrote:

Hi David,


I don't think we need to report the exception, but can just ignore
it. Either way we have to clear the exception before continuing.


I've fixed it in new webrev:

http://cr.openjdk.java.net/~ysuenaga/JDK-8152690/webrev.05/hotspot/
http://cr.openjdk.java.net/~ysuenaga/JDK-8152690/webrev.05/jdk/


Thanks,

Yasumasa


On 2016/04/22 15:33, David Holmes wrote:

On 22/04/2016 1:36 PM, Yasumasa Suenaga wrote:

Hi all,

I have uploaded webrev.04 as below.
Could you review again?

 >  - hotspot:
 >
http://cr.openjdk.java.net/~ysuenaga/JDK-8152690/webrev.04/hotspot/


Looks fine.


 >
 >  - jdk:
 > http://cr.openjdk.java.net/~ysuenaga/JDK-8152690/webrev.04/jdk/


As per private email (but repeated here on the record) in java.c:

715 if ((*env)->ExceptionOccurred(env)) {
1716   JLI_ReportExceptionDescription(env);
1717 }

I don't think we need to report the exception, but can just ignore
it. Either way we have to clear the exception before continuing.

Thanks,
David


Thanks,

Re: RFR: JDK-8152690: main thread does not have native thread name

[David Holmes]

Looks good to me.

I'll sponsor this "tomorrow".

Thanks,
David

On 23/04/2016 11:24 PM, Yasumasa Suenaga wrote:

Hi Kumar,

Thank you for your comment!
I've fixed them and uploaded new webrev. Could you review again?

   http://cr.openjdk.java.net/~ysuenaga/JDK-8152690/webrev.06/hotspot/
   http://cr.openjdk.java.net/~ysuenaga/JDK-8152690/webrev.06/jdk/


Thanks,

Yasumasa


On 2016/04/23 1:14, Kumar Srinivasan wrote:


Also a couple of minor suggestions:

- * Set native thread name as possible.
+ * Set native thread name if possible.


+  /*
-   * We can clear pending exception because exception at this point
-   * is not critical.
+   */

+  /*
+   * Clear non critical pending exceptions at this point.
+   */

Thanks
Kumar


Hi,

This is in the wrong place:

1715 if ((*env)->ExceptionOccurred(env)) {
1716   /*
1717* We can clear pending exception because exception at
this point
1718* is not critical.
1719*/
1720   (*env)->ExceptionClear(env);
1721 }

This above needs to be after
 391 SetNativeThreadName(env, "main");
 392

Here is why, supposing 1704 through 1711, returns a NULL,
but have also encountered an exception. In which case
the method SetNativeThreadName will return to the caller, as
if nothing has happened, because NULL_CHECK simply checks for NULL
and returns to the caller. This will cause the caller to enter
the VM with exceptions.

Kumar


On 4/22/2016 5:11 AM, Yasumasa Suenaga wrote:

Hi David,


I don't think we need to report the exception, but can just ignore
it. Either way we have to clear the exception before continuing.


I've fixed it in new webrev:

http://cr.openjdk.java.net/~ysuenaga/JDK-8152690/webrev.05/hotspot/
http://cr.openjdk.java.net/~ysuenaga/JDK-8152690/webrev.05/jdk/


Thanks,

Yasumasa


On 2016/04/22 15:33, David Holmes wrote:

On 22/04/2016 1:36 PM, Yasumasa Suenaga wrote:

Hi all,

I have uploaded webrev.04 as below.
Could you review again?

 >  - hotspot:
 >
http://cr.openjdk.java.net/~ysuenaga/JDK-8152690/webrev.04/hotspot/


Looks fine.


 >
 >  - jdk:
 > http://cr.openjdk.java.net/~ysuenaga/JDK-8152690/webrev.04/jdk/


As per private email (but repeated here on the record) in java.c:

715 if ((*env)->ExceptionOccurred(env)) {
1716   JLI_ReportExceptionDescription(env);
1717 }

I don't think we need to report the exception, but can just ignore
it. Either way we have to clear the exception before continuing.

Thanks,
David


Thanks,

Yasumasa

2016/04/19 22:43 "Yasumasa Suenaga" >:
 >
 > Hi David,
 >
 > Thank you for your comment.
 > I uploaded new webrev. Could you review again?
 >
 >  - hotspot:
 >
http://cr.openjdk.java.net/~ysuenaga/JDK-8152690/webrev.04/hotspot/
 >
 >  - jdk:
 > http://cr.openjdk.java.net/~ysuenaga/JDK-8152690/webrev.04/jdk/
 >
 >
 >> That aside I'm not sure why you do this so late in the process, I
would have done it immediately after here:
 >
 >
 > I think that native thread name ("main") should be set just before
 > main method call.
 > However, main thread is already started, so I moved it as you
suggested.
 >
 >
 >> One thing I dislike about the current structure is that we
have to
go from char* to java.lang.String to call setNativeName which then
calls
JVM_SetNativeThreadName which converts the j.l.String back to a
char* !
 >
 >
 > SoI proposed to export new JVM function to set native thread
name with
 > const char *.
 >
 >
 > Thanks,
 >
 > Yasumasa
 >
 >
 >
 > On 2016/04/19 14:04, David Holmes wrote:
 >>
 >> Hi Yasumasa,
 >>
 >> Thanks for persevering with this to get it into the current form.
Sorry I haven't been able to do a detailed review until now.
 >>
 >> On 19/04/2016 9:28 AM, Yasumasa Suenaga wrote:
 >>>
 >>> Hi Gerard,
 >>>
 >>> 2016/04/19 3:14 "Gerard Ziemski" 
 >>> >>:
 >>>  >
 >>>  > hi Yasumasa,
 >>>  >
 >>>  > Nice work. I have 2 questions:
 >>>  >
 >>>  > 
 >>>  > File: java.c
 >>>  >
 >>>  > #1 Shouldn’t we be checking for
“(*env)->ExceptionOccurred(env)”
 >>> after every single JNI call? In this example instead of
NULL_CHECK,
 >>> should we be using CHECK_EXCEPTION_NULL_LEAVE macro?
 >>>
 >>> It is not critical if we encounter error at JNI function call
because
 >>> we cannot set native thread name only.
 >>> So I think that we do not need to leave from launcher process.
 >>
 >>
 >> I agree we do not need to abort if an exception occurs (and in
fact
I don't think an exception is even possible from this code), but we
should ensure any pending exception is cleared before any futher JNI
calls might be made. Note that NULL_CHECK is already used extensively
throughout the launcher code - so if this usage is wrong then it
is all
wrong! More on this code below ...
 >>
 >> Other comments:
 >>
 >> hotspot/src/share/vm/prims/jvm.cpp
 >>
 >> Please add a comment to 

Re: RFR: JDK-8152690: main thread does not have native thread name

[David Holmes]

On 23/04/2016 12:41 AM, Kumar Srinivasan wrote:

Hi,

This is in the wrong place:

1715 if ((*env)->ExceptionOccurred(env)) {
1716   /*
1717* We can clear pending exception because exception at this
point
1718* is not critical.
1719*/
1720   (*env)->ExceptionClear(env);
1721 }

This above needs to be after
  391 SetNativeThreadName(env, "main");
  392

Here is why, supposing 1704 through 1711, returns a NULL,
but have also encountered an exception. In which case
the method SetNativeThreadName will return to the caller, as
if nothing has happened, because NULL_CHECK simply checks for NULL
and returns to the caller. This will cause the caller to enter
the VM with exceptions.


Ah! Now I see the pattern. All the existing functions that use 
NULL_CHECK are followed by CHECK_EXCEPTION_LEAVE.


Thanks for pointing that out Kumar!

David



Kumar


On 4/22/2016 5:11 AM, Yasumasa Suenaga wrote:

Hi David,


I don't think we need to report the exception, but can just ignore
it. Either way we have to clear the exception before continuing.


I've fixed it in new webrev:

http://cr.openjdk.java.net/~ysuenaga/JDK-8152690/webrev.05/hotspot/
  http://cr.openjdk.java.net/~ysuenaga/JDK-8152690/webrev.05/jdk/


Thanks,

Yasumasa


On 2016/04/22 15:33, David Holmes wrote:

On 22/04/2016 1:36 PM, Yasumasa Suenaga wrote:

Hi all,

I have uploaded webrev.04 as below.
Could you review again?

 >  - hotspot:
 > http://cr.openjdk.java.net/~ysuenaga/JDK-8152690/webrev.04/hotspot/


Looks fine.


 >
 >  - jdk:
 > http://cr.openjdk.java.net/~ysuenaga/JDK-8152690/webrev.04/jdk/


As per private email (but repeated here on the record) in java.c:

715 if ((*env)->ExceptionOccurred(env)) {
1716   JLI_ReportExceptionDescription(env);
1717 }

I don't think we need to report the exception, but can just ignore
it. Either way we have to clear the exception before continuing.

Thanks,
David


Thanks,

Yasumasa

2016/04/19 22:43 "Yasumasa Suenaga" >:
 >
 > Hi David,
 >
 > Thank you for your comment.
 > I uploaded new webrev. Could you review again?
 >
 >  - hotspot:
 > http://cr.openjdk.java.net/~ysuenaga/JDK-8152690/webrev.04/hotspot/
 >
 >  - jdk:
 > http://cr.openjdk.java.net/~ysuenaga/JDK-8152690/webrev.04/jdk/
 >
 >
 >> That aside I'm not sure why you do this so late in the process, I
would have done it immediately after here:
 >
 >
 > I think that native thread name ("main") should be set just before
 > main method call.
 > However, main thread is already started, so I moved it as you
suggested.
 >
 >
 >> One thing I dislike about the current structure is that we have to
go from char* to java.lang.String to call setNativeName which then
calls
JVM_SetNativeThreadName which converts the j.l.String back to a char* !
 >
 >
 > SoI proposed to export new JVM function to set native thread name
with
 > const char *.
 >
 >
 > Thanks,
 >
 > Yasumasa
 >
 >
 >
 > On 2016/04/19 14:04, David Holmes wrote:
 >>
 >> Hi Yasumasa,
 >>
 >> Thanks for persevering with this to get it into the current form.
Sorry I haven't been able to do a detailed review until now.
 >>
 >> On 19/04/2016 9:28 AM, Yasumasa Suenaga wrote:
 >>>
 >>> Hi Gerard,
 >>>
 >>> 2016/04/19 3:14 "Gerard Ziemski" 
 >>> >>:
 >>>  >
 >>>  > hi Yasumasa,
 >>>  >
 >>>  > Nice work. I have 2 questions:
 >>>  >
 >>>  > 
 >>>  > File: java.c
 >>>  >
 >>>  > #1 Shouldn’t we be checking for
“(*env)->ExceptionOccurred(env)”
 >>> after every single JNI call? In this example instead of
NULL_CHECK,
 >>> should we be using CHECK_EXCEPTION_NULL_LEAVE macro?
 >>>
 >>> It is not critical if we encounter error at JNI function call
because
 >>> we cannot set native thread name only.
 >>> So I think that we do not need to leave from launcher process.
 >>
 >>
 >> I agree we do not need to abort if an exception occurs (and in fact
I don't think an exception is even possible from this code), but we
should ensure any pending exception is cleared before any futher JNI
calls might be made. Note that NULL_CHECK is already used extensively
throughout the launcher code - so if this usage is wrong then it is all
wrong! More on this code below ...
 >>
 >> Other comments:
 >>
 >> hotspot/src/share/vm/prims/jvm.cpp
 >>
 >> Please add a comment to the method now that you removed the
internal
comment:
 >>
 >> // Sets the native thread name for a JavaThread. If specifically
 >> // requested JNI-attached threads can also have their native
name set;
 >> // otherwise we do not modify JNI-attached threads as it may
interfere
 >> // with the application that created them.
 >>
 >> ---
 >>
 >> jdk/src/java.base/share/classes/java/lang/Thread.java
 >>
 >> Please add the following comments:
 >>
 >> +// Don't modify JNI-attached threads
 >>   setNativeName(name, false);
 >>
 

Re: RFR: JDK-8152690: main thread does not have native thread name

[Kumar Srinivasan]

Hi Yasumasa,

Looks good.

Thanks
Kumar

On 4/23/2016 6:24 AM, Yasumasa Suenaga wrote:

Hi Kumar,

Thank you for your comment!
I've fixed them and uploaded new webrev. Could you review again?

http://cr.openjdk.java.net/~ysuenaga/JDK-8152690/webrev.06/hotspot/
  http://cr.openjdk.java.net/~ysuenaga/JDK-8152690/webrev.06/jdk/


Thanks,

Yasumasa


On 2016/04/23 1:14, Kumar Srinivasan wrote:


Also a couple of minor suggestions:

- * Set native thread name as possible.
+ * Set native thread name if possible.


+  /*
-   * We can clear pending exception because exception at this point
-   * is not critical.
+   */

+  /*
+   * Clear non critical pending exceptions at this point.
+   */

Thanks
Kumar


Hi,

This is in the wrong place:

1715 if ((*env)->ExceptionOccurred(env)) {
1716   /*
1717* We can clear pending exception because exception at 
this point

1718* is not critical.
1719*/
1720   (*env)->ExceptionClear(env);
1721 }

This above needs to be after
 391 SetNativeThreadName(env, "main");
 392

Here is why, supposing 1704 through 1711, returns a NULL,
but have also encountered an exception. In which case
the method SetNativeThreadName will return to the caller, as
if nothing has happened, because NULL_CHECK simply checks for NULL
and returns to the caller. This will cause the caller to enter
the VM with exceptions.

Kumar


On 4/22/2016 5:11 AM, Yasumasa Suenaga wrote:

Hi David,

I don't think we need to report the exception, but can just ignore 
it. Either way we have to clear the exception before continuing.


I've fixed it in new webrev:

http://cr.openjdk.java.net/~ysuenaga/JDK-8152690/webrev.05/hotspot/
http://cr.openjdk.java.net/~ysuenaga/JDK-8152690/webrev.05/jdk/


Thanks,

Yasumasa


On 2016/04/22 15:33, David Holmes wrote:

On 22/04/2016 1:36 PM, Yasumasa Suenaga wrote:

Hi all,

I have uploaded webrev.04 as below.
Could you review again?

 >  - hotspot:
 > 
http://cr.openjdk.java.net/~ysuenaga/JDK-8152690/webrev.04/hotspot/


Looks fine.


 >
 >  - jdk:
 > http://cr.openjdk.java.net/~ysuenaga/JDK-8152690/webrev.04/jdk/


As per private email (but repeated here on the record) in java.c:

715 if ((*env)->ExceptionOccurred(env)) {
1716   JLI_ReportExceptionDescription(env);
1717 }

I don't think we need to report the exception, but can just ignore 
it. Either way we have to clear the exception before continuing.


Thanks,
David


Thanks,

Yasumasa

2016/04/19 22:43 "Yasumasa Suenaga" >:
 >
 > Hi David,
 >
 > Thank you for your comment.
 > I uploaded new webrev. Could you review again?
 >
 >  - hotspot:
 > 
http://cr.openjdk.java.net/~ysuenaga/JDK-8152690/webrev.04/hotspot/

 >
 >  - jdk:
 > http://cr.openjdk.java.net/~ysuenaga/JDK-8152690/webrev.04/jdk/
 >
 >
 >> That aside I'm not sure why you do this so late in the 
process, I

would have done it immediately after here:
 >
 >
 > I think that native thread name ("main") should be set just 
before

 > main method call.
 > However, main thread is already started, so I moved it as you 
suggested.

 >
 >
 >> One thing I dislike about the current structure is that we 
have to
go from char* to java.lang.String to call setNativeName which 
then calls
JVM_SetNativeThreadName which converts the j.l.String back to a 
char* !

 >
 >
 > SoI proposed to export new JVM function to set native thread 
name with

 > const char *.
 >
 >
 > Thanks,
 >
 > Yasumasa
 >
 >
 >
 > On 2016/04/19 14:04, David Holmes wrote:
 >>
 >> Hi Yasumasa,
 >>
 >> Thanks for persevering with this to get it into the current 
form.

Sorry I haven't been able to do a detailed review until now.
 >>
 >> On 19/04/2016 9:28 AM, Yasumasa Suenaga wrote:
 >>>
 >>> Hi Gerard,
 >>>
 >>> 2016/04/19 3:14 "Gerard Ziemski" 
 >>> >>:

 >>>  >
 >>>  > hi Yasumasa,
 >>>  >
 >>>  > Nice work. I have 2 questions:
 >>>  >
 >>>  > 
 >>>  > File: java.c
 >>>  >
 >>>  > #1 Shouldn’t we be checking for 
“(*env)->ExceptionOccurred(env)”
 >>> after every single JNI call? In this example instead of 
NULL_CHECK,

 >>> should we be using CHECK_EXCEPTION_NULL_LEAVE macro?
 >>>
 >>> It is not critical if we encounter error at JNI function 
call because

 >>> we cannot set native thread name only.
 >>> So I think that we do not need to leave from launcher process.
 >>
 >>
 >> I agree we do not need to abort if an exception occurs (and 
in fact

I don't think an exception is even possible from this code), but we
should ensure any pending exception is cleared before any futher JNI
calls might be made. Note that NULL_CHECK is already used 
extensively
throughout the launcher code - so if this usage is wrong then it 
is all

wrong! More on this code below ...
 >>
 >> Other comments:
 >>
 >> hotspot/src/share/vm/prims/jvm.cpp
 >>
 >> Please add a comment 

Re: RFR: JDK-8152690: main thread does not have native thread name

[Yasumasa Suenaga]

Hi Kumar,

Thank you for your comment!
I've fixed them and uploaded new webrev. Could you review again?

  http://cr.openjdk.java.net/~ysuenaga/JDK-8152690/webrev.06/hotspot/
  http://cr.openjdk.java.net/~ysuenaga/JDK-8152690/webrev.06/jdk/


Thanks,

Yasumasa


On 2016/04/23 1:14, Kumar Srinivasan wrote:


Also a couple of minor suggestions:

- * Set native thread name as possible.
+ * Set native thread name if possible.


+  /*
-   * We can clear pending exception because exception at this point
-   * is not critical.
+   */

+  /*
+   * Clear non critical pending exceptions at this point.
+   */

Thanks
Kumar


Hi,

This is in the wrong place:

1715 if ((*env)->ExceptionOccurred(env)) {
1716   /*
1717* We can clear pending exception because exception at this point
1718* is not critical.
1719*/
1720   (*env)->ExceptionClear(env);
1721 }

This above needs to be after
 391 SetNativeThreadName(env, "main");
 392

Here is why, supposing 1704 through 1711, returns a NULL,
but have also encountered an exception. In which case
the method SetNativeThreadName will return to the caller, as
if nothing has happened, because NULL_CHECK simply checks for NULL
and returns to the caller. This will cause the caller to enter
the VM with exceptions.

Kumar


On 4/22/2016 5:11 AM, Yasumasa Suenaga wrote:

Hi David,


I don't think we need to report the exception, but can just ignore it. Either 
way we have to clear the exception before continuing.


I've fixed it in new webrev:

http://cr.openjdk.java.net/~ysuenaga/JDK-8152690/webrev.05/hotspot/
http://cr.openjdk.java.net/~ysuenaga/JDK-8152690/webrev.05/jdk/


Thanks,

Yasumasa


On 2016/04/22 15:33, David Holmes wrote:

On 22/04/2016 1:36 PM, Yasumasa Suenaga wrote:

Hi all,

I have uploaded webrev.04 as below.
Could you review again?

 >  - hotspot:
 > http://cr.openjdk.java.net/~ysuenaga/JDK-8152690/webrev.04/hotspot/


Looks fine.


 >
 >  - jdk:
 > http://cr.openjdk.java.net/~ysuenaga/JDK-8152690/webrev.04/jdk/


As per private email (but repeated here on the record) in java.c:

715 if ((*env)->ExceptionOccurred(env)) {
1716   JLI_ReportExceptionDescription(env);
1717 }

I don't think we need to report the exception, but can just ignore it. Either 
way we have to clear the exception before continuing.

Thanks,
David


Thanks,

Yasumasa

2016/04/19 22:43 "Yasumasa Suenaga" >:
 >
 > Hi David,
 >
 > Thank you for your comment.
 > I uploaded new webrev. Could you review again?
 >
 >  - hotspot:
 > http://cr.openjdk.java.net/~ysuenaga/JDK-8152690/webrev.04/hotspot/
 >
 >  - jdk:
 > http://cr.openjdk.java.net/~ysuenaga/JDK-8152690/webrev.04/jdk/
 >
 >
 >> That aside I'm not sure why you do this so late in the process, I
would have done it immediately after here:
 >
 >
 > I think that native thread name ("main") should be set just before
 > main method call.
 > However, main thread is already started, so I moved it as you suggested.
 >
 >
 >> One thing I dislike about the current structure is that we have to
go from char* to java.lang.String to call setNativeName which then calls
JVM_SetNativeThreadName which converts the j.l.String back to a char* !
 >
 >
 > SoI proposed to export new JVM function to set native thread name with
 > const char *.
 >
 >
 > Thanks,
 >
 > Yasumasa
 >
 >
 >
 > On 2016/04/19 14:04, David Holmes wrote:
 >>
 >> Hi Yasumasa,
 >>
 >> Thanks for persevering with this to get it into the current form.
Sorry I haven't been able to do a detailed review until now.
 >>
 >> On 19/04/2016 9:28 AM, Yasumasa Suenaga wrote:
 >>>
 >>> Hi Gerard,
 >>>
 >>> 2016/04/19 3:14 "Gerard Ziemski" 
 >>> >>:
 >>>  >
 >>>  > hi Yasumasa,
 >>>  >
 >>>  > Nice work. I have 2 questions:
 >>>  >
 >>>  > 
 >>>  > File: java.c
 >>>  >
 >>>  > #1 Shouldn’t we be checking for “(*env)->ExceptionOccurred(env)”
 >>> after every single JNI call? In this example instead of NULL_CHECK,
 >>> should we be using CHECK_EXCEPTION_NULL_LEAVE macro?
 >>>
 >>> It is not critical if we encounter error at JNI function call because
 >>> we cannot set native thread name only.
 >>> So I think that we do not need to leave from launcher process.
 >>
 >>
 >> I agree we do not need to abort if an exception occurs (and in fact
I don't think an exception is even possible from this code), but we
should ensure any pending exception is cleared before any futher JNI
calls might be made. Note that NULL_CHECK is already used extensively
throughout the launcher code - so if this usage is wrong then it is all
wrong! More on this code below ...
 >>
 >> Other comments:
 >>
 >> hotspot/src/share/vm/prims/jvm.cpp
 >>
 >> Please add a comment to the method now that you removed the internal
comment:
 >>
 >> // Sets the native thread name for a JavaThread. If 

Re: RFR: JDK-8152690: main thread does not have native thread name

[Kumar Srinivasan]

Also a couple of minor suggestions:

- * Set native thread name as possible.
+ * Set native thread name if possible.


+  /*
-   * We can clear pending exception because exception at this point
-   * is not critical.
+   */

+  /*
+   * Clear non critical pending exceptions at this point.
+   */

Thanks
Kumar


Hi,

This is in the wrong place:

1715 if ((*env)->ExceptionOccurred(env)) {
1716   /*
1717* We can clear pending exception because exception at this 
point

1718* is not critical.
1719*/
1720   (*env)->ExceptionClear(env);
1721 }

This above needs to be after
 391 SetNativeThreadName(env, "main");
 392

Here is why, supposing 1704 through 1711, returns a NULL,
but have also encountered an exception. In which case
the method SetNativeThreadName will return to the caller, as
if nothing has happened, because NULL_CHECK simply checks for NULL
and returns to the caller. This will cause the caller to enter
the VM with exceptions.

Kumar


On 4/22/2016 5:11 AM, Yasumasa Suenaga wrote:

Hi David,

I don't think we need to report the exception, but can just ignore 
it. Either way we have to clear the exception before continuing.


I've fixed it in new webrev:

http://cr.openjdk.java.net/~ysuenaga/JDK-8152690/webrev.05/hotspot/
http://cr.openjdk.java.net/~ysuenaga/JDK-8152690/webrev.05/jdk/


Thanks,

Yasumasa


On 2016/04/22 15:33, David Holmes wrote:

On 22/04/2016 1:36 PM, Yasumasa Suenaga wrote:

Hi all,

I have uploaded webrev.04 as below.
Could you review again?

 >  - hotspot:
 > http://cr.openjdk.java.net/~ysuenaga/JDK-8152690/webrev.04/hotspot/


Looks fine.


 >
 >  - jdk:
 > http://cr.openjdk.java.net/~ysuenaga/JDK-8152690/webrev.04/jdk/


As per private email (but repeated here on the record) in java.c:

715 if ((*env)->ExceptionOccurred(env)) {
1716   JLI_ReportExceptionDescription(env);
1717 }

I don't think we need to report the exception, but can just ignore 
it. Either way we have to clear the exception before continuing.


Thanks,
David


Thanks,

Yasumasa

2016/04/19 22:43 "Yasumasa Suenaga" >:
 >
 > Hi David,
 >
 > Thank you for your comment.
 > I uploaded new webrev. Could you review again?
 >
 >  - hotspot:
 > http://cr.openjdk.java.net/~ysuenaga/JDK-8152690/webrev.04/hotspot/
 >
 >  - jdk:
 > http://cr.openjdk.java.net/~ysuenaga/JDK-8152690/webrev.04/jdk/
 >
 >
 >> That aside I'm not sure why you do this so late in the process, I
would have done it immediately after here:
 >
 >
 > I think that native thread name ("main") should be set just before
 > main method call.
 > However, main thread is already started, so I moved it as you 
suggested.

 >
 >
 >> One thing I dislike about the current structure is that we have to
go from char* to java.lang.String to call setNativeName which then 
calls
JVM_SetNativeThreadName which converts the j.l.String back to a 
char* !

 >
 >
 > SoI proposed to export new JVM function to set native thread 
name with

 > const char *.
 >
 >
 > Thanks,
 >
 > Yasumasa
 >
 >
 >
 > On 2016/04/19 14:04, David Holmes wrote:
 >>
 >> Hi Yasumasa,
 >>
 >> Thanks for persevering with this to get it into the current form.
Sorry I haven't been able to do a detailed review until now.
 >>
 >> On 19/04/2016 9:28 AM, Yasumasa Suenaga wrote:
 >>>
 >>> Hi Gerard,
 >>>
 >>> 2016/04/19 3:14 "Gerard Ziemski" 
 >>> >>:

 >>>  >
 >>>  > hi Yasumasa,
 >>>  >
 >>>  > Nice work. I have 2 questions:
 >>>  >
 >>>  > 
 >>>  > File: java.c
 >>>  >
 >>>  > #1 Shouldn’t we be checking for 
“(*env)->ExceptionOccurred(env)”
 >>> after every single JNI call? In this example instead of 
NULL_CHECK,

 >>> should we be using CHECK_EXCEPTION_NULL_LEAVE macro?
 >>>
 >>> It is not critical if we encounter error at JNI function call  
because

 >>> we cannot set native thread name only.
 >>> So I think that we do not need to leave from launcher process.
 >>
 >>
 >> I agree we do not need to abort if an exception occurs (and in 
fact

I don't think an exception is even possible from this code), but we
should ensure any pending exception is cleared before any futher JNI
calls might be made. Note that NULL_CHECK is already used extensively
throughout the launcher code - so if this usage is wrong then it is 
all

wrong! More on this code below ...
 >>
 >> Other comments:
 >>
 >> hotspot/src/share/vm/prims/jvm.cpp
 >>
 >> Please add a comment to the method now that you removed the 
internal

comment:
 >>
 >> // Sets the native thread name for a JavaThread. If specifically
 >> // requested JNI-attached threads can also have their native 
name set;
 >> // otherwise we do not modify JNI-attached threads as it may 
interfere

 >> // with the application that created them.
 >>
 >> ---
 >>
 >> 

Re: RFR: JDK-8152690: main thread does not have native thread name

[Kumar Srinivasan]

Hi,

This is in the wrong place:

1715 if ((*env)->ExceptionOccurred(env)) {
1716   /*
1717* We can clear pending exception because exception at this point
1718* is not critical.
1719*/
1720   (*env)->ExceptionClear(env);
1721 }

This above needs to be after
 391 SetNativeThreadName(env, "main");
 392

Here is why, supposing 1704 through 1711, returns a NULL,
but have also encountered an exception. In which case
the method SetNativeThreadName will return to the caller, as
if nothing has happened, because NULL_CHECK simply checks for NULL
and returns to the caller. This will cause the caller to enter
the VM with exceptions.

Kumar


On 4/22/2016 5:11 AM, Yasumasa Suenaga wrote:

Hi David,

I don't think we need to report the exception, but can just ignore 
it. Either way we have to clear the exception before continuing.


I've fixed it in new webrev:

http://cr.openjdk.java.net/~ysuenaga/JDK-8152690/webrev.05/hotspot/
  http://cr.openjdk.java.net/~ysuenaga/JDK-8152690/webrev.05/jdk/


Thanks,

Yasumasa


On 2016/04/22 15:33, David Holmes wrote:

On 22/04/2016 1:36 PM, Yasumasa Suenaga wrote:

Hi all,

I have uploaded webrev.04 as below.
Could you review again?

 >  - hotspot:
 > http://cr.openjdk.java.net/~ysuenaga/JDK-8152690/webrev.04/hotspot/


Looks fine.


 >
 >  - jdk:
 > http://cr.openjdk.java.net/~ysuenaga/JDK-8152690/webrev.04/jdk/


As per private email (but repeated here on the record) in java.c:

715 if ((*env)->ExceptionOccurred(env)) {
1716   JLI_ReportExceptionDescription(env);
1717 }

I don't think we need to report the exception, but can just ignore 
it. Either way we have to clear the exception before continuing.


Thanks,
David


Thanks,

Yasumasa

2016/04/19 22:43 "Yasumasa Suenaga" >:
 >
 > Hi David,
 >
 > Thank you for your comment.
 > I uploaded new webrev. Could you review again?
 >
 >  - hotspot:
 > http://cr.openjdk.java.net/~ysuenaga/JDK-8152690/webrev.04/hotspot/
 >
 >  - jdk:
 > http://cr.openjdk.java.net/~ysuenaga/JDK-8152690/webrev.04/jdk/
 >
 >
 >> That aside I'm not sure why you do this so late in the process, I
would have done it immediately after here:
 >
 >
 > I think that native thread name ("main") should be set just before
 > main method call.
 > However, main thread is already started, so I moved it as you 
suggested.

 >
 >
 >> One thing I dislike about the current structure is that we have to
go from char* to java.lang.String to call setNativeName which then 
calls

JVM_SetNativeThreadName which converts the j.l.String back to a char* !
 >
 >
 > SoI proposed to export new JVM function to set native thread name 
with

 > const char *.
 >
 >
 > Thanks,
 >
 > Yasumasa
 >
 >
 >
 > On 2016/04/19 14:04, David Holmes wrote:
 >>
 >> Hi Yasumasa,
 >>
 >> Thanks for persevering with this to get it into the current form.
Sorry I haven't been able to do a detailed review until now.
 >>
 >> On 19/04/2016 9:28 AM, Yasumasa Suenaga wrote:
 >>>
 >>> Hi Gerard,
 >>>
 >>> 2016/04/19 3:14 "Gerard Ziemski" 
 >>> >>:

 >>>  >
 >>>  > hi Yasumasa,
 >>>  >
 >>>  > Nice work. I have 2 questions:
 >>>  >
 >>>  > 
 >>>  > File: java.c
 >>>  >
 >>>  > #1 Shouldn’t we be checking for 
“(*env)->ExceptionOccurred(env)”
 >>> after every single JNI call? In this example instead of 
NULL_CHECK,

 >>> should we be using CHECK_EXCEPTION_NULL_LEAVE macro?
 >>>
 >>> It is not critical if we encounter error at JNI function call  
because

 >>> we cannot set native thread name only.
 >>> So I think that we do not need to leave from launcher process.
 >>
 >>
 >> I agree we do not need to abort if an exception occurs (and in fact
I don't think an exception is even possible from this code), but we
should ensure any pending exception is cleared before any futher JNI
calls might be made. Note that NULL_CHECK is already used extensively
throughout the launcher code - so if this usage is wrong then it is all
wrong! More on this code below ...
 >>
 >> Other comments:
 >>
 >> hotspot/src/share/vm/prims/jvm.cpp
 >>
 >> Please add a comment to the method now that you removed the 
internal

comment:
 >>
 >> // Sets the native thread name for a JavaThread. If specifically
 >> // requested JNI-attached threads can also have their native 
name set;
 >> // otherwise we do not modify JNI-attached threads as it may 
interfere

 >> // with the application that created them.
 >>
 >> ---
 >>
 >> jdk/src/java.base/share/classes/java/lang/Thread.java
 >>
 >> Please add the following comments:
 >>
 >> +// Don't modify JNI-attached threads
 >>   setNativeName(name, false);
 >>
 >> + // May be called directly via JNI or reflection (when 
permitted) to

 >> + // allow JNI-attached threads to set their native name
 >>   private native void setNativeName(String name, 

Re: RFR: JDK-8152690: main thread does not have native thread name

[Yasumasa Suenaga]

Hi David,


I don't think we need to report the exception, but can just ignore it. Either 
way we have to clear the exception before continuing.


I've fixed it in new webrev:

  http://cr.openjdk.java.net/~ysuenaga/JDK-8152690/webrev.05/hotspot/
  http://cr.openjdk.java.net/~ysuenaga/JDK-8152690/webrev.05/jdk/


Thanks,

Yasumasa


On 2016/04/22 15:33, David Holmes wrote:

On 22/04/2016 1:36 PM, Yasumasa Suenaga wrote:

Hi all,

I have uploaded webrev.04 as below.
Could you review again?

 >  - hotspot:
 > http://cr.openjdk.java.net/~ysuenaga/JDK-8152690/webrev.04/hotspot/


Looks fine.


 >
 >  - jdk:
 > http://cr.openjdk.java.net/~ysuenaga/JDK-8152690/webrev.04/jdk/


As per private email (but repeated here on the record) in java.c:

715 if ((*env)->ExceptionOccurred(env)) {
1716   JLI_ReportExceptionDescription(env);
1717 }

I don't think we need to report the exception, but can just ignore it. Either 
way we have to clear the exception before continuing.

Thanks,
David


Thanks,

Yasumasa

2016/04/19 22:43 "Yasumasa Suenaga" >:
 >
 > Hi David,
 >
 > Thank you for your comment.
 > I uploaded new webrev. Could you review again?
 >
 >  - hotspot:
 > http://cr.openjdk.java.net/~ysuenaga/JDK-8152690/webrev.04/hotspot/
 >
 >  - jdk:
 > http://cr.openjdk.java.net/~ysuenaga/JDK-8152690/webrev.04/jdk/
 >
 >
 >> That aside I'm not sure why you do this so late in the process, I
would have done it immediately after here:
 >
 >
 > I think that native thread name ("main") should be set just before
 > main method call.
 > However, main thread is already started, so I moved it as you suggested.
 >
 >
 >> One thing I dislike about the current structure is that we have to
go from char* to java.lang.String to call setNativeName which then calls
JVM_SetNativeThreadName which converts the j.l.String back to a char* !
 >
 >
 > SoI proposed to export new JVM function to set native thread name with
 > const char *.
 >
 >
 > Thanks,
 >
 > Yasumasa
 >
 >
 >
 > On 2016/04/19 14:04, David Holmes wrote:
 >>
 >> Hi Yasumasa,
 >>
 >> Thanks for persevering with this to get it into the current form.
Sorry I haven't been able to do a detailed review until now.
 >>
 >> On 19/04/2016 9:28 AM, Yasumasa Suenaga wrote:
 >>>
 >>> Hi Gerard,
 >>>
 >>> 2016/04/19 3:14 "Gerard Ziemski" 
 >>> >>:
 >>>  >
 >>>  > hi Yasumasa,
 >>>  >
 >>>  > Nice work. I have 2 questions:
 >>>  >
 >>>  > 
 >>>  > File: java.c
 >>>  >
 >>>  > #1 Shouldn’t we be checking for “(*env)->ExceptionOccurred(env)”
 >>> after every single JNI call? In this example instead of NULL_CHECK,
 >>> should we be using CHECK_EXCEPTION_NULL_LEAVE macro?
 >>>
 >>> It is not critical if we encounter error at JNI function call  because
 >>> we cannot set native thread name only.
 >>> So I think that we do not need to leave from launcher process.
 >>
 >>
 >> I agree we do not need to abort if an exception occurs (and in fact
I don't think an exception is even possible from this code), but we
should ensure any pending exception is cleared before any futher JNI
calls might be made. Note that NULL_CHECK is already used extensively
throughout the launcher code - so if this usage is wrong then it is all
wrong! More on this code below ...
 >>
 >> Other comments:
 >>
 >> hotspot/src/share/vm/prims/jvm.cpp
 >>
 >> Please add a comment to the method now that you removed the internal
comment:
 >>
 >> // Sets the native thread name for a JavaThread. If specifically
 >> // requested JNI-attached threads can also have their native name set;
 >> // otherwise we do not modify JNI-attached threads as it may interfere
 >> // with the application that created them.
 >>
 >> ---
 >>
 >> jdk/src/java.base/share/classes/java/lang/Thread.java
 >>
 >> Please add the following comments:
 >>
 >> +// Don't modify JNI-attached threads
 >>   setNativeName(name, false);
 >>
 >> + // May be called directly via JNI or reflection (when permitted) to
 >> + // allow JNI-attached threads to set their native name
 >>   private native void setNativeName(String name, boolean
allowAttachedThread);
 >>
 >> ---
 >>
 >> jd/src/java.base/share/native/libjli/java.c
 >>
 >> 328 #define LEAVE() \
 >> 329 SetNativeThreadName(env, "DestroyJavaVM"); \
 >>
 >> I was going to suggest this be set later, but realized we have to be
attached to do this and that happens inside DestroyJavaVM. :)
 >>
 >> + /* Set native thread name. */
 >> + SetNativeThreadName(env, "main");
 >>
 >> The comment is redundant given the name of the method. That aside
I'm not sure why you do this so late in the process, I would have done
it immediately after here:
 >>
 >>   386 if (!InitializeJVM(, , )) {
 >>   387 JLI_ReportErrorMessage(JVM_ERROR1);
 >>   388 exit(1);
 >>   389 }
 >>   +   

Re: RFR: JDK-8152690: main thread does not have native thread name

[David Holmes]

On 22/04/2016 1:36 PM, Yasumasa Suenaga wrote:

Hi all,

I have uploaded webrev.04 as below.
Could you review again?

 >  - hotspot:
 > http://cr.openjdk.java.net/~ysuenaga/JDK-8152690/webrev.04/hotspot/


Looks fine.


 >
 >  - jdk:
 > http://cr.openjdk.java.net/~ysuenaga/JDK-8152690/webrev.04/jdk/


As per private email (but repeated here on the record) in java.c:

715 if ((*env)->ExceptionOccurred(env)) {
1716   JLI_ReportExceptionDescription(env);
1717 }

I don't think we need to report the exception, but can just ignore it. 
Either way we have to clear the exception before continuing.


Thanks,
David


Thanks,

Yasumasa

2016/04/19 22:43 "Yasumasa Suenaga" >:
 >
 > Hi David,
 >
 > Thank you for your comment.
 > I uploaded new webrev. Could you review again?
 >
 >  - hotspot:
 > http://cr.openjdk.java.net/~ysuenaga/JDK-8152690/webrev.04/hotspot/
 >
 >  - jdk:
 > http://cr.openjdk.java.net/~ysuenaga/JDK-8152690/webrev.04/jdk/
 >
 >
 >> That aside I'm not sure why you do this so late in the process, I
would have done it immediately after here:
 >
 >
 > I think that native thread name ("main") should be set just before
 > main method call.
 > However, main thread is already started, so I moved it as you suggested.
 >
 >
 >> One thing I dislike about the current structure is that we have to
go from char* to java.lang.String to call setNativeName which then calls
JVM_SetNativeThreadName which converts the j.l.String back to a char* !
 >
 >
 > SoI proposed to export new JVM function to set native thread name with
 > const char *.
 >
 >
 > Thanks,
 >
 > Yasumasa
 >
 >
 >
 > On 2016/04/19 14:04, David Holmes wrote:
 >>
 >> Hi Yasumasa,
 >>
 >> Thanks for persevering with this to get it into the current form.
Sorry I haven't been able to do a detailed review until now.
 >>
 >> On 19/04/2016 9:28 AM, Yasumasa Suenaga wrote:
 >>>
 >>> Hi Gerard,
 >>>
 >>> 2016/04/19 3:14 "Gerard Ziemski" 
 >>> >>:
 >>>  >
 >>>  > hi Yasumasa,
 >>>  >
 >>>  > Nice work. I have 2 questions:
 >>>  >
 >>>  > 
 >>>  > File: java.c
 >>>  >
 >>>  > #1 Shouldn’t we be checking for “(*env)->ExceptionOccurred(env)”
 >>> after every single JNI call? In this example instead of NULL_CHECK,
 >>> should we be using CHECK_EXCEPTION_NULL_LEAVE macro?
 >>>
 >>> It is not critical if we encounter error at JNI function call  because
 >>> we cannot set native thread name only.
 >>> So I think that we do not need to leave from launcher process.
 >>
 >>
 >> I agree we do not need to abort if an exception occurs (and in fact
I don't think an exception is even possible from this code), but we
should ensure any pending exception is cleared before any futher JNI
calls might be made. Note that NULL_CHECK is already used extensively
throughout the launcher code - so if this usage is wrong then it is all
wrong! More on this code below ...
 >>
 >> Other comments:
 >>
 >> hotspot/src/share/vm/prims/jvm.cpp
 >>
 >> Please add a comment to the method now that you removed the internal
comment:
 >>
 >> // Sets the native thread name for a JavaThread. If specifically
 >> // requested JNI-attached threads can also have their native name set;
 >> // otherwise we do not modify JNI-attached threads as it may interfere
 >> // with the application that created them.
 >>
 >> ---
 >>
 >> jdk/src/java.base/share/classes/java/lang/Thread.java
 >>
 >> Please add the following comments:
 >>
 >> +// Don't modify JNI-attached threads
 >>   setNativeName(name, false);
 >>
 >> + // May be called directly via JNI or reflection (when permitted) to
 >> + // allow JNI-attached threads to set their native name
 >>   private native void setNativeName(String name, boolean
allowAttachedThread);
 >>
 >> ---
 >>
 >> jd/src/java.base/share/native/libjli/java.c
 >>
 >> 328 #define LEAVE() \
 >> 329 SetNativeThreadName(env, "DestroyJavaVM"); \
 >>
 >> I was going to suggest this be set later, but realized we have to be
attached to do this and that happens inside DestroyJavaVM. :)
 >>
 >> + /* Set native thread name. */
 >> + SetNativeThreadName(env, "main");
 >>
 >> The comment is redundant given the name of the method. That aside
I'm not sure why you do this so late in the process, I would have done
it immediately after here:
 >>
 >>   386 if (!InitializeJVM(, , )) {
 >>   387 JLI_ReportErrorMessage(JVM_ERROR1);
 >>   388 exit(1);
 >>   389 }
 >>   +   SetNativeThreadName(env, "main");
 >>
 >>
 >> + /**
 >> +  * Set native thread name as possible.
 >> +  */
 >>
 >> Other than the as->if change I'm unclear where the "possible" bit
comes into play - why would it not be possible?
 >>
 >> 1705 NULL_CHECK(cls = FindBootStrapClass(env, "java/lang/Thread"));
 >> 1706 NULL_CHECK(currentThreadID = (*env)->GetStaticMethodID(env,
cls,
 >> 

Re: RFR: JDK-8152690: main thread does not have native thread name

[Yasumasa Suenaga]

Hi all,

I have uploaded webrev.04 as below.
Could you review again?

>  - hotspot:
> http://cr.openjdk.java.net/~ysuenaga/JDK-8152690/webrev.04/hotspot/
>
>  - jdk:
> http://cr.openjdk.java.net/~ysuenaga/JDK-8152690/webrev.04/jdk/

Thanks,

Yasumasa

2016/04/19 22:43 "Yasumasa Suenaga" :
>
> Hi David,
>
> Thank you for your comment.
> I uploaded new webrev. Could you review again?
>
>  - hotspot:
> http://cr.openjdk.java.net/~ysuenaga/JDK-8152690/webrev.04/hotspot/
>
>  - jdk:
> http://cr.openjdk.java.net/~ysuenaga/JDK-8152690/webrev.04/jdk/
>
>
>> That aside I'm not sure why you do this so late in the process, I would
have done it immediately after here:
>
>
> I think that native thread name ("main") should be set just before
> main method call.
> However, main thread is already started, so I moved it as you suggested.
>
>
>> One thing I dislike about the current structure is that we have to go
from char* to java.lang.String to call setNativeName which then calls
JVM_SetNativeThreadName which converts the j.l.String back to a char* !
>
>
> SoI proposed to export new JVM function to set native thread name with
> const char *.
>
>
> Thanks,
>
> Yasumasa
>
>
>
> On 2016/04/19 14:04, David Holmes wrote:
>>
>> Hi Yasumasa,
>>
>> Thanks for persevering with this to get it into the current form. Sorry
I haven't been able to do a detailed review until now.
>>
>> On 19/04/2016 9:28 AM, Yasumasa Suenaga wrote:
>>>
>>> Hi Gerard,
>>>
>>> 2016/04/19 3:14 "Gerard Ziemski" >> >:
>>>  >
>>>  > hi Yasumasa,
>>>  >
>>>  > Nice work. I have 2 questions:
>>>  >
>>>  > 
>>>  > File: java.c
>>>  >
>>>  > #1 Shouldn’t we be checking for “(*env)->ExceptionOccurred(env)”
>>> after every single JNI call? In this example instead of NULL_CHECK,
>>> should we be using CHECK_EXCEPTION_NULL_LEAVE macro?
>>>
>>> It is not critical if we encounter error at JNI function call  because
>>> we cannot set native thread name only.
>>> So I think that we do not need to leave from launcher process.
>>
>>
>> I agree we do not need to abort if an exception occurs (and in fact I
don't think an exception is even possible from this code), but we should
ensure any pending exception is cleared before any futher JNI calls might
be made. Note that NULL_CHECK is already used extensively throughout the
launcher code - so if this usage is wrong then it is all wrong! More on
this code below ...
>>
>> Other comments:
>>
>> hotspot/src/share/vm/prims/jvm.cpp
>>
>> Please add a comment to the method now that you removed the internal
comment:
>>
>> // Sets the native thread name for a JavaThread. If specifically
>> // requested JNI-attached threads can also have their native name set;
>> // otherwise we do not modify JNI-attached threads as it may interfere
>> // with the application that created them.
>>
>> ---
>>
>> jdk/src/java.base/share/classes/java/lang/Thread.java
>>
>> Please add the following comments:
>>
>> +// Don't modify JNI-attached threads
>>   setNativeName(name, false);
>>
>> + // May be called directly via JNI or reflection (when permitted) to
>> + // allow JNI-attached threads to set their native name
>>   private native void setNativeName(String name, boolean
allowAttachedThread);
>>
>> ---
>>
>> jd/src/java.base/share/native/libjli/java.c
>>
>> 328 #define LEAVE() \
>> 329 SetNativeThreadName(env, "DestroyJavaVM"); \
>>
>> I was going to suggest this be set later, but realized we have to be
attached to do this and that happens inside DestroyJavaVM. :)
>>
>> + /* Set native thread name. */
>> + SetNativeThreadName(env, "main");
>>
>> The comment is redundant given the name of the method. That aside I'm
not sure why you do this so late in the process, I would have done it
immediately after here:
>>
>>   386 if (!InitializeJVM(, , )) {
>>   387 JLI_ReportErrorMessage(JVM_ERROR1);
>>   388 exit(1);
>>   389 }
>>   +   SetNativeThreadName(env, "main");
>>
>>
>> + /**
>> +  * Set native thread name as possible.
>> +  */
>>
>> Other than the as->if change I'm unclear where the "possible" bit comes
into play - why would it not be possible?
>>
>> 1705 NULL_CHECK(cls = FindBootStrapClass(env, "java/lang/Thread"));
>> 1706 NULL_CHECK(currentThreadID = (*env)->GetStaticMethodID(env, cls,
>> 1707  "currentThread",
"()Ljava/lang/Thread;"));
>> 1708 NULL_CHECK(currentThread = (*env)->CallStaticObjectMethod(env,
cls,
>> 1709 currentThreadID));
>> 1710 NULL_CHECK(setNativeNameID = (*env)->GetMethodID(env, cls,
>> 1711 "setNativeName",
"(Ljava/lang/String;Z)V"));
>> 1712 NULL_CHECK(nameString = (*env)->NewStringUTF(env, name));
>> 1713 (*env)->CallVoidMethod(env, currentThread, setNativeNameID,
>> 1714nameString, JNI_TRUE);
>>
>> As above NULL_CHECK is fine here, but 

Re: RFR: JDK-8152690: main thread does not have native thread name

[Yasumasa Suenaga]

Well if it is hard then the jbs must be labelled so, noreg-hard.


I agree to label noreg-hard.


I concur.


I added noreg-hard to JBS.


Yasumasa


On 2016/04/21 16:01, David Holmes wrote:

On 21/04/2016 12:59 AM, Yasumasa Suenaga wrote:

Hi Kumar,


Isn't there a management API or something to enumerate the threads ?


Not that shows native thread names.

Existing launcher tests should of course be unaffected by this change.


On Linux, user apps can get native thread name through
pthread_getname_np(3).
However, this function is not called in hotspot and jdk.

So I think it is difficult to get native thread name in all platforms.



Well if it is hard then the jbs must be labelled so, noreg-hard.


I agree to label noreg-hard.


I concur.

Thanks,
David



Thanks,

Yasumasa


On 2016/04/20 23:26, Kumar Srinivasan wrote:


On 4/20/2016 6:06 AM, David Holmes wrote:

On 20/04/2016 10:58 PM, Kumar Srinivasan wrote:


Hello,

One more thing, what about a launcher regression test ?


What kind of regression test? I've manually visually inspected the
desired behaviour but a test for it is very problematic. AFAIK there
are no tests for setting the native thread name.


Isn't there a management API or something to enumerate the threads ?
I am more worried about some seemingly innocuous launcher change
causing a regression.

Well if it is hard then the jbs must be labelled so, noreg-hard.

Kumar



David


Thanks
Kumar



On 4/19/2016 1:32 PM, David Holmes wrote:

On 20/04/2016 3:00 AM, Kumar Srinivasan wrote:

Hi David,

  493 /* Set native thread name. */
  494 SetNativeThreadName(env, "main");
  495
  496 /* Invoke main method. */
  497 (*env)->CallStaticVoidMethod(env, mainClass, mainID,
mainArgs);

Supposing an exception is thrown in 494 (a very remote case),
will we not re-enter the VM in 497 with an exception ?


Yes as I said:


1712 NULL_CHECK(nameString = (*env)->NewStringUTF(env, name));
1713 (*env)->CallVoidMethod(env, currentThread, setNativeNameID,
1714nameString, JNI_TRUE);

As above NULL_CHECK is fine here, but we should check for and clear
any pending exception after CallVoidMethod.



Agreed.

Kumar


Thanks,
David


Kumar


On 19/04/2016 10:54 PM, Gerard Ziemski wrote:



On Apr 19, 2016, at 12:04 AM, David Holmes

wrote:

On 19/04/2016 9:28 AM, Yasumasa Suenaga wrote:

Hi Gerard,

2016/04/19 3:14 "Gerard Ziemski" >:


hi Yasumasa,

Nice work. I have 2 questions:


File: java.c

#1 Shouldn’t we be checking for
“(*env)->ExceptionOccurred(env)”

after every single JNI call? In this example instead of
NULL_CHECK,
should we be using CHECK_EXCEPTION_NULL_LEAVE macro?

It is not critical if we encounter error at JNI function call
because
we cannot set native thread name only.
So I think that we do not need to leave from launcher process.


I agree we do not need to abort if an exception occurs (and in
fact
I don't think an exception is even possible from this code),
but we
should ensure any pending exception is cleared before any
futher JNI
calls might be made. Note that NULL_CHECK is already used
extensively throughout the launcher code - so if this usage is
wrong
then it is all wrong! More on this code below ...


Isn’t there a risk that:

(*env)->CallVoidMethod(env, currentThread, setNativeNameID,
+   nameString, JNI_TRUE);

will raise an exception?

In the least, shouldn’t we clear any possible pending
exceptions at
the beginning of “SetNativeThreadName“, as you say, but also at
the
very end?


I was actually saying at the end, after the call (even though I
don't
think any exceptions are possible - except for "async"
exceptions of
course). We shouldn't be able to get to the call with any exception
pending as in that case the JNI calls will be returning NULL and we
will exit - again this pattern is used extensively in the launcher.

David



cheers

Re: RFR: JDK-8152690: main thread does not have native thread name

[David Holmes]

On 21/04/2016 12:59 AM, Yasumasa Suenaga wrote:

Hi Kumar,


Isn't there a management API or something to enumerate the threads ?


Not that shows native thread names.

Existing launcher tests should of course be unaffected by this change.


On Linux, user apps can get native thread name through
pthread_getname_np(3).
However, this function is not called in hotspot and jdk.

So I think it is difficult to get native thread name in all platforms.



Well if it is hard then the jbs must be labelled so, noreg-hard.


I agree to label noreg-hard.


I concur.

Thanks,
David



Thanks,

Yasumasa


On 2016/04/20 23:26, Kumar Srinivasan wrote:


On 4/20/2016 6:06 AM, David Holmes wrote:

On 20/04/2016 10:58 PM, Kumar Srinivasan wrote:


Hello,

One more thing, what about a launcher regression test ?


What kind of regression test? I've manually visually inspected the
desired behaviour but a test for it is very problematic. AFAIK there
are no tests for setting the native thread name.


Isn't there a management API or something to enumerate the threads ?
I am more worried about some seemingly innocuous launcher change
causing a regression.

Well if it is hard then the jbs must be labelled so, noreg-hard.

Kumar



David


Thanks
Kumar



On 4/19/2016 1:32 PM, David Holmes wrote:

On 20/04/2016 3:00 AM, Kumar Srinivasan wrote:

Hi David,

  493 /* Set native thread name. */
  494 SetNativeThreadName(env, "main");
  495
  496 /* Invoke main method. */
  497 (*env)->CallStaticVoidMethod(env, mainClass, mainID,
mainArgs);

Supposing an exception is thrown in 494 (a very remote case),
will we not re-enter the VM in 497 with an exception ?


Yes as I said:


1712 NULL_CHECK(nameString = (*env)->NewStringUTF(env, name));
1713 (*env)->CallVoidMethod(env, currentThread, setNativeNameID,
1714nameString, JNI_TRUE);

As above NULL_CHECK is fine here, but we should check for and clear
any pending exception after CallVoidMethod.



Agreed.

Kumar


Thanks,
David


Kumar


On 19/04/2016 10:54 PM, Gerard Ziemski wrote:



On Apr 19, 2016, at 12:04 AM, David Holmes

wrote:

On 19/04/2016 9:28 AM, Yasumasa Suenaga wrote:

Hi Gerard,

2016/04/19 3:14 "Gerard Ziemski" >:


hi Yasumasa,

Nice work. I have 2 questions:


File: java.c

#1 Shouldn’t we be checking for
“(*env)->ExceptionOccurred(env)”

after every single JNI call? In this example instead of
NULL_CHECK,
should we be using CHECK_EXCEPTION_NULL_LEAVE macro?

It is not critical if we encounter error at JNI function call
because
we cannot set native thread name only.
So I think that we do not need to leave from launcher process.


I agree we do not need to abort if an exception occurs (and in
fact
I don't think an exception is even possible from this code),
but we
should ensure any pending exception is cleared before any
futher JNI
calls might be made. Note that NULL_CHECK is already used
extensively throughout the launcher code - so if this usage is
wrong
then it is all wrong! More on this code below ...


Isn’t there a risk that:

(*env)->CallVoidMethod(env, currentThread, setNativeNameID,
+   nameString, JNI_TRUE);

will raise an exception?

In the least, shouldn’t we clear any possible pending
exceptions at
the beginning of “SetNativeThreadName“, as you say, but also at
the
very end?


I was actually saying at the end, after the call (even though I
don't
think any exceptions are possible - except for "async"
exceptions of
course). We shouldn't be able to get to the call with any exception
pending as in that case the JNI calls will be returning NULL and we
will exit - again this pattern is used extensively in the launcher.

David



cheers

Re: RFR: JDK-8152690: main thread does not have native thread name

[Yasumasa Suenaga]

Hi Kumar,


Isn't there a management API or something to enumerate the threads ?


On Linux, user apps can get native thread name through pthread_getname_np(3).
However, this function is not called in hotspot and jdk.

So I think it is difficult to get native thread name in all platforms.



Well if it is hard then the jbs must be labelled so, noreg-hard.


I agree to label noreg-hard.


Thanks,

Yasumasa


On 2016/04/20 23:26, Kumar Srinivasan wrote:


On 4/20/2016 6:06 AM, David Holmes wrote:

On 20/04/2016 10:58 PM, Kumar Srinivasan wrote:


Hello,

One more thing, what about a launcher regression test ?


What kind of regression test? I've manually visually inspected the desired 
behaviour but a test for it is very problematic. AFAIK there are no tests for 
setting the native thread name.


Isn't there a management API or something to enumerate the threads ?
I am more worried about some seemingly innocuous launcher change
causing a regression.

Well if it is hard then the jbs must be labelled so, noreg-hard.

Kumar



David


Thanks
Kumar



On 4/19/2016 1:32 PM, David Holmes wrote:

On 20/04/2016 3:00 AM, Kumar Srinivasan wrote:

Hi David,

  493 /* Set native thread name. */
  494 SetNativeThreadName(env, "main");
  495
  496 /* Invoke main method. */
  497 (*env)->CallStaticVoidMethod(env, mainClass, mainID,
mainArgs);

Supposing an exception is thrown in 494 (a very remote case),
will we not re-enter the VM in 497 with an exception ?


Yes as I said:


1712 NULL_CHECK(nameString = (*env)->NewStringUTF(env, name));
1713 (*env)->CallVoidMethod(env, currentThread, setNativeNameID,
1714nameString, JNI_TRUE);

As above NULL_CHECK is fine here, but we should check for and clear
any pending exception after CallVoidMethod.



Agreed.

Kumar


Thanks,
David


Kumar


On 19/04/2016 10:54 PM, Gerard Ziemski wrote:



On Apr 19, 2016, at 12:04 AM, David Holmes 
wrote:

On 19/04/2016 9:28 AM, Yasumasa Suenaga wrote:

Hi Gerard,

2016/04/19 3:14 "Gerard Ziemski" >:


hi Yasumasa,

Nice work. I have 2 questions:


File: java.c

#1 Shouldn’t we be checking for “(*env)->ExceptionOccurred(env)”

after every single JNI call? In this example instead of NULL_CHECK,
should we be using CHECK_EXCEPTION_NULL_LEAVE macro?

It is not critical if we encounter error at JNI function call
because
we cannot set native thread name only.
So I think that we do not need to leave from launcher process.


I agree we do not need to abort if an exception occurs (and in fact
I don't think an exception is even possible from this code), but we
should ensure any pending exception is cleared before any futher JNI
calls might be made. Note that NULL_CHECK is already used
extensively throughout the launcher code - so if this usage is wrong
then it is all wrong! More on this code below ...


Isn’t there a risk that:

(*env)->CallVoidMethod(env, currentThread, setNativeNameID,
+   nameString, JNI_TRUE);

will raise an exception?

In the least, shouldn’t we clear any possible pending exceptions at
the beginning of “SetNativeThreadName“, as you say, but also at the
very end?


I was actually saying at the end, after the call (even though I don't
think any exceptions are possible - except for "async" exceptions of
course). We shouldn't be able to get to the call with any exception
pending as in that case the JNI calls will be returning NULL and we
will exit - again this pattern is used extensively in the launcher.

David



cheers

Re: RFR: JDK-8152690: main thread does not have native thread name

[Kumar Srinivasan]

On 4/20/2016 6:06 AM, David Holmes wrote:

On 20/04/2016 10:58 PM, Kumar Srinivasan wrote:


Hello,

One more thing, what about a launcher regression test ?


What kind of regression test? I've manually visually inspected the 
desired behaviour but a test for it is very problematic. AFAIK there 
are no tests for setting the native thread name.


Isn't there a management API or something to enumerate the threads ?
I am more worried about some seemingly innocuous launcher change
causing a regression.

Well if it is hard then the jbs must be labelled so, noreg-hard.

Kumar



David


Thanks
Kumar



On 4/19/2016 1:32 PM, David Holmes wrote:

On 20/04/2016 3:00 AM, Kumar Srinivasan wrote:

Hi David,

  493 /* Set native thread name. */
  494 SetNativeThreadName(env, "main");
  495
  496 /* Invoke main method. */
  497 (*env)->CallStaticVoidMethod(env, mainClass, mainID,
mainArgs);

Supposing an exception is thrown in 494 (a very remote case),
will we not re-enter the VM in 497 with an exception ?


Yes as I said:


1712 NULL_CHECK(nameString = (*env)->NewStringUTF(env, name));
1713 (*env)->CallVoidMethod(env, currentThread, setNativeNameID,
1714nameString, JNI_TRUE);

As above NULL_CHECK is fine here, but we should check for and clear
any pending exception after CallVoidMethod.



Agreed.

Kumar


Thanks,
David


Kumar


On 19/04/2016 10:54 PM, Gerard Ziemski wrote:


On Apr 19, 2016, at 12:04 AM, David Holmes 


wrote:

On 19/04/2016 9:28 AM, Yasumasa Suenaga wrote:

Hi Gerard,

2016/04/19 3:14 "Gerard Ziemski" >:


hi Yasumasa,

Nice work. I have 2 questions:


File: java.c

#1 Shouldn’t we be checking for “(*env)->ExceptionOccurred(env)”
after every single JNI call? In this example instead of 
NULL_CHECK,

should we be using CHECK_EXCEPTION_NULL_LEAVE macro?

It is not critical if we encounter error at JNI function call
because
we cannot set native thread name only.
So I think that we do not need to leave from launcher process.


I agree we do not need to abort if an exception occurs (and in 
fact
I don't think an exception is even possible from this code), 
but we
should ensure any pending exception is cleared before any 
futher JNI

calls might be made. Note that NULL_CHECK is already used
extensively throughout the launcher code - so if this usage is 
wrong

then it is all wrong! More on this code below ...


Isn’t there a risk that:

(*env)->CallVoidMethod(env, currentThread, setNativeNameID,
+   nameString, JNI_TRUE);

will raise an exception?

In the least, shouldn’t we clear any possible pending exceptions at
the beginning of “SetNativeThreadName“, as you say, but also at the
very end?


I was actually saying at the end, after the call (even though I 
don't

think any exceptions are possible - except for "async" exceptions of
course). We shouldn't be able to get to the call with any exception
pending as in that case the JNI calls will be returning NULL and we
will exit - again this pattern is used extensively in the launcher.

David



cheers

Re: RFR: JDK-8152690: main thread does not have native thread name

[David Holmes]

On 20/04/2016 10:58 PM, Kumar Srinivasan wrote:


Hello,

One more thing, what about a launcher regression test ?


What kind of regression test? I've manually visually inspected the 
desired behaviour but a test for it is very problematic. AFAIK there are 
no tests for setting the native thread name.


David


Thanks
Kumar



On 4/19/2016 1:32 PM, David Holmes wrote:

On 20/04/2016 3:00 AM, Kumar Srinivasan wrote:

Hi David,

  493 /* Set native thread name. */
  494 SetNativeThreadName(env, "main");
  495
  496 /* Invoke main method. */
  497 (*env)->CallStaticVoidMethod(env, mainClass, mainID,
mainArgs);

Supposing an exception is thrown in 494 (a very remote case),
will we not re-enter the VM in 497 with an exception ?


Yes as I said:


1712 NULL_CHECK(nameString = (*env)->NewStringUTF(env, name));
1713 (*env)->CallVoidMethod(env, currentThread, setNativeNameID,
1714nameString, JNI_TRUE);

As above NULL_CHECK is fine here, but we should check for and clear
any pending exception after CallVoidMethod.



Agreed.

Kumar


Thanks,
David


Kumar


On 19/04/2016 10:54 PM, Gerard Ziemski wrote:



On Apr 19, 2016, at 12:04 AM, David Holmes 
wrote:

On 19/04/2016 9:28 AM, Yasumasa Suenaga wrote:

Hi Gerard,

2016/04/19 3:14 "Gerard Ziemski" >:


hi Yasumasa,

Nice work. I have 2 questions:


File: java.c

#1 Shouldn’t we be checking for “(*env)->ExceptionOccurred(env)”

after every single JNI call? In this example instead of NULL_CHECK,
should we be using CHECK_EXCEPTION_NULL_LEAVE macro?

It is not critical if we encounter error at JNI function call
because
we cannot set native thread name only.
So I think that we do not need to leave from launcher process.


I agree we do not need to abort if an exception occurs (and in fact
I don't think an exception is even possible from this code), but we
should ensure any pending exception is cleared before any futher JNI
calls might be made. Note that NULL_CHECK is already used
extensively throughout the launcher code - so if this usage is wrong
then it is all wrong! More on this code below ...


Isn’t there a risk that:

(*env)->CallVoidMethod(env, currentThread, setNativeNameID,
+   nameString, JNI_TRUE);

will raise an exception?

In the least, shouldn’t we clear any possible pending exceptions at
the beginning of “SetNativeThreadName“, as you say, but also at the
very end?


I was actually saying at the end, after the call (even though I don't
think any exceptions are possible - except for "async" exceptions of
course). We shouldn't be able to get to the call with any exception
pending as in that case the JNI calls will be returning NULL and we
will exit - again this pattern is used extensively in the launcher.

David



cheers

Re: RFR: JDK-8152690: main thread does not have native thread name

[Kumar Srinivasan]

Hello,

One more thing, what about a launcher regression test ?

Thanks
Kumar



On 4/19/2016 1:32 PM, David Holmes wrote:

On 20/04/2016 3:00 AM, Kumar Srinivasan wrote:

Hi David,

  493 /* Set native thread name. */
  494 SetNativeThreadName(env, "main");
  495
  496 /* Invoke main method. */
  497 (*env)->CallStaticVoidMethod(env, mainClass, mainID, 
mainArgs);


Supposing an exception is thrown in 494 (a very remote case),
will we not re-enter the VM in 497 with an exception ?


Yes as I said:


1712 NULL_CHECK(nameString = (*env)->NewStringUTF(env, name));
1713 (*env)->CallVoidMethod(env, currentThread, setNativeNameID,
1714nameString, JNI_TRUE);

As above NULL_CHECK is fine here, but we should check for and clear 
any pending exception after CallVoidMethod.



Agreed.

Kumar


Thanks,
David


Kumar


On 19/04/2016 10:54 PM, Gerard Ziemski wrote:



On Apr 19, 2016, at 12:04 AM, David Holmes 
wrote:

On 19/04/2016 9:28 AM, Yasumasa Suenaga wrote:

Hi Gerard,

2016/04/19 3:14 "Gerard Ziemski" >:


hi Yasumasa,

Nice work. I have 2 questions:


File: java.c

#1 Shouldn’t we be checking for “(*env)->ExceptionOccurred(env)”

after every single JNI call? In this example instead of NULL_CHECK,
should we be using CHECK_EXCEPTION_NULL_LEAVE macro?

It is not critical if we encounter error at JNI function call  
because

we cannot set native thread name only.
So I think that we do not need to leave from launcher process.


I agree we do not need to abort if an exception occurs (and in fact
I don't think an exception is even possible from this code), but we
should ensure any pending exception is cleared before any futher JNI
calls might be made. Note that NULL_CHECK is already used
extensively throughout the launcher code - so if this usage is wrong
then it is all wrong! More on this code below ...


Isn’t there a risk that:

(*env)->CallVoidMethod(env, currentThread, setNativeNameID,
+   nameString, JNI_TRUE);

will raise an exception?

In the least, shouldn’t we clear any possible pending exceptions at
the beginning of “SetNativeThreadName“, as you say, but also at the
very end?


I was actually saying at the end, after the call (even though I don't
think any exceptions are possible - except for "async" exceptions of
course). We shouldn't be able to get to the call with any exception
pending as in that case the JNI calls will be returning NULL and we
will exit - again this pattern is used extensively in the launcher.

David



cheers

Re: RFR: JDK-8152690: main thread does not have native thread name

[Kumar Srinivasan]

On 4/19/2016 1:32 PM, David Holmes wrote:

On 20/04/2016 3:00 AM, Kumar Srinivasan wrote:

Hi David,

  493 /* Set native thread name. */
  494 SetNativeThreadName(env, "main");
  495
  496 /* Invoke main method. */
  497 (*env)->CallStaticVoidMethod(env, mainClass, mainID, 
mainArgs);


Supposing an exception is thrown in 494 (a very remote case),
will we not re-enter the VM in 497 with an exception ?


Yes as I said:


1712 NULL_CHECK(nameString = (*env)->NewStringUTF(env, name));
1713 (*env)->CallVoidMethod(env, currentThread, setNativeNameID,
1714nameString, JNI_TRUE);

As above NULL_CHECK is fine here, but we should check for and clear 
any pending exception after CallVoidMethod.



Agreed.

Kumar


Thanks,
David


Kumar


On 19/04/2016 10:54 PM, Gerard Ziemski wrote:



On Apr 19, 2016, at 12:04 AM, David Holmes 
wrote:

On 19/04/2016 9:28 AM, Yasumasa Suenaga wrote:

Hi Gerard,

2016/04/19 3:14 "Gerard Ziemski" >:


hi Yasumasa,

Nice work. I have 2 questions:


File: java.c

#1 Shouldn’t we be checking for “(*env)->ExceptionOccurred(env)”

after every single JNI call? In this example instead of NULL_CHECK,
should we be using CHECK_EXCEPTION_NULL_LEAVE macro?

It is not critical if we encounter error at JNI function call  
because

we cannot set native thread name only.
So I think that we do not need to leave from launcher process.


I agree we do not need to abort if an exception occurs (and in fact
I don't think an exception is even possible from this code), but we
should ensure any pending exception is cleared before any futher JNI
calls might be made. Note that NULL_CHECK is already used
extensively throughout the launcher code - so if this usage is wrong
then it is all wrong! More on this code below ...


Isn’t there a risk that:

(*env)->CallVoidMethod(env, currentThread, setNativeNameID,
+   nameString, JNI_TRUE);

will raise an exception?

In the least, shouldn’t we clear any possible pending exceptions at
the beginning of “SetNativeThreadName“, as you say, but also at the
very end?


I was actually saying at the end, after the call (even though I don't
think any exceptions are possible - except for "async" exceptions of
course). We shouldn't be able to get to the call with any exception
pending as in that case the JNI calls will be returning NULL and we
will exit - again this pattern is used extensively in the launcher.

David



cheers

Re: RFR: JDK-8152690: main thread does not have native thread name

[David Holmes]

On 20/04/2016 3:00 AM, Kumar Srinivasan wrote:

Hi David,

  493 /* Set native thread name. */
  494 SetNativeThreadName(env, "main");
  495
  496 /* Invoke main method. */
  497 (*env)->CallStaticVoidMethod(env, mainClass, mainID, mainArgs);

Supposing an exception is thrown in 494 (a very remote case),
will we not re-enter the VM in 497 with an exception ?


Yes as I said:


1712 NULL_CHECK(nameString = (*env)->NewStringUTF(env, name));
1713 (*env)->CallVoidMethod(env, currentThread, setNativeNameID,
1714nameString, JNI_TRUE);

As above NULL_CHECK is fine here, but we should check for and clear any pending 
exception after CallVoidMethod.


Thanks,
David


Kumar


On 19/04/2016 10:54 PM, Gerard Ziemski wrote:



On Apr 19, 2016, at 12:04 AM, David Holmes 
wrote:

On 19/04/2016 9:28 AM, Yasumasa Suenaga wrote:

Hi Gerard,

2016/04/19 3:14 "Gerard Ziemski" >:


hi Yasumasa,

Nice work. I have 2 questions:


File: java.c

#1 Shouldn’t we be checking for “(*env)->ExceptionOccurred(env)”

after every single JNI call? In this example instead of NULL_CHECK,
should we be using CHECK_EXCEPTION_NULL_LEAVE macro?

It is not critical if we encounter error at JNI function call  because
we cannot set native thread name only.
So I think that we do not need to leave from launcher process.


I agree we do not need to abort if an exception occurs (and in fact
I don't think an exception is even possible from this code), but we
should ensure any pending exception is cleared before any futher JNI
calls might be made. Note that NULL_CHECK is already used
extensively throughout the launcher code - so if this usage is wrong
then it is all wrong! More on this code below ...


Isn’t there a risk that:

(*env)->CallVoidMethod(env, currentThread, setNativeNameID,
+   nameString, JNI_TRUE);

will raise an exception?

In the least, shouldn’t we clear any possible pending exceptions at
the beginning of “SetNativeThreadName“, as you say, but also at the
very end?


I was actually saying at the end, after the call (even though I don't
think any exceptions are possible - except for "async" exceptions of
course). We shouldn't be able to get to the call with any exception
pending as in that case the JNI calls will be returning NULL and we
will exit - again this pattern is used extensively in the launcher.

David



cheers

Re: RFR: JDK-8152690: main thread does not have native thread name

[Kumar Srinivasan]

Hi David,

 493 /* Set native thread name. */
 494 SetNativeThreadName(env, "main");
 495
 496 /* Invoke main method. */
 497 (*env)->CallStaticVoidMethod(env, mainClass, mainID, mainArgs);

Supposing an exception is thrown in 494 (a very remote case),
will we not re-enter the VM in 497 with an exception ?

Kumar


On 19/04/2016 10:54 PM, Gerard Ziemski wrote:


On Apr 19, 2016, at 12:04 AM, David Holmes  
wrote:


On 19/04/2016 9:28 AM, Yasumasa Suenaga wrote:

Hi Gerard,

2016/04/19 3:14 "Gerard Ziemski" >:


hi Yasumasa,

Nice work. I have 2 questions:


File: java.c

#1 Shouldn’t we be checking for “(*env)->ExceptionOccurred(env)”

after every single JNI call? In this example instead of NULL_CHECK,
should we be using CHECK_EXCEPTION_NULL_LEAVE macro?

It is not critical if we encounter error at JNI function call  because
we cannot set native thread name only.
So I think that we do not need to leave from launcher process.


I agree we do not need to abort if an exception occurs (and in fact 
I don't think an exception is even possible from this code), but we 
should ensure any pending exception is cleared before any futher JNI 
calls might be made. Note that NULL_CHECK is already used 
extensively throughout the launcher code - so if this usage is wrong 
then it is all wrong! More on this code below ...


Isn’t there a risk that:

(*env)->CallVoidMethod(env, currentThread, setNativeNameID,
+   nameString, JNI_TRUE);

will raise an exception?

In the least, shouldn’t we clear any possible pending exceptions at 
the beginning of “SetNativeThreadName“, as you say, but also at the 
very end?


I was actually saying at the end, after the call (even though I don't 
think any exceptions are possible - except for "async" exceptions of 
course). We shouldn't be able to get to the call with any exception 
pending as in that case the JNI calls will be returning NULL and we 
will exit - again this pattern is used extensively in the launcher.


David



cheers

Re: RFR: JDK-8152690: main thread does not have native thread name

[David Holmes]

On 19/04/2016 10:54 PM, Gerard Ziemski wrote:



On Apr 19, 2016, at 12:04 AM, David Holmes  wrote:

On 19/04/2016 9:28 AM, Yasumasa Suenaga wrote:

Hi Gerard,

2016/04/19 3:14 "Gerard Ziemski" >:


hi Yasumasa,

Nice work. I have 2 questions:


File: java.c

#1 Shouldn’t we be checking for “(*env)->ExceptionOccurred(env)”

after every single JNI call? In this example instead of NULL_CHECK,
should we be using CHECK_EXCEPTION_NULL_LEAVE macro?

It is not critical if we encounter error at JNI function call  because
we cannot set native thread name only.
So I think that we do not need to leave from launcher process.


I agree we do not need to abort if an exception occurs (and in fact I don't 
think an exception is even possible from this code), but we should ensure any 
pending exception is cleared before any futher JNI calls might be made. Note 
that NULL_CHECK is already used extensively throughout the launcher code - so 
if this usage is wrong then it is all wrong! More on this code below ...


Isn’t there a risk that:

(*env)->CallVoidMethod(env, currentThread, setNativeNameID,
+   nameString, JNI_TRUE);

will raise an exception?

In the least, shouldn’t we clear any possible pending exceptions at the 
beginning of “SetNativeThreadName“, as you say, but also at the very end?


I was actually saying at the end, after the call (even though I don't 
think any exceptions are possible - except for "async" exceptions of 
course). We shouldn't be able to get to the call with any exception 
pending as in that case the JNI calls will be returning NULL and we will 
exit - again this pattern is used extensively in the launcher.


David



cheers

Re: RFR: JDK-8152690: main thread does not have native thread name

[Gerard Ziemski]

> On Apr 19, 2016, at 12:04 AM, David Holmes  wrote:
> 
> On 19/04/2016 9:28 AM, Yasumasa Suenaga wrote:
>> Hi Gerard,
>> 
>> 2016/04/19 3:14 "Gerard Ziemski" > >:
>> >
>> > hi Yasumasa,
>> >
>> > Nice work. I have 2 questions:
>> >
>> > 
>> > File: java.c
>> >
>> > #1 Shouldn’t we be checking for “(*env)->ExceptionOccurred(env)”
>> after every single JNI call? In this example instead of NULL_CHECK,
>> should we be using CHECK_EXCEPTION_NULL_LEAVE macro?
>> 
>> It is not critical if we encounter error at JNI function call  because
>> we cannot set native thread name only.
>> So I think that we do not need to leave from launcher process.
> 
> I agree we do not need to abort if an exception occurs (and in fact I don't 
> think an exception is even possible from this code), but we should ensure any 
> pending exception is cleared before any futher JNI calls might be made. Note 
> that NULL_CHECK is already used extensively throughout the launcher code - so 
> if this usage is wrong then it is all wrong! More on this code below ...

Isn’t there a risk that:

(*env)->CallVoidMethod(env, currentThread, setNativeNameID,
+   nameString, JNI_TRUE);

will raise an exception?

In the least, shouldn’t we clear any possible pending exceptions at the 
beginning of “SetNativeThreadName“, as you say, but also at the very end?


cheers

Re: RFR: JDK-8152690: main thread does not have native thread name

[David Holmes]

Hi Yasumasa,

Thanks for persevering with this to get it into the current form. Sorry 
I haven't been able to do a detailed review until now.


On 19/04/2016 9:28 AM, Yasumasa Suenaga wrote:

Hi Gerard,

2016/04/19 3:14 "Gerard Ziemski" >:
 >
 > hi Yasumasa,
 >
 > Nice work. I have 2 questions:
 >
 > 
 > File: java.c
 >
 > #1 Shouldn’t we be checking for “(*env)->ExceptionOccurred(env)”
after every single JNI call? In this example instead of NULL_CHECK,
should we be using CHECK_EXCEPTION_NULL_LEAVE macro?

It is not critical if we encounter error at JNI function call  because
we cannot set native thread name only.
So I think that we do not need to leave from launcher process.


I agree we do not need to abort if an exception occurs (and in fact I 
don't think an exception is even possible from this code), but we should 
ensure any pending exception is cleared before any futher JNI calls 
might be made. Note that NULL_CHECK is already used extensively 
throughout the launcher code - so if this usage is wrong then it is all 
wrong! More on this code below ...


Other comments:

hotspot/src/share/vm/prims/jvm.cpp

Please add a comment to the method now that you removed the internal 
comment:


// Sets the native thread name for a JavaThread. If specifically
// requested JNI-attached threads can also have their native name set;
// otherwise we do not modify JNI-attached threads as it may interfere
// with the application that created them.

---

jdk/src/java.base/share/classes/java/lang/Thread.java

Please add the following comments:

+// Don't modify JNI-attached threads
 setNativeName(name, false);

+ // May be called directly via JNI or reflection (when permitted) to
+ // allow JNI-attached threads to set their native name
 private native void setNativeName(String name, boolean 
allowAttachedThread);


---

jd/src/java.base/share/native/libjli/java.c

328 #define LEAVE() \
329 SetNativeThreadName(env, "DestroyJavaVM"); \

I was going to suggest this be set later, but realized we have to be 
attached to do this and that happens inside DestroyJavaVM. :)


+ /* Set native thread name. */
+ SetNativeThreadName(env, "main");

The comment is redundant given the name of the method. That aside I'm 
not sure why you do this so late in the process, I would have done it 
immediately after here:


 386 if (!InitializeJVM(, , )) {
 387 JLI_ReportErrorMessage(JVM_ERROR1);
 388 exit(1);
 389 }
 +   SetNativeThreadName(env, "main");


+ /**
+  * Set native thread name as possible.
+  */

Other than the as->if change I'm unclear where the "possible" bit comes 
into play - why would it not be possible?


1705 NULL_CHECK(cls = FindBootStrapClass(env, "java/lang/Thread"));
1706 NULL_CHECK(currentThreadID = (*env)->GetStaticMethodID(env, cls,
1707  "currentThread", 
"()Ljava/lang/Thread;"));

1708 NULL_CHECK(currentThread = (*env)->CallStaticObjectMethod(env, cls,
1709 
currentThreadID));

1710 NULL_CHECK(setNativeNameID = (*env)->GetMethodID(env, cls,
1711 "setNativeName", 
"(Ljava/lang/String;Z)V"));

1712 NULL_CHECK(nameString = (*env)->NewStringUTF(env, name));
1713 (*env)->CallVoidMethod(env, currentThread, setNativeNameID,
1714nameString, JNI_TRUE);

As above NULL_CHECK is fine here, but we should check for and clear any 
pending exception after CallVoidMethod.


One thing I dislike about the current structure is that we have to go 
from char* to java.lang.String to call setNativeName which then calls 
JVM_SetNativeThreadName which converts the j.l.String back to a char* ! 
Overall I wonder about the affect on startup cost. But if there is an 
issue we can revisit this.


Thanks,
David
-



 > #2 Should the comment for “SetNativeThreadName” be “Set native thread
name if possible.” not "Set native thread name as possible.”?

Sorry for my bad English :-)

Thanks,

Yasumasa

 > cheers
 >
 > > On Apr 16, 2016, at 4:29 AM, Yasumasa Suenaga > wrote:
 > >
 > > Hi David,
 > >
 > > I uploaded new webrev:
 > >
 > > - hotspot:
 > > http://cr.openjdk.java.net/~ysuenaga/JDK-8152690/webrev.03/hotspot/
 > >
 > > - jdk:
 > > http://cr.openjdk.java.net/~ysuenaga/JDK-8152690/webrev.03/jdk/
 > >
 > >
 > >> it won't work unless you change the semantics of setName so I'm
not sure what you were thinking here. To take advantage of an arg taking
JVM_SetNativThreadName you would need to call it directly as no Java
code will call it . ???
 > >
 > > I added a flag for setting native thread name to JNI-attached thread.
 > > This change can set native thread name if main thread changes to
JNI-attached thread.
 > >
 > >
 > > Thanks,
 > >
 > > Yasumasa
 > >
 > >
 > > On 2016/04/16 16:11, David Holmes wrote:
 > >> On 16/04/2016 3:27 PM, Yasumasa 

Re: RFR: JDK-8152690: main thread does not have native thread name

[Yasumasa Suenaga]

Hi Gerard,

2016/04/19 3:14 "Gerard Ziemski" :
>
> hi Yasumasa,
>
> Nice work. I have 2 questions:
>
> 
> File: java.c
>
> #1 Shouldn’t we be checking for “(*env)->ExceptionOccurred(env)” after
every single JNI call? In this example instead of NULL_CHECK, should we be
using CHECK_EXCEPTION_NULL_LEAVE macro?

It is not critical if we encounter error at JNI function call  because we
cannot set native thread name only.
So I think that we do not need to leave from launcher process.

> #2 Should the comment for “SetNativeThreadName” be “Set native thread
name if possible.” not "Set native thread name as possible.”?

Sorry for my bad English :-)

Thanks,

Yasumasa

> cheers
>
> > On Apr 16, 2016, at 4:29 AM, Yasumasa Suenaga 
wrote:
> >
> > Hi David,
> >
> > I uploaded new webrev:
> >
> > - hotspot:
> >http://cr.openjdk.java.net/~ysuenaga/JDK-8152690/webrev.03/hotspot/
> >
> > - jdk:
> >http://cr.openjdk.java.net/~ysuenaga/JDK-8152690/webrev.03/jdk/
> >
> >
> >> it won't work unless you change the semantics of setName so I'm not
sure what you were thinking here. To take advantage of an arg taking
JVM_SetNativThreadName you would need to call it directly as no Java code
will call it . ???
> >
> > I added a flag for setting native thread name to JNI-attached thread.
> > This change can set native thread name if main thread changes to
JNI-attached thread.
> >
> >
> > Thanks,
> >
> > Yasumasa
> >
> >
> > On 2016/04/16 16:11, David Holmes wrote:
> >> On 16/04/2016 3:27 PM, Yasumasa Suenaga wrote:
> >>> Hi David,
> >>>
>  That change in behaviour may be a problem, it could be considered a
>  regression that setName stops setting the native thread main, even
>  though we never really intended it to work in the first place. :(
Such
>  a change needs to go through CCC.
> >>>
> >>> I understood.
> >>> Can I send CCC request?
> >>> (I'm jdk 9 commiter, but I'm not employee at Oracle.)
> >>
> >> Sorry you can't file a CCC request, you would need a sponsor for that.
But at this stage I don't think I agree with the proposed change because of
the change in behaviour - there's no way to restore the "broken" behaviour.
> >>
> >>> I want to continue to discuss about it on JDK-8154331 [1].
> >>
> >> Okay we can do that.
> >>
> >>>
>  Further, we expect the launcher to use the supported JNI interface
(as
>  other processes would), not the internal JVM interface that exists
for
>  the JDK sources to communicate with the JVM.
> >>>
> >>> I think that we do not use JVM interface if we add new method in
> >>> LauncherHelper as below:
> >>> 
> >>> diff -r f02139a1ac84
> >>> src/java.base/share/classes/sun/launcher/LauncherHelper.java
> >>> --- a/src/java.base/share/classes/sun/launcher/LauncherHelper.java
> >>> Wed Apr 13 14:19:30 2016 +
> >>> +++ b/src/java.base/share/classes/sun/launcher/LauncherHelper.java
> >>> Sat Apr 16 11:25:53 2016 +0900
> >>> @@ -960,4 +960,8 @@
> >>>  else
> >>>  return md.toNameAndVersion() + " (" + loc + ")";
> >>>  }
> >>> +
> >>> +static void setNativeThreadName(String name) {
> >>> +Thread.currentThread().setName(name);
> >>> +}
> >>
> >> You could also make that call via JNI directly, so not sure the helper
adds much here. But it won't work unless you change the semantics of
setName so I'm not sure what you were thinking here. To take advantage of
an arg taking JVM_SetNativThreadName you would need to call it directly as
no Java code will call it . ???
> >>
> >> David
> >> -
> >>
> >>>  }
> >>> diff -r f02139a1ac84 src/java.base/share/native/libjli/java.c
> >>> --- a/src/java.base/share/native/libjli/java.cWed Apr 13 14:19:30
> >>> 2016 +
> >>> +++ b/src/java.base/share/native/libjli/java.cSat Apr 16 11:25:53
> >>> 2016 +0900
> >>> @@ -125,6 +125,7 @@
> >>>  static void PrintUsage(JNIEnv* env, jboolean doXUsage);
> >>>  static void ShowSettings(JNIEnv* env, char *optString);
> >>>  static void ListModules(JNIEnv* env, char *optString);
> >>> +static void SetNativeThreadName(JNIEnv* env, char *name);
> >>>
> >>>  static void SetPaths(int argc, char **argv);
> >>>
> >>> @@ -325,6 +326,7 @@
> >>>   * mainThread.isAlive() to work as expected.
> >>>   */
> >>>  #define LEAVE() \
> >>> +SetNativeThreadName(env, "DestroyJavaVM"); \
> >>>  do { \
> >>>  if ((*vm)->DetachCurrentThread(vm) != JNI_OK) { \
> >>>  JLI_ReportErrorMessage(JVM_ERROR2); \
> >>> @@ -488,6 +490,9 @@
> >>>  mainArgs = CreateApplicationArgs(env, argv, argc);
> >>>  CHECK_EXCEPTION_NULL_LEAVE(mainArgs);
> >>>
> >>> +/* Set native thread name. */
> >>> +SetNativeThreadName(env, "main");
> >>> +
> >>>  /* Invoke main method. */
> >>>  (*env)->CallStaticVoidMethod(env, mainClass, mainID, mainArgs);
> >>>
> >>> @@ -1686,6 +1691,22 @@
> >>>   joptString);
> >>>  }
> >>>
> >>> +/**
> >>> + * Set 

Re: RFR: JDK-8152690: main thread does not have native thread name

[Kumar Srinivasan]

Oops on my part, Gerard is right. We need to make SetNativeThreadName exit,
if there is an error, otherwise it would enter the VM with an exception 
at the

call site.

So I think CHECK_EXCEPTION_NULL_LEAVE is the right thing to do, or we
need to have a check and exit at the call site.

Kumar


hi Yasumasa,

Nice work. I have 2 questions:


File: java.c

#1 Shouldn’t we be checking for “(*env)->ExceptionOccurred(env)” after every 
single JNI call? In this example instead of NULL_CHECK, should we be using 
CHECK_EXCEPTION_NULL_LEAVE macro?

#2 Should the comment for “SetNativeThreadName” be “Set native thread name if 
possible.” not "Set native thread name as possible.”?


cheers


On Apr 16, 2016, at 4:29 AM, Yasumasa Suenaga  wrote:

Hi David,

I uploaded new webrev:

- hotspot:
http://cr.openjdk.java.net/~ysuenaga/JDK-8152690/webrev.03/hotspot/

- jdk:
http://cr.openjdk.java.net/~ysuenaga/JDK-8152690/webrev.03/jdk/



it won't work unless you change the semantics of setName so I'm not sure what 
you were thinking here. To take advantage of an arg taking 
JVM_SetNativThreadName you would need to call it directly as no Java code will 
call it . ???

I added a flag for setting native thread name to JNI-attached thread.
This change can set native thread name if main thread changes to JNI-attached 
thread.


Thanks,

Yasumasa


On 2016/04/16 16:11, David Holmes wrote:

On 16/04/2016 3:27 PM, Yasumasa Suenaga wrote:

Hi David,


That change in behaviour may be a problem, it could be considered a
regression that setName stops setting the native thread main, even
though we never really intended it to work in the first place. :( Such
a change needs to go through CCC.

I understood.
Can I send CCC request?
(I'm jdk 9 commiter, but I'm not employee at Oracle.)

Sorry you can't file a CCC request, you would need a sponsor for that. But at this stage 
I don't think I agree with the proposed change because of the change in behaviour - 
there's no way to restore the "broken" behaviour.


I want to continue to discuss about it on JDK-8154331 [1].

Okay we can do that.


Further, we expect the launcher to use the supported JNI interface (as
other processes would), not the internal JVM interface that exists for
the JDK sources to communicate with the JVM.

I think that we do not use JVM interface if we add new method in
LauncherHelper as below:

diff -r f02139a1ac84
src/java.base/share/classes/sun/launcher/LauncherHelper.java
--- a/src/java.base/share/classes/sun/launcher/LauncherHelper.java
Wed Apr 13 14:19:30 2016 +
+++ b/src/java.base/share/classes/sun/launcher/LauncherHelper.java
Sat Apr 16 11:25:53 2016 +0900
@@ -960,4 +960,8 @@
  else
  return md.toNameAndVersion() + " (" + loc + ")";
  }
+
+static void setNativeThreadName(String name) {
+Thread.currentThread().setName(name);
+}

You could also make that call via JNI directly, so not sure the helper adds 
much here. But it won't work unless you change the semantics of setName so I'm 
not sure what you were thinking here. To take advantage of an arg taking 
JVM_SetNativThreadName you would need to call it directly as no Java code will 
call it . ???

David
-


  }
diff -r f02139a1ac84 src/java.base/share/native/libjli/java.c
--- a/src/java.base/share/native/libjli/java.cWed Apr 13 14:19:30
2016 +
+++ b/src/java.base/share/native/libjli/java.cSat Apr 16 11:25:53
2016 +0900
@@ -125,6 +125,7 @@
  static void PrintUsage(JNIEnv* env, jboolean doXUsage);
  static void ShowSettings(JNIEnv* env, char *optString);
  static void ListModules(JNIEnv* env, char *optString);
+static void SetNativeThreadName(JNIEnv* env, char *name);

  static void SetPaths(int argc, char **argv);

@@ -325,6 +326,7 @@
   * mainThread.isAlive() to work as expected.
   */
  #define LEAVE() \
+SetNativeThreadName(env, "DestroyJavaVM"); \
  do { \
  if ((*vm)->DetachCurrentThread(vm) != JNI_OK) { \
  JLI_ReportErrorMessage(JVM_ERROR2); \
@@ -488,6 +490,9 @@
  mainArgs = CreateApplicationArgs(env, argv, argc);
  CHECK_EXCEPTION_NULL_LEAVE(mainArgs);

+/* Set native thread name. */
+SetNativeThreadName(env, "main");
+
  /* Invoke main method. */
  (*env)->CallStaticVoidMethod(env, mainClass, mainID, mainArgs);

@@ -1686,6 +1691,22 @@
   joptString);
  }

+/**
+ * Set native thread name as possible.
+ */
+static void
+SetNativeThreadName(JNIEnv *env, char *name)
+{
+jmethodID setNativeThreadNameID;
+jstring nameString;
+jclass cls = GetLauncherHelperClass(env);
+NULL_CHECK(cls);
+NULL_CHECK(setNativeThreadNameID = (*env)->GetStaticMethodID(env, cls,
+"setNativeThreadName", "(Ljava/lang/String;)V"));
+NULL_CHECK(nameString = (*env)->NewStringUTF(env, name));
+(*env)->CallStaticVoidMethod(env, cls, setNativeThreadNameID,
nameString);
+}
+
  /*
   * Prints default usage or 

Re: RFR: JDK-8152690: main thread does not have native thread name

[Gerard Ziemski]

hi Yasumasa,

Nice work. I have 2 questions:


File: java.c

#1 Shouldn’t we be checking for “(*env)->ExceptionOccurred(env)” after every 
single JNI call? In this example instead of NULL_CHECK, should we be using 
CHECK_EXCEPTION_NULL_LEAVE macro?

#2 Should the comment for “SetNativeThreadName” be “Set native thread name if 
possible.” not "Set native thread name as possible.”?


cheers

> On Apr 16, 2016, at 4:29 AM, Yasumasa Suenaga  wrote:
> 
> Hi David,
> 
> I uploaded new webrev:
> 
> - hotspot:
>http://cr.openjdk.java.net/~ysuenaga/JDK-8152690/webrev.03/hotspot/
> 
> - jdk:
>http://cr.openjdk.java.net/~ysuenaga/JDK-8152690/webrev.03/jdk/
> 
> 
>> it won't work unless you change the semantics of setName so I'm not sure 
>> what you were thinking here. To take advantage of an arg taking 
>> JVM_SetNativThreadName you would need to call it directly as no Java code 
>> will call it . ???
> 
> I added a flag for setting native thread name to JNI-attached thread.
> This change can set native thread name if main thread changes to JNI-attached 
> thread.
> 
> 
> Thanks,
> 
> Yasumasa
> 
> 
> On 2016/04/16 16:11, David Holmes wrote:
>> On 16/04/2016 3:27 PM, Yasumasa Suenaga wrote:
>>> Hi David,
>>> 
 That change in behaviour may be a problem, it could be considered a
 regression that setName stops setting the native thread main, even
 though we never really intended it to work in the first place. :( Such
 a change needs to go through CCC.
>>> 
>>> I understood.
>>> Can I send CCC request?
>>> (I'm jdk 9 commiter, but I'm not employee at Oracle.)
>> 
>> Sorry you can't file a CCC request, you would need a sponsor for that. But 
>> at this stage I don't think I agree with the proposed change because of the 
>> change in behaviour - there's no way to restore the "broken" behaviour.
>> 
>>> I want to continue to discuss about it on JDK-8154331 [1].
>> 
>> Okay we can do that.
>> 
>>> 
 Further, we expect the launcher to use the supported JNI interface (as
 other processes would), not the internal JVM interface that exists for
 the JDK sources to communicate with the JVM.
>>> 
>>> I think that we do not use JVM interface if we add new method in
>>> LauncherHelper as below:
>>> 
>>> diff -r f02139a1ac84
>>> src/java.base/share/classes/sun/launcher/LauncherHelper.java
>>> --- a/src/java.base/share/classes/sun/launcher/LauncherHelper.java
>>> Wed Apr 13 14:19:30 2016 +
>>> +++ b/src/java.base/share/classes/sun/launcher/LauncherHelper.java
>>> Sat Apr 16 11:25:53 2016 +0900
>>> @@ -960,4 +960,8 @@
>>>  else
>>>  return md.toNameAndVersion() + " (" + loc + ")";
>>>  }
>>> +
>>> +static void setNativeThreadName(String name) {
>>> +Thread.currentThread().setName(name);
>>> +}
>> 
>> You could also make that call via JNI directly, so not sure the helper adds 
>> much here. But it won't work unless you change the semantics of setName so 
>> I'm not sure what you were thinking here. To take advantage of an arg taking 
>> JVM_SetNativThreadName you would need to call it directly as no Java code 
>> will call it . ???
>> 
>> David
>> -
>> 
>>>  }
>>> diff -r f02139a1ac84 src/java.base/share/native/libjli/java.c
>>> --- a/src/java.base/share/native/libjli/java.cWed Apr 13 14:19:30
>>> 2016 +
>>> +++ b/src/java.base/share/native/libjli/java.cSat Apr 16 11:25:53
>>> 2016 +0900
>>> @@ -125,6 +125,7 @@
>>>  static void PrintUsage(JNIEnv* env, jboolean doXUsage);
>>>  static void ShowSettings(JNIEnv* env, char *optString);
>>>  static void ListModules(JNIEnv* env, char *optString);
>>> +static void SetNativeThreadName(JNIEnv* env, char *name);
>>> 
>>>  static void SetPaths(int argc, char **argv);
>>> 
>>> @@ -325,6 +326,7 @@
>>>   * mainThread.isAlive() to work as expected.
>>>   */
>>>  #define LEAVE() \
>>> +SetNativeThreadName(env, "DestroyJavaVM"); \
>>>  do { \
>>>  if ((*vm)->DetachCurrentThread(vm) != JNI_OK) { \
>>>  JLI_ReportErrorMessage(JVM_ERROR2); \
>>> @@ -488,6 +490,9 @@
>>>  mainArgs = CreateApplicationArgs(env, argv, argc);
>>>  CHECK_EXCEPTION_NULL_LEAVE(mainArgs);
>>> 
>>> +/* Set native thread name. */
>>> +SetNativeThreadName(env, "main");
>>> +
>>>  /* Invoke main method. */
>>>  (*env)->CallStaticVoidMethod(env, mainClass, mainID, mainArgs);
>>> 
>>> @@ -1686,6 +1691,22 @@
>>>   joptString);
>>>  }
>>> 
>>> +/**
>>> + * Set native thread name as possible.
>>> + */
>>> +static void
>>> +SetNativeThreadName(JNIEnv *env, char *name)
>>> +{
>>> +jmethodID setNativeThreadNameID;
>>> +jstring nameString;
>>> +jclass cls = GetLauncherHelperClass(env);
>>> +NULL_CHECK(cls);
>>> +NULL_CHECK(setNativeThreadNameID = (*env)->GetStaticMethodID(env, cls,
>>> +"setNativeThreadName", "(Ljava/lang/String;)V"));
>>> +NULL_CHECK(nameString = (*env)->NewStringUTF(env, 

Re: RFR: JDK-8152690: main thread does not have native thread name

[Kumar Srinivasan]

Hi,

Looks ok to me.

Thanks
Kumar


On 17/04/2016 8:38 PM, David Holmes wrote:

On 17/04/2016 1:38 PM, Yasumasa Suenaga wrote:

Hi David,


Need to hear from core-libs and/or launcher folk as this touches a
number of pieces of code.


I'm waiting more reviewer(s) .

BTW, Should I make patches which are based on jdk9/dev repos?
My proposal includes hotspot changes.
So I want to know whether I can push all changes jdk9/dev/{jdk,hotspot}
after reviewing.


No, jdk9/hs please.


And it will need to go via JPRT so I will sponsor it for you.

Thanks,
David


Thanks,
David


I can update my webrev to adapt to jdk9/dev repos if need.


Thanks,

Yasumasa


On 2016/04/17 7:20, David Holmes wrote:

Hi Yasumasa,

On 16/04/2016 7:29 PM, Yasumasa Suenaga wrote:

Hi David,

I uploaded new webrev:

  - hotspot:

http://cr.openjdk.java.net/~ysuenaga/JDK-8152690/webrev.03/hotspot/

  - jdk:
http://cr.openjdk.java.net/~ysuenaga/JDK-8152690/webrev.03/jdk/


Ah sneaky! :) Using JNI to by-pass access control so we can set up a
private method to do the name setting, yet avoid any API change that
would require a CCC request. I think I like it. :)

Need to hear from core-libs and/or launcher folk as this touches a
number of pieces of code.

Thanks,
David
-




it won't work unless you change the semantics of setName so I'm not
sure what you were thinking here. To take advantage of an arg taking
JVM_SetNativThreadName you would need to call it directly as no Java
code will call it . ???


I added a flag for setting native thread name to JNI-attached thread.
This change can set native thread name if main thread changes to
JNI-attached thread.


Thanks,

Yasumasa


On 2016/04/16 16:11, David Holmes wrote:

On 16/04/2016 3:27 PM, Yasumasa Suenaga wrote:

Hi David,

That change in behaviour may be a problem, it could be 
considered a

regression that setName stops setting the native thread main, even
though we never really intended it to work in the first place. :(
Such
a change needs to go through CCC.


I understood.
Can I send CCC request?
(I'm jdk 9 commiter, but I'm not employee at Oracle.)


Sorry you can't file a CCC request, you would need a sponsor for 
that.

But at this stage I don't think I agree with the proposed change
because of the change in behaviour - there's no way to restore the
"broken" behaviour.


I want to continue to discuss about it on JDK-8154331 [1].


Okay we can do that.




Further, we expect the launcher to use the supported JNI interface
(as
other processes would), not the internal JVM interface that exists
for
the JDK sources to communicate with the JVM.


I think that we do not use JVM interface if we add new method in
LauncherHelper as below:

diff -r f02139a1ac84
src/java.base/share/classes/sun/launcher/LauncherHelper.java
--- a/src/java.base/share/classes/sun/launcher/LauncherHelper.java
Wed Apr 13 14:19:30 2016 +
+++ b/src/java.base/share/classes/sun/launcher/LauncherHelper.java
Sat Apr 16 11:25:53 2016 +0900
@@ -960,4 +960,8 @@
  else
  return md.toNameAndVersion() + " (" + loc + ")";
  }
+
+static void setNativeThreadName(String name) {
+Thread.currentThread().setName(name);
+}


You could also make that call via JNI directly, so not sure the 
helper

adds much here. But it won't work unless you change the semantics of
setName so I'm not sure what you were thinking here. To take 
advantage

of an arg taking JVM_SetNativThreadName you would need to call it
directly as no Java code will call it . ???

David
-


  }
diff -r f02139a1ac84 src/java.base/share/native/libjli/java.c
--- a/src/java.base/share/native/libjli/java.cWed Apr 13 
14:19:30

2016 +
+++ b/src/java.base/share/native/libjli/java.cSat Apr 16 
11:25:53

2016 +0900
@@ -125,6 +125,7 @@
  static void PrintUsage(JNIEnv* env, jboolean doXUsage);
  static void ShowSettings(JNIEnv* env, char *optString);
  static void ListModules(JNIEnv* env, char *optString);
+static void SetNativeThreadName(JNIEnv* env, char *name);

  static void SetPaths(int argc, char **argv);

@@ -325,6 +326,7 @@
   * mainThread.isAlive() to work as expected.
   */
  #define LEAVE() \
+SetNativeThreadName(env, "DestroyJavaVM"); \
  do { \
  if ((*vm)->DetachCurrentThread(vm) != JNI_OK) { \
  JLI_ReportErrorMessage(JVM_ERROR2); \
@@ -488,6 +490,9 @@
  mainArgs = CreateApplicationArgs(env, argv, argc);
  CHECK_EXCEPTION_NULL_LEAVE(mainArgs);

+/* Set native thread name. */
+SetNativeThreadName(env, "main");
+
  /* Invoke main method. */
  (*env)->CallStaticVoidMethod(env, mainClass, mainID, 
mainArgs);


@@ -1686,6 +1691,22 @@
   joptString);
  }

+/**
+ * Set native thread name as possible.
+ */
+static void
+SetNativeThreadName(JNIEnv *env, char *name)
+{
+jmethodID setNativeThreadNameID;
+jstring nameString;
+jclass cls = GetLauncherHelperClass(env);
+NULL_CHECK(cls);
+

Re: RFR: JDK-8152690: main thread does not have native thread name

[Yasumasa Suenaga]

2016/04/18 13:41 "David Holmes" :
>
> On 17/04/2016 8:38 PM, David Holmes wrote:
>>
>> On 17/04/2016 1:38 PM, Yasumasa Suenaga wrote:
>>>
>>> Hi David,
>>>
 Need to hear from core-libs and/or launcher folk as this touches a
 number of pieces of code.
>>>
>>>
>>> I'm waiting more reviewer(s) .
>>>
>>> BTW, Should I make patches which are based on jdk9/dev repos?
>>> My proposal includes hotspot changes.
>>> So I want to know whether I can push all changes jdk9/dev/{jdk,hotspot}
>>> after reviewing.
>>
>>
>> No, jdk9/hs please.
>
>
> And it will need to go via JPRT so I will sponsor it for you.

Thanks!
I will send changesets to you after reviewing.

Yasumasa

> Thanks,
> David
>
>
>> Thanks,
>> David
>>
>>> I can update my webrev to adapt to jdk9/dev repos if need.
>>>
>>>
>>> Thanks,
>>>
>>> Yasumasa
>>>
>>>
>>> On 2016/04/17 7:20, David Holmes wrote:

 Hi Yasumasa,

 On 16/04/2016 7:29 PM, Yasumasa Suenaga wrote:
>
> Hi David,
>
> I uploaded new webrev:
>
>   - hotspot:
>
> http://cr.openjdk.java.net/~ysuenaga/JDK-8152690/webrev.03/hotspot/
>
>   - jdk:
>  http://cr.openjdk.java.net/~ysuenaga/JDK-8152690/webrev.03/jdk/


 Ah sneaky! :) Using JNI to by-pass access control so we can set up a
 private method to do the name setting, yet avoid any API change that
 would require a CCC request. I think I like it. :)

 Need to hear from core-libs and/or launcher folk as this touches a
 number of pieces of code.

 Thanks,
 David
 -

>
>> it won't work unless you change the semantics of setName so I'm not
>> sure what you were thinking here. To take advantage of an arg taking
>> JVM_SetNativThreadName you would need to call it directly as no Java
>> code will call it . ???
>
>
> I added a flag for setting native thread name to JNI-attached thread.
> This change can set native thread name if main thread changes to
> JNI-attached thread.
>
>
> Thanks,
>
> Yasumasa
>
>
> On 2016/04/16 16:11, David Holmes wrote:
>>
>> On 16/04/2016 3:27 PM, Yasumasa Suenaga wrote:
>>>
>>> Hi David,
>>>
 That change in behaviour may be a problem, it could be considered a
 regression that setName stops setting the native thread main, even
 though we never really intended it to work in the first place. :(
 Such
 a change needs to go through CCC.
>>>
>>>
>>> I understood.
>>> Can I send CCC request?
>>> (I'm jdk 9 commiter, but I'm not employee at Oracle.)
>>
>>
>> Sorry you can't file a CCC request, you would need a sponsor for
that.
>> But at this stage I don't think I agree with the proposed change
>> because of the change in behaviour - there's no way to restore the
>> "broken" behaviour.
>>
>>> I want to continue to discuss about it on JDK-8154331 [1].
>>
>>
>> Okay we can do that.
>>
>>>
 Further, we expect the launcher to use the supported JNI interface
 (as
 other processes would), not the internal JVM interface that exists
 for
 the JDK sources to communicate with the JVM.
>>>
>>>
>>> I think that we do not use JVM interface if we add new method in
>>> LauncherHelper as below:
>>> 
>>> diff -r f02139a1ac84
>>> src/java.base/share/classes/sun/launcher/LauncherHelper.java
>>> --- a/src/java.base/share/classes/sun/launcher/LauncherHelper.java
>>> Wed Apr 13 14:19:30 2016 +
>>> +++ b/src/java.base/share/classes/sun/launcher/LauncherHelper.java
>>> Sat Apr 16 11:25:53 2016 +0900
>>> @@ -960,4 +960,8 @@
>>>   else
>>>   return md.toNameAndVersion() + " (" + loc + ")";
>>>   }
>>> +
>>> +static void setNativeThreadName(String name) {
>>> +Thread.currentThread().setName(name);
>>> +}
>>
>>
>> You could also make that call via JNI directly, so not sure the
helper
>> adds much here. But it won't work unless you change the semantics of
>> setName so I'm not sure what you were thinking here. To take
advantage
>> of an arg taking JVM_SetNativThreadName you would need to call it
>> directly as no Java code will call it . ???
>>
>> David
>> -
>>
>>>   }
>>> diff -r f02139a1ac84 src/java.base/share/native/libjli/java.c
>>> --- a/src/java.base/share/native/libjli/java.cWed Apr 13
14:19:30
>>> 2016 +
>>> +++ b/src/java.base/share/native/libjli/java.cSat Apr 16
11:25:53
>>> 2016 +0900
>>> @@ -125,6 +125,7 @@
>>>   static void PrintUsage(JNIEnv* env, jboolean doXUsage);
>>>   static void ShowSettings(JNIEnv* env, char *optString);
>>>   static void ListModules(JNIEnv* env, char *optString);
>>> +static void 

Re: RFR: JDK-8152690: main thread does not have native thread name

[David Holmes]

On 17/04/2016 8:38 PM, David Holmes wrote:

On 17/04/2016 1:38 PM, Yasumasa Suenaga wrote:

Hi David,


Need to hear from core-libs and/or launcher folk as this touches a
number of pieces of code.


I'm waiting more reviewer(s) .

BTW, Should I make patches which are based on jdk9/dev repos?
My proposal includes hotspot changes.
So I want to know whether I can push all changes jdk9/dev/{jdk,hotspot}
after reviewing.


No, jdk9/hs please.


And it will need to go via JPRT so I will sponsor it for you.

Thanks,
David


Thanks,
David


I can update my webrev to adapt to jdk9/dev repos if need.


Thanks,

Yasumasa


On 2016/04/17 7:20, David Holmes wrote:

Hi Yasumasa,

On 16/04/2016 7:29 PM, Yasumasa Suenaga wrote:

Hi David,

I uploaded new webrev:

  - hotspot:

http://cr.openjdk.java.net/~ysuenaga/JDK-8152690/webrev.03/hotspot/

  - jdk:
 http://cr.openjdk.java.net/~ysuenaga/JDK-8152690/webrev.03/jdk/


Ah sneaky! :) Using JNI to by-pass access control so we can set up a
private method to do the name setting, yet avoid any API change that
would require a CCC request. I think I like it. :)

Need to hear from core-libs and/or launcher folk as this touches a
number of pieces of code.

Thanks,
David
-




it won't work unless you change the semantics of setName so I'm not
sure what you were thinking here. To take advantage of an arg taking
JVM_SetNativThreadName you would need to call it directly as no Java
code will call it . ???


I added a flag for setting native thread name to JNI-attached thread.
This change can set native thread name if main thread changes to
JNI-attached thread.


Thanks,

Yasumasa


On 2016/04/16 16:11, David Holmes wrote:

On 16/04/2016 3:27 PM, Yasumasa Suenaga wrote:

Hi David,


That change in behaviour may be a problem, it could be considered a
regression that setName stops setting the native thread main, even
though we never really intended it to work in the first place. :(
Such
a change needs to go through CCC.


I understood.
Can I send CCC request?
(I'm jdk 9 commiter, but I'm not employee at Oracle.)


Sorry you can't file a CCC request, you would need a sponsor for that.
But at this stage I don't think I agree with the proposed change
because of the change in behaviour - there's no way to restore the
"broken" behaviour.


I want to continue to discuss about it on JDK-8154331 [1].


Okay we can do that.




Further, we expect the launcher to use the supported JNI interface
(as
other processes would), not the internal JVM interface that exists
for
the JDK sources to communicate with the JVM.


I think that we do not use JVM interface if we add new method in
LauncherHelper as below:

diff -r f02139a1ac84
src/java.base/share/classes/sun/launcher/LauncherHelper.java
--- a/src/java.base/share/classes/sun/launcher/LauncherHelper.java
Wed Apr 13 14:19:30 2016 +
+++ b/src/java.base/share/classes/sun/launcher/LauncherHelper.java
Sat Apr 16 11:25:53 2016 +0900
@@ -960,4 +960,8 @@
  else
  return md.toNameAndVersion() + " (" + loc + ")";
  }
+
+static void setNativeThreadName(String name) {
+Thread.currentThread().setName(name);
+}


You could also make that call via JNI directly, so not sure the helper
adds much here. But it won't work unless you change the semantics of
setName so I'm not sure what you were thinking here. To take advantage
of an arg taking JVM_SetNativThreadName you would need to call it
directly as no Java code will call it . ???

David
-


  }
diff -r f02139a1ac84 src/java.base/share/native/libjli/java.c
--- a/src/java.base/share/native/libjli/java.cWed Apr 13 14:19:30
2016 +
+++ b/src/java.base/share/native/libjli/java.cSat Apr 16 11:25:53
2016 +0900
@@ -125,6 +125,7 @@
  static void PrintUsage(JNIEnv* env, jboolean doXUsage);
  static void ShowSettings(JNIEnv* env, char *optString);
  static void ListModules(JNIEnv* env, char *optString);
+static void SetNativeThreadName(JNIEnv* env, char *name);

  static void SetPaths(int argc, char **argv);

@@ -325,6 +326,7 @@
   * mainThread.isAlive() to work as expected.
   */
  #define LEAVE() \
+SetNativeThreadName(env, "DestroyJavaVM"); \
  do { \
  if ((*vm)->DetachCurrentThread(vm) != JNI_OK) { \
  JLI_ReportErrorMessage(JVM_ERROR2); \
@@ -488,6 +490,9 @@
  mainArgs = CreateApplicationArgs(env, argv, argc);
  CHECK_EXCEPTION_NULL_LEAVE(mainArgs);

+/* Set native thread name. */
+SetNativeThreadName(env, "main");
+
  /* Invoke main method. */
  (*env)->CallStaticVoidMethod(env, mainClass, mainID, mainArgs);

@@ -1686,6 +1691,22 @@
   joptString);
  }

+/**
+ * Set native thread name as possible.
+ */
+static void
+SetNativeThreadName(JNIEnv *env, char *name)
+{
+jmethodID setNativeThreadNameID;
+jstring nameString;
+jclass cls = GetLauncherHelperClass(env);
+NULL_CHECK(cls);
+NULL_CHECK(setNativeThreadNameID =

Re: RFR: JDK-8152690: main thread does not have native thread name

[David Holmes]

On 17/04/2016 1:38 PM, Yasumasa Suenaga wrote:

Hi David,


Need to hear from core-libs and/or launcher folk as this touches a
number of pieces of code.


I'm waiting more reviewer(s) .

BTW, Should I make patches which are based on jdk9/dev repos?
My proposal includes hotspot changes.
So I want to know whether I can push all changes jdk9/dev/{jdk,hotspot}
after reviewing.


No, jdk9/hs please.

Thanks,
David


I can update my webrev to adapt to jdk9/dev repos if need.


Thanks,

Yasumasa


On 2016/04/17 7:20, David Holmes wrote:

Hi Yasumasa,

On 16/04/2016 7:29 PM, Yasumasa Suenaga wrote:

Hi David,

I uploaded new webrev:

  - hotspot:
 http://cr.openjdk.java.net/~ysuenaga/JDK-8152690/webrev.03/hotspot/

  - jdk:
 http://cr.openjdk.java.net/~ysuenaga/JDK-8152690/webrev.03/jdk/


Ah sneaky! :) Using JNI to by-pass access control so we can set up a
private method to do the name setting, yet avoid any API change that
would require a CCC request. I think I like it. :)

Need to hear from core-libs and/or launcher folk as this touches a
number of pieces of code.

Thanks,
David
-




it won't work unless you change the semantics of setName so I'm not
sure what you were thinking here. To take advantage of an arg taking
JVM_SetNativThreadName you would need to call it directly as no Java
code will call it . ???


I added a flag for setting native thread name to JNI-attached thread.
This change can set native thread name if main thread changes to
JNI-attached thread.


Thanks,

Yasumasa


On 2016/04/16 16:11, David Holmes wrote:

On 16/04/2016 3:27 PM, Yasumasa Suenaga wrote:

Hi David,


That change in behaviour may be a problem, it could be considered a
regression that setName stops setting the native thread main, even
though we never really intended it to work in the first place. :(
Such
a change needs to go through CCC.


I understood.
Can I send CCC request?
(I'm jdk 9 commiter, but I'm not employee at Oracle.)


Sorry you can't file a CCC request, you would need a sponsor for that.
But at this stage I don't think I agree with the proposed change
because of the change in behaviour - there's no way to restore the
"broken" behaviour.


I want to continue to discuss about it on JDK-8154331 [1].


Okay we can do that.




Further, we expect the launcher to use the supported JNI interface
(as
other processes would), not the internal JVM interface that exists
for
the JDK sources to communicate with the JVM.


I think that we do not use JVM interface if we add new method in
LauncherHelper as below:

diff -r f02139a1ac84
src/java.base/share/classes/sun/launcher/LauncherHelper.java
--- a/src/java.base/share/classes/sun/launcher/LauncherHelper.java
Wed Apr 13 14:19:30 2016 +
+++ b/src/java.base/share/classes/sun/launcher/LauncherHelper.java
Sat Apr 16 11:25:53 2016 +0900
@@ -960,4 +960,8 @@
  else
  return md.toNameAndVersion() + " (" + loc + ")";
  }
+
+static void setNativeThreadName(String name) {
+Thread.currentThread().setName(name);
+}


You could also make that call via JNI directly, so not sure the helper
adds much here. But it won't work unless you change the semantics of
setName so I'm not sure what you were thinking here. To take advantage
of an arg taking JVM_SetNativThreadName you would need to call it
directly as no Java code will call it . ???

David
-


  }
diff -r f02139a1ac84 src/java.base/share/native/libjli/java.c
--- a/src/java.base/share/native/libjli/java.cWed Apr 13 14:19:30
2016 +
+++ b/src/java.base/share/native/libjli/java.cSat Apr 16 11:25:53
2016 +0900
@@ -125,6 +125,7 @@
  static void PrintUsage(JNIEnv* env, jboolean doXUsage);
  static void ShowSettings(JNIEnv* env, char *optString);
  static void ListModules(JNIEnv* env, char *optString);
+static void SetNativeThreadName(JNIEnv* env, char *name);

  static void SetPaths(int argc, char **argv);

@@ -325,6 +326,7 @@
   * mainThread.isAlive() to work as expected.
   */
  #define LEAVE() \
+SetNativeThreadName(env, "DestroyJavaVM"); \
  do { \
  if ((*vm)->DetachCurrentThread(vm) != JNI_OK) { \
  JLI_ReportErrorMessage(JVM_ERROR2); \
@@ -488,6 +490,9 @@
  mainArgs = CreateApplicationArgs(env, argv, argc);
  CHECK_EXCEPTION_NULL_LEAVE(mainArgs);

+/* Set native thread name. */
+SetNativeThreadName(env, "main");
+
  /* Invoke main method. */
  (*env)->CallStaticVoidMethod(env, mainClass, mainID, mainArgs);

@@ -1686,6 +1691,22 @@
   joptString);
  }

+/**
+ * Set native thread name as possible.
+ */
+static void
+SetNativeThreadName(JNIEnv *env, char *name)
+{
+jmethodID setNativeThreadNameID;
+jstring nameString;
+jclass cls = GetLauncherHelperClass(env);
+NULL_CHECK(cls);
+NULL_CHECK(setNativeThreadNameID =
(*env)->GetStaticMethodID(env, cls,
+"setNativeThreadName", "(Ljava/lang/String;)V"));
+NULL_CHECK(nameString = 

Re: RFR: JDK-8152690: main thread does not have native thread name

[Yasumasa Suenaga]

Hi David,


Need to hear from core-libs and/or launcher folk as this touches a number of 
pieces of code.


I'm waiting more reviewer(s) .

BTW, Should I make patches which are based on jdk9/dev repos?
My proposal includes hotspot changes.
So I want to know whether I can push all changes jdk9/dev/{jdk,hotspot} after 
reviewing.

I can update my webrev to adapt to jdk9/dev repos if need.


Thanks,

Yasumasa


On 2016/04/17 7:20, David Holmes wrote:

Hi Yasumasa,

On 16/04/2016 7:29 PM, Yasumasa Suenaga wrote:

Hi David,

I uploaded new webrev:

  - hotspot:
 http://cr.openjdk.java.net/~ysuenaga/JDK-8152690/webrev.03/hotspot/

  - jdk:
 http://cr.openjdk.java.net/~ysuenaga/JDK-8152690/webrev.03/jdk/


Ah sneaky! :) Using JNI to by-pass access control so we can set up a private 
method to do the name setting, yet avoid any API change that would require a 
CCC request. I think I like it. :)

Need to hear from core-libs and/or launcher folk as this touches a number of 
pieces of code.

Thanks,
David
-




it won't work unless you change the semantics of setName so I'm not
sure what you were thinking here. To take advantage of an arg taking
JVM_SetNativThreadName you would need to call it directly as no Java
code will call it . ???


I added a flag for setting native thread name to JNI-attached thread.
This change can set native thread name if main thread changes to
JNI-attached thread.


Thanks,

Yasumasa


On 2016/04/16 16:11, David Holmes wrote:

On 16/04/2016 3:27 PM, Yasumasa Suenaga wrote:

Hi David,


That change in behaviour may be a problem, it could be considered a
regression that setName stops setting the native thread main, even
though we never really intended it to work in the first place. :( Such
a change needs to go through CCC.


I understood.
Can I send CCC request?
(I'm jdk 9 commiter, but I'm not employee at Oracle.)


Sorry you can't file a CCC request, you would need a sponsor for that.
But at this stage I don't think I agree with the proposed change
because of the change in behaviour - there's no way to restore the
"broken" behaviour.


I want to continue to discuss about it on JDK-8154331 [1].


Okay we can do that.




Further, we expect the launcher to use the supported JNI interface (as
other processes would), not the internal JVM interface that exists for
the JDK sources to communicate with the JVM.


I think that we do not use JVM interface if we add new method in
LauncherHelper as below:

diff -r f02139a1ac84
src/java.base/share/classes/sun/launcher/LauncherHelper.java
--- a/src/java.base/share/classes/sun/launcher/LauncherHelper.java
Wed Apr 13 14:19:30 2016 +
+++ b/src/java.base/share/classes/sun/launcher/LauncherHelper.java
Sat Apr 16 11:25:53 2016 +0900
@@ -960,4 +960,8 @@
  else
  return md.toNameAndVersion() + " (" + loc + ")";
  }
+
+static void setNativeThreadName(String name) {
+Thread.currentThread().setName(name);
+}


You could also make that call via JNI directly, so not sure the helper
adds much here. But it won't work unless you change the semantics of
setName so I'm not sure what you were thinking here. To take advantage
of an arg taking JVM_SetNativThreadName you would need to call it
directly as no Java code will call it . ???

David
-


  }
diff -r f02139a1ac84 src/java.base/share/native/libjli/java.c
--- a/src/java.base/share/native/libjli/java.cWed Apr 13 14:19:30
2016 +
+++ b/src/java.base/share/native/libjli/java.cSat Apr 16 11:25:53
2016 +0900
@@ -125,6 +125,7 @@
  static void PrintUsage(JNIEnv* env, jboolean doXUsage);
  static void ShowSettings(JNIEnv* env, char *optString);
  static void ListModules(JNIEnv* env, char *optString);
+static void SetNativeThreadName(JNIEnv* env, char *name);

  static void SetPaths(int argc, char **argv);

@@ -325,6 +326,7 @@
   * mainThread.isAlive() to work as expected.
   */
  #define LEAVE() \
+SetNativeThreadName(env, "DestroyJavaVM"); \
  do { \
  if ((*vm)->DetachCurrentThread(vm) != JNI_OK) { \
  JLI_ReportErrorMessage(JVM_ERROR2); \
@@ -488,6 +490,9 @@
  mainArgs = CreateApplicationArgs(env, argv, argc);
  CHECK_EXCEPTION_NULL_LEAVE(mainArgs);

+/* Set native thread name. */
+SetNativeThreadName(env, "main");
+
  /* Invoke main method. */
  (*env)->CallStaticVoidMethod(env, mainClass, mainID, mainArgs);

@@ -1686,6 +1691,22 @@
   joptString);
  }

+/**
+ * Set native thread name as possible.
+ */
+static void
+SetNativeThreadName(JNIEnv *env, char *name)
+{
+jmethodID setNativeThreadNameID;
+jstring nameString;
+jclass cls = GetLauncherHelperClass(env);
+NULL_CHECK(cls);
+NULL_CHECK(setNativeThreadNameID =
(*env)->GetStaticMethodID(env, cls,
+"setNativeThreadName", "(Ljava/lang/String;)V"));
+NULL_CHECK(nameString = (*env)->NewStringUTF(env, name));
+(*env)->CallStaticVoidMethod(env, cls, 

Re: RFR: JDK-8152690: main thread does not have native thread name

[David Holmes]

Hi Yasumasa,

On 16/04/2016 7:29 PM, Yasumasa Suenaga wrote:

Hi David,

I uploaded new webrev:

  - hotspot:
 http://cr.openjdk.java.net/~ysuenaga/JDK-8152690/webrev.03/hotspot/

  - jdk:
 http://cr.openjdk.java.net/~ysuenaga/JDK-8152690/webrev.03/jdk/


Ah sneaky! :) Using JNI to by-pass access control so we can set up a 
private method to do the name setting, yet avoid any API change that 
would require a CCC request. I think I like it. :)


Need to hear from core-libs and/or launcher folk as this touches a 
number of pieces of code.


Thanks,
David
-




it won't work unless you change the semantics of setName so I'm not
sure what you were thinking here. To take advantage of an arg taking
JVM_SetNativThreadName you would need to call it directly as no Java
code will call it . ???


I added a flag for setting native thread name to JNI-attached thread.
This change can set native thread name if main thread changes to
JNI-attached thread.


Thanks,

Yasumasa


On 2016/04/16 16:11, David Holmes wrote:

On 16/04/2016 3:27 PM, Yasumasa Suenaga wrote:

Hi David,


That change in behaviour may be a problem, it could be considered a
regression that setName stops setting the native thread main, even
though we never really intended it to work in the first place. :( Such
a change needs to go through CCC.


I understood.
Can I send CCC request?
(I'm jdk 9 commiter, but I'm not employee at Oracle.)


Sorry you can't file a CCC request, you would need a sponsor for that.
But at this stage I don't think I agree with the proposed change
because of the change in behaviour - there's no way to restore the
"broken" behaviour.


I want to continue to discuss about it on JDK-8154331 [1].


Okay we can do that.




Further, we expect the launcher to use the supported JNI interface (as
other processes would), not the internal JVM interface that exists for
the JDK sources to communicate with the JVM.


I think that we do not use JVM interface if we add new method in
LauncherHelper as below:

diff -r f02139a1ac84
src/java.base/share/classes/sun/launcher/LauncherHelper.java
--- a/src/java.base/share/classes/sun/launcher/LauncherHelper.java
Wed Apr 13 14:19:30 2016 +
+++ b/src/java.base/share/classes/sun/launcher/LauncherHelper.java
Sat Apr 16 11:25:53 2016 +0900
@@ -960,4 +960,8 @@
  else
  return md.toNameAndVersion() + " (" + loc + ")";
  }
+
+static void setNativeThreadName(String name) {
+Thread.currentThread().setName(name);
+}


You could also make that call via JNI directly, so not sure the helper
adds much here. But it won't work unless you change the semantics of
setName so I'm not sure what you were thinking here. To take advantage
of an arg taking JVM_SetNativThreadName you would need to call it
directly as no Java code will call it . ???

David
-


  }
diff -r f02139a1ac84 src/java.base/share/native/libjli/java.c
--- a/src/java.base/share/native/libjli/java.cWed Apr 13 14:19:30
2016 +
+++ b/src/java.base/share/native/libjli/java.cSat Apr 16 11:25:53
2016 +0900
@@ -125,6 +125,7 @@
  static void PrintUsage(JNIEnv* env, jboolean doXUsage);
  static void ShowSettings(JNIEnv* env, char *optString);
  static void ListModules(JNIEnv* env, char *optString);
+static void SetNativeThreadName(JNIEnv* env, char *name);

  static void SetPaths(int argc, char **argv);

@@ -325,6 +326,7 @@
   * mainThread.isAlive() to work as expected.
   */
  #define LEAVE() \
+SetNativeThreadName(env, "DestroyJavaVM"); \
  do { \
  if ((*vm)->DetachCurrentThread(vm) != JNI_OK) { \
  JLI_ReportErrorMessage(JVM_ERROR2); \
@@ -488,6 +490,9 @@
  mainArgs = CreateApplicationArgs(env, argv, argc);
  CHECK_EXCEPTION_NULL_LEAVE(mainArgs);

+/* Set native thread name. */
+SetNativeThreadName(env, "main");
+
  /* Invoke main method. */
  (*env)->CallStaticVoidMethod(env, mainClass, mainID, mainArgs);

@@ -1686,6 +1691,22 @@
   joptString);
  }

+/**
+ * Set native thread name as possible.
+ */
+static void
+SetNativeThreadName(JNIEnv *env, char *name)
+{
+jmethodID setNativeThreadNameID;
+jstring nameString;
+jclass cls = GetLauncherHelperClass(env);
+NULL_CHECK(cls);
+NULL_CHECK(setNativeThreadNameID =
(*env)->GetStaticMethodID(env, cls,
+"setNativeThreadName", "(Ljava/lang/String;)V"));
+NULL_CHECK(nameString = (*env)->NewStringUTF(env, name));
+(*env)->CallStaticVoidMethod(env, cls, setNativeThreadNameID,
nameString);
+}
+
  /*
   * Prints default usage or the Xusage message, see
sun.launcher.LauncherHelper.java
   */


So I want to add new arg to JVM_SetNativeThreadName().


However this is still a change to the exported JVM interface and so
has to be approved.


Do you mean that this change needs CCC?


Thanks,

Yasumasa


[1]
http://mail.openjdk.java.net/pipermail/hotspot-runtime-dev/2016-April/019034.html




On 2016/04/16 

Re: RFR: JDK-8152690: main thread does not have native thread name

[Yasumasa Suenaga]

Hi David,

I uploaded new webrev:

 - hotspot:
http://cr.openjdk.java.net/~ysuenaga/JDK-8152690/webrev.03/hotspot/

 - jdk:
http://cr.openjdk.java.net/~ysuenaga/JDK-8152690/webrev.03/jdk/



it won't work unless you change the semantics of setName so I'm not sure what 
you were thinking here. To take advantage of an arg taking 
JVM_SetNativThreadName you would need to call it directly as no Java code will 
call it . ???


I added a flag for setting native thread name to JNI-attached thread.
This change can set native thread name if main thread changes to JNI-attached 
thread.


Thanks,

Yasumasa


On 2016/04/16 16:11, David Holmes wrote:

On 16/04/2016 3:27 PM, Yasumasa Suenaga wrote:

Hi David,


That change in behaviour may be a problem, it could be considered a
regression that setName stops setting the native thread main, even
though we never really intended it to work in the first place. :( Such
a change needs to go through CCC.


I understood.
Can I send CCC request?
(I'm jdk 9 commiter, but I'm not employee at Oracle.)


Sorry you can't file a CCC request, you would need a sponsor for that. But at this stage 
I don't think I agree with the proposed change because of the change in behaviour - 
there's no way to restore the "broken" behaviour.


I want to continue to discuss about it on JDK-8154331 [1].


Okay we can do that.




Further, we expect the launcher to use the supported JNI interface (as
other processes would), not the internal JVM interface that exists for
the JDK sources to communicate with the JVM.


I think that we do not use JVM interface if we add new method in
LauncherHelper as below:

diff -r f02139a1ac84
src/java.base/share/classes/sun/launcher/LauncherHelper.java
--- a/src/java.base/share/classes/sun/launcher/LauncherHelper.java
Wed Apr 13 14:19:30 2016 +
+++ b/src/java.base/share/classes/sun/launcher/LauncherHelper.java
Sat Apr 16 11:25:53 2016 +0900
@@ -960,4 +960,8 @@
  else
  return md.toNameAndVersion() + " (" + loc + ")";
  }
+
+static void setNativeThreadName(String name) {
+Thread.currentThread().setName(name);
+}


You could also make that call via JNI directly, so not sure the helper adds 
much here. But it won't work unless you change the semantics of setName so I'm 
not sure what you were thinking here. To take advantage of an arg taking 
JVM_SetNativThreadName you would need to call it directly as no Java code will 
call it . ???

David
-


  }
diff -r f02139a1ac84 src/java.base/share/native/libjli/java.c
--- a/src/java.base/share/native/libjli/java.cWed Apr 13 14:19:30
2016 +
+++ b/src/java.base/share/native/libjli/java.cSat Apr 16 11:25:53
2016 +0900
@@ -125,6 +125,7 @@
  static void PrintUsage(JNIEnv* env, jboolean doXUsage);
  static void ShowSettings(JNIEnv* env, char *optString);
  static void ListModules(JNIEnv* env, char *optString);
+static void SetNativeThreadName(JNIEnv* env, char *name);

  static void SetPaths(int argc, char **argv);

@@ -325,6 +326,7 @@
   * mainThread.isAlive() to work as expected.
   */
  #define LEAVE() \
+SetNativeThreadName(env, "DestroyJavaVM"); \
  do { \
  if ((*vm)->DetachCurrentThread(vm) != JNI_OK) { \
  JLI_ReportErrorMessage(JVM_ERROR2); \
@@ -488,6 +490,9 @@
  mainArgs = CreateApplicationArgs(env, argv, argc);
  CHECK_EXCEPTION_NULL_LEAVE(mainArgs);

+/* Set native thread name. */
+SetNativeThreadName(env, "main");
+
  /* Invoke main method. */
  (*env)->CallStaticVoidMethod(env, mainClass, mainID, mainArgs);

@@ -1686,6 +1691,22 @@
   joptString);
  }

+/**
+ * Set native thread name as possible.
+ */
+static void
+SetNativeThreadName(JNIEnv *env, char *name)
+{
+jmethodID setNativeThreadNameID;
+jstring nameString;
+jclass cls = GetLauncherHelperClass(env);
+NULL_CHECK(cls);
+NULL_CHECK(setNativeThreadNameID = (*env)->GetStaticMethodID(env, cls,
+"setNativeThreadName", "(Ljava/lang/String;)V"));
+NULL_CHECK(nameString = (*env)->NewStringUTF(env, name));
+(*env)->CallStaticVoidMethod(env, cls, setNativeThreadNameID,
nameString);
+}
+
  /*
   * Prints default usage or the Xusage message, see
sun.launcher.LauncherHelper.java
   */


So I want to add new arg to JVM_SetNativeThreadName().


However this is still a change to the exported JVM interface and so
has to be approved.


Do you mean that this change needs CCC?


Thanks,

Yasumasa


[1]
http://mail.openjdk.java.net/pipermail/hotspot-runtime-dev/2016-April/019034.html



On 2016/04/16 7:26, David Holmes wrote:

On 15/04/2016 11:20 PM, Yasumasa Suenaga wrote:

Hi David,


I think it is a bug based on the comment here:

JavaThread(bool is_attaching_via_jni = false); // for main thread and
JNI attached threads


I filed it to JBS as JDK-8154331.
I will send review request to hotspot-runtime-dev.



Though that will introduce a change in behaviour by 

Re: RFR: JDK-8152690: main thread does not have native thread name

[David Holmes]

On 16/04/2016 3:27 PM, Yasumasa Suenaga wrote:

Hi David,


That change in behaviour may be a problem, it could be considered a
regression that setName stops setting the native thread main, even
though we never really intended it to work in the first place. :( Such
a change needs to go through CCC.


I understood.
Can I send CCC request?
(I'm jdk 9 commiter, but I'm not employee at Oracle.)


Sorry you can't file a CCC request, you would need a sponsor for that. 
But at this stage I don't think I agree with the proposed change because 
of the change in behaviour - there's no way to restore the "broken" 
behaviour.



I want to continue to discuss about it on JDK-8154331 [1].


Okay we can do that.




Further, we expect the launcher to use the supported JNI interface (as
other processes would), not the internal JVM interface that exists for
the JDK sources to communicate with the JVM.


I think that we do not use JVM interface if we add new method in
LauncherHelper as below:

diff -r f02139a1ac84
src/java.base/share/classes/sun/launcher/LauncherHelper.java
--- a/src/java.base/share/classes/sun/launcher/LauncherHelper.java
Wed Apr 13 14:19:30 2016 +
+++ b/src/java.base/share/classes/sun/launcher/LauncherHelper.java
Sat Apr 16 11:25:53 2016 +0900
@@ -960,4 +960,8 @@
  else
  return md.toNameAndVersion() + " (" + loc + ")";
  }
+
+static void setNativeThreadName(String name) {
+Thread.currentThread().setName(name);
+}


You could also make that call via JNI directly, so not sure the helper 
adds much here. But it won't work unless you change the semantics of 
setName so I'm not sure what you were thinking here. To take advantage 
of an arg taking JVM_SetNativThreadName you would need to call it 
directly as no Java code will call it . ???


David
-


  }
diff -r f02139a1ac84 src/java.base/share/native/libjli/java.c
--- a/src/java.base/share/native/libjli/java.cWed Apr 13 14:19:30
2016 +
+++ b/src/java.base/share/native/libjli/java.cSat Apr 16 11:25:53
2016 +0900
@@ -125,6 +125,7 @@
  static void PrintUsage(JNIEnv* env, jboolean doXUsage);
  static void ShowSettings(JNIEnv* env, char *optString);
  static void ListModules(JNIEnv* env, char *optString);
+static void SetNativeThreadName(JNIEnv* env, char *name);

  static void SetPaths(int argc, char **argv);

@@ -325,6 +326,7 @@
   * mainThread.isAlive() to work as expected.
   */
  #define LEAVE() \
+SetNativeThreadName(env, "DestroyJavaVM"); \
  do { \
  if ((*vm)->DetachCurrentThread(vm) != JNI_OK) { \
  JLI_ReportErrorMessage(JVM_ERROR2); \
@@ -488,6 +490,9 @@
  mainArgs = CreateApplicationArgs(env, argv, argc);
  CHECK_EXCEPTION_NULL_LEAVE(mainArgs);

+/* Set native thread name. */
+SetNativeThreadName(env, "main");
+
  /* Invoke main method. */
  (*env)->CallStaticVoidMethod(env, mainClass, mainID, mainArgs);

@@ -1686,6 +1691,22 @@
   joptString);
  }

+/**
+ * Set native thread name as possible.
+ */
+static void
+SetNativeThreadName(JNIEnv *env, char *name)
+{
+jmethodID setNativeThreadNameID;
+jstring nameString;
+jclass cls = GetLauncherHelperClass(env);
+NULL_CHECK(cls);
+NULL_CHECK(setNativeThreadNameID = (*env)->GetStaticMethodID(env, cls,
+"setNativeThreadName", "(Ljava/lang/String;)V"));
+NULL_CHECK(nameString = (*env)->NewStringUTF(env, name));
+(*env)->CallStaticVoidMethod(env, cls, setNativeThreadNameID,
nameString);
+}
+
  /*
   * Prints default usage or the Xusage message, see
sun.launcher.LauncherHelper.java
   */


So I want to add new arg to JVM_SetNativeThreadName().


However this is still a change to the exported JVM interface and so
has to be approved.


Do you mean that this change needs CCC?


Thanks,

Yasumasa


[1]
http://mail.openjdk.java.net/pipermail/hotspot-runtime-dev/2016-April/019034.html



On 2016/04/16 7:26, David Holmes wrote:

On 15/04/2016 11:20 PM, Yasumasa Suenaga wrote:

Hi David,


I think it is a bug based on the comment here:

JavaThread(bool is_attaching_via_jni = false); // for main thread and
JNI attached threads


I filed it to JBS as JDK-8154331.
I will send review request to hotspot-runtime-dev.



Though that will introduce a change in behaviour by itself as setName
will no longer set the native name for the main thread.


I know.


That change in behaviour may be a problem, it could be considered a
regression that setName stops setting the native thread main, even
though we never really intended it to work in the first place. :( Such
a change needs to go through CCC.


I checked changeset history.
JVM_SetNativeThreadName() was introduced in JDK-7098194, and it is
backported JDK 8.


Yes this all came in as part of the OSX port in 7u2.


However, this function seems to be called from Thread#setNativeName()
only.
In addition, Thread#setNativeName() is private method.

Thus I think that we 

Re: RFR: JDK-8152690: main thread does not have native thread name

[David Holmes]

On 16/04/2016 3:27 PM, Yasumasa Suenaga wrote:

Hi David,


That change in behaviour may be a problem, it could be considered a
regression that setName stops setting the native thread main, even
though we never really intended it to work in the first place. :( Such
a change needs to go through CCC.


I understood.
Can I send CCC request?
(I'm jdk 9 commiter, but I'm not employee at Oracle.)


Sorry you can't file a CCC request, you would need a sponsor for that. 
But at this stage I don't think I agree with the proposed change because 
of the change in behaviour - there's no way to restore the "broken" 
behaviour.



I want to continue to discuss about it on JDK-8154331 [1].


Okay we can do that.




Further, we expect the launcher to use the supported JNI interface (as
other processes would), not the internal JVM interface that exists for
the JDK sources to communicate with the JVM.


I think that we do not use JVM interface if we add new method in
LauncherHelper as below:

diff -r f02139a1ac84
src/java.base/share/classes/sun/launcher/LauncherHelper.java
--- a/src/java.base/share/classes/sun/launcher/LauncherHelper.java
Wed Apr 13 14:19:30 2016 +
+++ b/src/java.base/share/classes/sun/launcher/LauncherHelper.java
Sat Apr 16 11:25:53 2016 +0900
@@ -960,4 +960,8 @@
  else
  return md.toNameAndVersion() + " (" + loc + ")";
  }
+
+static void setNativeThreadName(String name) {
+Thread.currentThread().setName(name);
+}


You could also make that call via JNI directly, so not sure the helper 
adds much here. But it won't work unless you change the semantics of 
setName so I'm not sure what you were thinking here. To take advantage 
of an arg taking JVM_SetNativThreadName you would need to call it 
directly as no Java code will call it . ???


David
-


  }
diff -r f02139a1ac84 src/java.base/share/native/libjli/java.c
--- a/src/java.base/share/native/libjli/java.cWed Apr 13 14:19:30
2016 +
+++ b/src/java.base/share/native/libjli/java.cSat Apr 16 11:25:53
2016 +0900
@@ -125,6 +125,7 @@
  static void PrintUsage(JNIEnv* env, jboolean doXUsage);
  static void ShowSettings(JNIEnv* env, char *optString);
  static void ListModules(JNIEnv* env, char *optString);
+static void SetNativeThreadName(JNIEnv* env, char *name);

  static void SetPaths(int argc, char **argv);

@@ -325,6 +326,7 @@
   * mainThread.isAlive() to work as expected.
   */
  #define LEAVE() \
+SetNativeThreadName(env, "DestroyJavaVM"); \
  do { \
  if ((*vm)->DetachCurrentThread(vm) != JNI_OK) { \
  JLI_ReportErrorMessage(JVM_ERROR2); \
@@ -488,6 +490,9 @@
  mainArgs = CreateApplicationArgs(env, argv, argc);
  CHECK_EXCEPTION_NULL_LEAVE(mainArgs);

+/* Set native thread name. */
+SetNativeThreadName(env, "main");
+
  /* Invoke main method. */
  (*env)->CallStaticVoidMethod(env, mainClass, mainID, mainArgs);

@@ -1686,6 +1691,22 @@
   joptString);
  }

+/**
+ * Set native thread name as possible.
+ */
+static void
+SetNativeThreadName(JNIEnv *env, char *name)
+{
+jmethodID setNativeThreadNameID;
+jstring nameString;
+jclass cls = GetLauncherHelperClass(env);
+NULL_CHECK(cls);
+NULL_CHECK(setNativeThreadNameID = (*env)->GetStaticMethodID(env, cls,
+"setNativeThreadName", "(Ljava/lang/String;)V"));
+NULL_CHECK(nameString = (*env)->NewStringUTF(env, name));
+(*env)->CallStaticVoidMethod(env, cls, setNativeThreadNameID,
nameString);
+}
+
  /*
   * Prints default usage or the Xusage message, see
sun.launcher.LauncherHelper.java
   */


So I want to add new arg to JVM_SetNativeThreadName().


However this is still a change to the exported JVM interface and so
has to be approved.


Do you mean that this change needs CCC?


Thanks,

Yasumasa


[1]
http://mail.openjdk.java.net/pipermail/hotspot-runtime-dev/2016-April/019034.html



On 2016/04/16 7:26, David Holmes wrote:

On 15/04/2016 11:20 PM, Yasumasa Suenaga wrote:

Hi David,


I think it is a bug based on the comment here:

JavaThread(bool is_attaching_via_jni = false); // for main thread and
JNI attached threads


I filed it to JBS as JDK-8154331.
I will send review request to hotspot-runtime-dev.



Though that will introduce a change in behaviour by itself as setName
will no longer set the native name for the main thread.


I know.


That change in behaviour may be a problem, it could be considered a
regression that setName stops setting the native thread main, even
though we never really intended it to work in the first place. :( Such
a change needs to go through CCC.


I checked changeset history.
JVM_SetNativeThreadName() was introduced in JDK-7098194, and it is
backported JDK 8.


Yes this all came in as part of the OSX port in 7u2.


However, this function seems to be called from Thread#setNativeName()
only.
In addition, Thread#setNativeName() is private method.

Thus I think that we 

Re: RFR: JDK-8152690: main thread does not have native thread name

[Yasumasa Suenaga]

Hi David,


That change in behaviour may be a problem, it could be considered a regression 
that setName stops setting the native thread main, even though we never really 
intended it to work in the first place. :( Such a change needs to go through 
CCC.


I understood.
Can I send CCC request?
(I'm jdk 9 commiter, but I'm not employee at Oracle.)

I want to continue to discuss about it on JDK-8154331 [1].



Further, we expect the launcher to use the supported JNI interface (as other 
processes would), not the internal JVM interface that exists for the JDK 
sources to communicate with the JVM.


I think that we do not use JVM interface if we add new method in LauncherHelper 
as below:

diff -r f02139a1ac84 
src/java.base/share/classes/sun/launcher/LauncherHelper.java
--- a/src/java.base/share/classes/sun/launcher/LauncherHelper.java  Wed Apr 
13 14:19:30 2016 +
+++ b/src/java.base/share/classes/sun/launcher/LauncherHelper.java  Sat Apr 
16 11:25:53 2016 +0900
@@ -960,4 +960,8 @@
 else
 return md.toNameAndVersion() + " (" + loc + ")";
 }
+
+static void setNativeThreadName(String name) {
+Thread.currentThread().setName(name);
+}
 }
diff -r f02139a1ac84 src/java.base/share/native/libjli/java.c
--- a/src/java.base/share/native/libjli/java.c  Wed Apr 13 14:19:30 2016 +
+++ b/src/java.base/share/native/libjli/java.c  Sat Apr 16 11:25:53 2016 +0900
@@ -125,6 +125,7 @@
 static void PrintUsage(JNIEnv* env, jboolean doXUsage);
 static void ShowSettings(JNIEnv* env, char *optString);
 static void ListModules(JNIEnv* env, char *optString);
+static void SetNativeThreadName(JNIEnv* env, char *name);
 
 static void SetPaths(int argc, char **argv);
 
@@ -325,6 +326,7 @@

  * mainThread.isAlive() to work as expected.
  */
 #define LEAVE() \
+SetNativeThreadName(env, "DestroyJavaVM"); \
 do { \
 if ((*vm)->DetachCurrentThread(vm) != JNI_OK) { \
 JLI_ReportErrorMessage(JVM_ERROR2); \
@@ -488,6 +490,9 @@
 mainArgs = CreateApplicationArgs(env, argv, argc);
 CHECK_EXCEPTION_NULL_LEAVE(mainArgs);
 
+/* Set native thread name. */

+SetNativeThreadName(env, "main");
+
 /* Invoke main method. */
 (*env)->CallStaticVoidMethod(env, mainClass, mainID, mainArgs);
 
@@ -1686,6 +1691,22 @@

  joptString);
 }
 
+/**

+ * Set native thread name as possible.
+ */
+static void
+SetNativeThreadName(JNIEnv *env, char *name)
+{
+jmethodID setNativeThreadNameID;
+jstring nameString;
+jclass cls = GetLauncherHelperClass(env);
+NULL_CHECK(cls);
+NULL_CHECK(setNativeThreadNameID = (*env)->GetStaticMethodID(env, cls,
+"setNativeThreadName", "(Ljava/lang/String;)V"));
+NULL_CHECK(nameString = (*env)->NewStringUTF(env, name));
+(*env)->CallStaticVoidMethod(env, cls, setNativeThreadNameID, nameString);
+}
+
 /*
  * Prints default usage or the Xusage message, see 
sun.launcher.LauncherHelper.java
  */


So I want to add new arg to JVM_SetNativeThreadName().


However this is still a change to the exported JVM interface and so has to be 
approved.


Do you mean that this change needs CCC?


Thanks,

Yasumasa
 


[1] 
http://mail.openjdk.java.net/pipermail/hotspot-runtime-dev/2016-April/019034.html


On 2016/04/16 7:26, David Holmes wrote:

On 15/04/2016 11:20 PM, Yasumasa Suenaga wrote:

Hi David,


I think it is a bug based on the comment here:

JavaThread(bool is_attaching_via_jni = false); // for main thread and
JNI attached threads


I filed it to JBS as JDK-8154331.
I will send review request to hotspot-runtime-dev.



Though that will introduce a change in behaviour by itself as setName
will no longer set the native name for the main thread.


I know.


That change in behaviour may be a problem, it could be considered a regression 
that setName stops setting the native thread main, even though we never really 
intended it to work in the first place. :( Such a change needs to go through 
CCC.


I checked changeset history.
JVM_SetNativeThreadName() was introduced in JDK-7098194, and it is
backported JDK 8.


Yes this all came in as part of the OSX port in 7u2.


However, this function seems to be called from Thread#setNativeName() only.
In addition, Thread#setNativeName() is private method.

Thus I think that we can add an argument to JVM_SetNativeThreadName()
for force setting.
(e.g. "bool forced")

It makes a change of JVM API.
However, this function is NOT public, so I think we can add one more
argument.

What do you think about this?
If it is accepted, we can set native thread name from Java launcher.


The private/public aspect of the Java API is not really at issue. Yes we would add 
another arg to the JVM function to allow it to apply to JNI-attached threads as well (I'd 
prefer the arg reflect that not just "force"). However this is still a change 
to the exported JVM interface and so has to be approved. Further, we expect the 

Re: RFR: JDK-8152690: main thread does not have native thread name

[David Holmes]

On 15/04/2016 11:20 PM, Yasumasa Suenaga wrote:

Hi David,


I think it is a bug based on the comment here:

JavaThread(bool is_attaching_via_jni = false); // for main thread and
JNI attached threads


I filed it to JBS as JDK-8154331.
I will send review request to hotspot-runtime-dev.



Though that will introduce a change in behaviour by itself as setName
will no longer set the native name for the main thread.


I know.


That change in behaviour may be a problem, it could be considered a 
regression that setName stops setting the native thread main, even 
though we never really intended it to work in the first place. :( Such a 
change needs to go through CCC.



I checked changeset history.
JVM_SetNativeThreadName() was introduced in JDK-7098194, and it is
backported JDK 8.


Yes this all came in as part of the OSX port in 7u2.


However, this function seems to be called from Thread#setNativeName() only.
In addition, Thread#setNativeName() is private method.

Thus I think that we can add an argument to JVM_SetNativeThreadName()
for force setting.
(e.g. "bool forced")

It makes a change of JVM API.
However, this function is NOT public, so I think we can add one more
argument.

What do you think about this?
If it is accepted, we can set native thread name from Java launcher.


The private/public aspect of the Java API is not really at issue. Yes we 
would add another arg to the JVM function to allow it to apply to 
JNI-attached threads as well (I'd prefer the arg reflect that not just 
"force"). However this is still a change to the exported JVM interface 
and so has to be approved. Further, we expect the launcher to use the 
supported JNI interface (as other processes would), not the internal JVM 
interface that exists for the JDK sources to communicate with the JVM.


David
-



Thanks,

Yasumasa


On 2016/04/15 19:16, David Holmes wrote:

Hi Yasumasa,

On 15/04/2016 6:53 PM, Yasumasa Suenaga wrote:

Hi David,


The fact that the "main" thread is not tagged as being a JNI-attached
thread seems accidental to me


Should I file it to JBS?


I think it is a bug based on the comment here:

JavaThread(bool is_attaching_via_jni = false); // for main thread and
JNI attached threads

Though that will introduce a change in behaviour by itself as setName
will no longer set the native name for the main thread.


I think that we can fix as below:
---
diff -r 52aa0ee93b32 src/share/vm/runtime/thread.cpp
--- a/src/share/vm/runtime/thread.cpp   Thu Apr 14 13:31:11 2016 +0200
+++ b/src/share/vm/runtime/thread.cpp   Fri Apr 15 17:50:10 2016 +0900
@@ -3592,7 +3592,7 @@
  #endif // INCLUDE_JVMCI

// Attach the main thread to this os thread
-  JavaThread* main_thread = new JavaThread();
+  JavaThread* main_thread = new JavaThread(true);
main_thread->set_thread_state(_thread_in_vm);
main_thread->initialize_thread_current();
// must do this before set_active_handles
@@ -3776,6 +3776,9 @@
// Notify JVMTI agents that VM initialization is complete - nop if
no agents.
JvmtiExport::post_vm_initialized();

+  // Change attach status to "attached"
+  main_thread->set_done_attaching_via_jni();
+


I think we can do this straight after the JavaThread constructor.


if (TRACE_START() != JNI_OK) {
  vm_exit_during_initialization("Failed to start tracing backend.");
}
---



If it wants to name its native threads then it is free to do so,


Currently, JVM_SetNativeThreadName() cannot change native thread name
when the caller thread is JNI-attached thread.
However, I think that it should be changed if Java developer calls
Thread#setName() explicitly.
It is not the same of changing native thread name at
Threads::create_vm().

If it is allowed, I want to fix SetNativeThreadName() as below.
What do you think about this?


The decision to not change the name of JNI-attached threads was a
deliberate one** - this functionality originated with the OSX port and
it was reported that the initial feedback with this feature was to
ensure it didn't mess with thread names that had been set by the host
process. If we do as you propose then we will just have an
inconsistency for people to complain about: "why does my native thread
only have a name if I call cur.setName(cur.getName()) ?"

** If you follow the bugs and related discussions on this, the
semantics and limitations (truncation, current thread only, non-JNI
threads only) of setting the native thread name were supposed to be
documented in the release notes - but as far as I can see that never
happened. :(

My position on this remains that if it is desirable for the main
thread (and DestroyJavaVM thread) to have native names then the
launcher needs to be setting them using the available platform APIs.
Unfortunately this is complicated - as evidenced by the VM code for
this - due to the need to verify API availability.

Any change in behaviour in relation to Thread.setName would have to go
through our CCC process I think. But a 

Re: RFR: JDK-8152690: main thread does not have native thread name

[Yasumasa Suenaga]

Hi David,


I think it is a bug based on the comment here:

JavaThread(bool is_attaching_via_jni = false); // for main thread and JNI 
attached threads


I filed it to JBS as JDK-8154331.
I will send review request to hotspot-runtime-dev.



Though that will introduce a change in behaviour by itself as setName will no 
longer set the native name for the main thread.


I know.

I checked changeset history.
JVM_SetNativeThreadName() was introduced in JDK-7098194, and it is backported 
JDK 8.
However, this function seems to be called from Thread#setNativeName() only.
In addition, Thread#setNativeName() is private method.

Thus I think that we can add an argument to JVM_SetNativeThreadName() for force 
setting.
(e.g. "bool forced")

It makes a change of JVM API.
However, this function is NOT public, so I think we can add one more argument.

What do you think about this?
If it is accepted, we can set native thread name from Java launcher.


Thanks,

Yasumasa


On 2016/04/15 19:16, David Holmes wrote:

Hi Yasumasa,

On 15/04/2016 6:53 PM, Yasumasa Suenaga wrote:

Hi David,


The fact that the "main" thread is not tagged as being a JNI-attached
thread seems accidental to me


Should I file it to JBS?


I think it is a bug based on the comment here:

JavaThread(bool is_attaching_via_jni = false); // for main thread and JNI 
attached threads

Though that will introduce a change in behaviour by itself as setName will no 
longer set the native name for the main thread.


I think that we can fix as below:
---
diff -r 52aa0ee93b32 src/share/vm/runtime/thread.cpp
--- a/src/share/vm/runtime/thread.cpp   Thu Apr 14 13:31:11 2016 +0200
+++ b/src/share/vm/runtime/thread.cpp   Fri Apr 15 17:50:10 2016 +0900
@@ -3592,7 +3592,7 @@
  #endif // INCLUDE_JVMCI

// Attach the main thread to this os thread
-  JavaThread* main_thread = new JavaThread();
+  JavaThread* main_thread = new JavaThread(true);
main_thread->set_thread_state(_thread_in_vm);
main_thread->initialize_thread_current();
// must do this before set_active_handles
@@ -3776,6 +3776,9 @@
// Notify JVMTI agents that VM initialization is complete - nop if
no agents.
JvmtiExport::post_vm_initialized();

+  // Change attach status to "attached"
+  main_thread->set_done_attaching_via_jni();
+


I think we can do this straight after the JavaThread constructor.


if (TRACE_START() != JNI_OK) {
  vm_exit_during_initialization("Failed to start tracing backend.");
}
---



If it wants to name its native threads then it is free to do so,


Currently, JVM_SetNativeThreadName() cannot change native thread name
when the caller thread is JNI-attached thread.
However, I think that it should be changed if Java developer calls
Thread#setName() explicitly.
It is not the same of changing native thread name at Threads::create_vm().

If it is allowed, I want to fix SetNativeThreadName() as below.
What do you think about this?


The decision to not change the name of JNI-attached threads was a deliberate one** - this 
functionality originated with the OSX port and it was reported that the initial feedback 
with this feature was to ensure it didn't mess with thread names that had been set by the 
host process. If we do as you propose then we will just have an inconsistency for people 
to complain about: "why does my native thread only have a name if I call 
cur.setName(cur.getName()) ?"

** If you follow the bugs and related discussions on this, the semantics and 
limitations (truncation, current thread only, non-JNI threads only) of setting 
the native thread name were supposed to be documented in the release notes - 
but as far as I can see that never happened. :(

My position on this remains that if it is desirable for the main thread (and 
DestroyJavaVM thread) to have native names then the launcher needs to be 
setting them using the available platform APIs. Unfortunately this is 
complicated - as evidenced by the VM code for this - due to the need to verify 
API availability.

Any change in behaviour in relation to Thread.setName would have to go through 
our CCC process I think. But a change in the launcher would not.

Sorry.

David
-


---
--- a/src/share/vm/prims/jvm.cppThu Apr 14 13:31:11 2016 +0200
+++ b/src/share/vm/prims/jvm.cppFri Apr 15 17:50:10 2016 +0900
@@ -3187,7 +3187,7 @@
JavaThread* thr = java_lang_Thread::thread(java_thread);
// Thread naming only supported for the current thread, doesn't work
for
// target threads.
-  if (Thread::current() == thr && !thr->has_attached_via_jni()) {
+  if (Thread::current() == thr) {
  // we don't set the name of an attached thread to avoid stepping
  // on other programs
  const char *thread_name =
java_lang_String::as_utf8_string(JNIHandles::resolve_non_null(name));
---


Thanks,

Yasumasa


On 2016/04/15 13:32, David Holmes wrote:



On 15/04/2016 1:11 AM, Yasumasa Suenaga wrote:

Roger,
Thanks for your 

Re: RFR: JDK-8152690: main thread does not have native thread name

[David Holmes]

Hi Yasumasa,

On 15/04/2016 6:53 PM, Yasumasa Suenaga wrote:

Hi David,


The fact that the "main" thread is not tagged as being a JNI-attached
thread seems accidental to me


Should I file it to JBS?


I think it is a bug based on the comment here:

JavaThread(bool is_attaching_via_jni = false); // for main thread and 
JNI attached threads


Though that will introduce a change in behaviour by itself as setName 
will no longer set the native name for the main thread.



I think that we can fix as below:
---
diff -r 52aa0ee93b32 src/share/vm/runtime/thread.cpp
--- a/src/share/vm/runtime/thread.cpp   Thu Apr 14 13:31:11 2016 +0200
+++ b/src/share/vm/runtime/thread.cpp   Fri Apr 15 17:50:10 2016 +0900
@@ -3592,7 +3592,7 @@
  #endif // INCLUDE_JVMCI

// Attach the main thread to this os thread
-  JavaThread* main_thread = new JavaThread();
+  JavaThread* main_thread = new JavaThread(true);
main_thread->set_thread_state(_thread_in_vm);
main_thread->initialize_thread_current();
// must do this before set_active_handles
@@ -3776,6 +3776,9 @@
// Notify JVMTI agents that VM initialization is complete - nop if
no agents.
JvmtiExport::post_vm_initialized();

+  // Change attach status to "attached"
+  main_thread->set_done_attaching_via_jni();
+


I think we can do this straight after the JavaThread constructor.


if (TRACE_START() != JNI_OK) {
  vm_exit_during_initialization("Failed to start tracing backend.");
}
---



If it wants to name its native threads then it is free to do so,


Currently, JVM_SetNativeThreadName() cannot change native thread name
when the caller thread is JNI-attached thread.
However, I think that it should be changed if Java developer calls
Thread#setName() explicitly.
It is not the same of changing native thread name at Threads::create_vm().

If it is allowed, I want to fix SetNativeThreadName() as below.
What do you think about this?


The decision to not change the name of JNI-attached threads was a 
deliberate one** - this functionality originated with the OSX port and 
it was reported that the initial feedback with this feature was to 
ensure it didn't mess with thread names that had been set by the host 
process. If we do as you propose then we will just have an inconsistency 
for people to complain about: "why does my native thread only have a 
name if I call cur.setName(cur.getName()) ?"


** If you follow the bugs and related discussions on this, the semantics 
and limitations (truncation, current thread only, non-JNI threads only) 
of setting the native thread name were supposed to be documented in the 
release notes - but as far as I can see that never happened. :(


My position on this remains that if it is desirable for the main thread 
(and DestroyJavaVM thread) to have native names then the launcher needs 
to be setting them using the available platform APIs. Unfortunately this 
is complicated - as evidenced by the VM code for this - due to the need 
to verify API availability.


Any change in behaviour in relation to Thread.setName would have to go 
through our CCC process I think. But a change in the launcher would not.


Sorry.

David
-


---
--- a/src/share/vm/prims/jvm.cppThu Apr 14 13:31:11 2016 +0200
+++ b/src/share/vm/prims/jvm.cppFri Apr 15 17:50:10 2016 +0900
@@ -3187,7 +3187,7 @@
JavaThread* thr = java_lang_Thread::thread(java_thread);
// Thread naming only supported for the current thread, doesn't work
for
// target threads.
-  if (Thread::current() == thr && !thr->has_attached_via_jni()) {
+  if (Thread::current() == thr) {
  // we don't set the name of an attached thread to avoid stepping
  // on other programs
  const char *thread_name =
java_lang_String::as_utf8_string(JNIHandles::resolve_non_null(name));
---


Thanks,

Yasumasa


On 2016/04/15 13:32, David Holmes wrote:



On 15/04/2016 1:11 AM, Yasumasa Suenaga wrote:

Roger,
Thanks for your comment!

David,


I'll wait to see what Kumar thinks about this. I don't like exposing
a new JVM function this way.


I tried to call Thread#setName() after initializing VM (before calling
main method),
I could set native thread name.
However, DestroyJavaVM() calls AttachCurrentThread(). So we can't set
native thread name for DestroyJavaVM.


Right - I came to the same realization earlier this morning. Which,
unfortunately, takes me back to the basic premise here that we don't
set the name of threads not created by the JVM. The fact that the
"main" thread is not tagged as being a JNI-attached thread seems
accidental to me - so JVM_SetNativeThreadName is only working by
accident for the initial attach, and can't be used for the
DestroyJavaVM part - which leaves the thread inconsistently named at
the native level.

I'm afraid my view here is that the launcher has to be treated like
any other process that might host a JVM. If it wants to name its
native threads then it is free to do so, but I 

Re: RFR: JDK-8152690: main thread does not have native thread name

[Yasumasa Suenaga]

Hi David,


The fact that the "main" thread is not tagged as being a JNI-attached thread 
seems accidental to me


Should I file it to JBS?
I think that we can fix as below:
---
diff -r 52aa0ee93b32 src/share/vm/runtime/thread.cpp
--- a/src/share/vm/runtime/thread.cpp   Thu Apr 14 13:31:11 2016 +0200
+++ b/src/share/vm/runtime/thread.cpp   Fri Apr 15 17:50:10 2016 +0900
@@ -3592,7 +3592,7 @@
 #endif // INCLUDE_JVMCI

   // Attach the main thread to this os thread
-  JavaThread* main_thread = new JavaThread();
+  JavaThread* main_thread = new JavaThread(true);
   main_thread->set_thread_state(_thread_in_vm);
   main_thread->initialize_thread_current();
   // must do this before set_active_handles
@@ -3776,6 +3776,9 @@
   // Notify JVMTI agents that VM initialization is complete - nop if no agents.
   JvmtiExport::post_vm_initialized();

+  // Change attach status to "attached"
+  main_thread->set_done_attaching_via_jni();
+
   if (TRACE_START() != JNI_OK) {
 vm_exit_during_initialization("Failed to start tracing backend.");
   }
---



If it wants to name its native threads then it is free to do so,


Currently, JVM_SetNativeThreadName() cannot change native thread name
when the caller thread is JNI-attached thread.
However, I think that it should be changed if Java developer calls
Thread#setName() explicitly.
It is not the same of changing native thread name at Threads::create_vm().

If it is allowed, I want to fix SetNativeThreadName() as below.
What do you think about this?
---
--- a/src/share/vm/prims/jvm.cppThu Apr 14 13:31:11 2016 +0200
+++ b/src/share/vm/prims/jvm.cppFri Apr 15 17:50:10 2016 +0900
@@ -3187,7 +3187,7 @@
   JavaThread* thr = java_lang_Thread::thread(java_thread);
   // Thread naming only supported for the current thread, doesn't work for
   // target threads.
-  if (Thread::current() == thr && !thr->has_attached_via_jni()) {
+  if (Thread::current() == thr) {
 // we don't set the name of an attached thread to avoid stepping
 // on other programs
 const char *thread_name = 
java_lang_String::as_utf8_string(JNIHandles::resolve_non_null(name));
---


Thanks,

Yasumasa


On 2016/04/15 13:32, David Holmes wrote:



On 15/04/2016 1:11 AM, Yasumasa Suenaga wrote:

Roger,
Thanks for your comment!

David,


I'll wait to see what Kumar thinks about this. I don't like exposing
a new JVM function this way.


I tried to call Thread#setName() after initializing VM (before calling
main method),
I could set native thread name.
However, DestroyJavaVM() calls AttachCurrentThread(). So we can't set
native thread name for DestroyJavaVM.


Right - I came to the same realization earlier this morning. Which, unfortunately, takes 
me back to the basic premise here that we don't set the name of threads not created by 
the JVM. The fact that the "main" thread is not tagged as being a JNI-attached 
thread seems accidental to me - so JVM_SetNativeThreadName is only working by accident 
for the initial attach, and can't be used for the DestroyJavaVM part - which leaves the 
thread inconsistently named at the native level.

I'm afraid my view here is that the launcher has to be treated like any other 
process that might host a JVM. If it wants to name its native threads then it 
is free to do so, but I would not be exporting a function from the JVM to do 
that - it would have to use the OS specific API's for that on a 
platform-by-platform basis.

Sorry.

David
-





Thanks,

Yasumasa


On 2016/04/14 23:24, Roger Riggs wrote:

Hi,

Comments:

jvm.h:  The function names are too similar but perform different
functions:

-JVM_SetNativeThreadName0 vs JVM_SetNativeThreadName

-  The first function applies to the current thread, the second one a
specific java thread.
  It would seem useful for there to be a comment somewhere about what
the new function does.

windows/native/libjli/java_md.c: line 408 casts to (void*) instead of
(SetNativeThreadName0_t)
as is done on unix and mac.

- macosx/native/libjli/java_md_macosx.c:
   - 737: looks wrong to overwriteifn->GetCreatedJavaVMs used at line 730
   - 738  Incorrect indentation; if possible keep the cast on the same
line as dlsym...

$.02, Roger


On 4/14/2016 9:32 AM, Yasumasa Suenaga wrote:

That is an interesting question which I haven't had time to check -
sorry. If the main thread is considered a JNI-attached thread then
my suggestion wont work. If it isn't then my suggestion should work
(but it means we have an inconsistency in our treatment of
JNI-attached threads :( )


I ran following program on JDK 9 EA b112, and I confirmed native
thread name (test) was set.
-
public class Sleep{
  public static void main(String[] args) throws Exception{
Thread.currentThread().setName("test");
Thread.sleep(360);
  }
}
-



I'll wait to see what Kumar thinks about this. I don't like exposing
a new JVM function this way.


I will update webrev after hearing Kumar's 

Re: RFR: JDK-8152690: main thread does not have native thread name

[David Holmes]

On 15/04/2016 1:11 AM, Yasumasa Suenaga wrote:

Roger,
Thanks for your comment!

David,


I'll wait to see what Kumar thinks about this. I don't like exposing
a new JVM function this way.


I tried to call Thread#setName() after initializing VM (before calling
main method),
I could set native thread name.
However, DestroyJavaVM() calls AttachCurrentThread(). So we can't set
native thread name for DestroyJavaVM.


Right - I came to the same realization earlier this morning. Which, 
unfortunately, takes me back to the basic premise here that we don't set 
the name of threads not created by the JVM. The fact that the "main" 
thread is not tagged as being a JNI-attached thread seems accidental to 
me - so JVM_SetNativeThreadName is only working by accident for the 
initial attach, and can't be used for the DestroyJavaVM part - which 
leaves the thread inconsistently named at the native level.


I'm afraid my view here is that the launcher has to be treated like any 
other process that might host a JVM. If it wants to name its native 
threads then it is free to do so, but I would not be exporting a 
function from the JVM to do that - it would have to use the OS specific 
API's for that on a platform-by-platform basis.


Sorry.

David
-





Thanks,

Yasumasa


On 2016/04/14 23:24, Roger Riggs wrote:

Hi,

Comments:

jvm.h:  The function names are too similar but perform different
functions:

-JVM_SetNativeThreadName0 vs JVM_SetNativeThreadName

-  The first function applies to the current thread, the second one a
specific java thread.
  It would seem useful for there to be a comment somewhere about what
the new function does.

windows/native/libjli/java_md.c: line 408 casts to (void*) instead of
(SetNativeThreadName0_t)
as is done on unix and mac.

- macosx/native/libjli/java_md_macosx.c:
   - 737: looks wrong to overwriteifn->GetCreatedJavaVMs used at line 730
   - 738  Incorrect indentation; if possible keep the cast on the same
line as dlsym...

$.02, Roger


On 4/14/2016 9:32 AM, Yasumasa Suenaga wrote:

That is an interesting question which I haven't had time to check -
sorry. If the main thread is considered a JNI-attached thread then
my suggestion wont work. If it isn't then my suggestion should work
(but it means we have an inconsistency in our treatment of
JNI-attached threads :( )


I ran following program on JDK 9 EA b112, and I confirmed native
thread name (test) was set.
-
public class Sleep{
  public static void main(String[] args) throws Exception{
Thread.currentThread().setName("test");
Thread.sleep(360);
  }
}
-



I'll wait to see what Kumar thinks about this. I don't like exposing
a new JVM function this way.


I will update webrev after hearing Kumar's comment.


Thanks,

Yasumasa


On 2016/04/14 21:32, David Holmes wrote:

On 14/04/2016 1:52 PM, Yasumasa Suenaga wrote:

Hi,

On 2016/04/14 9:34, David Holmes wrote:

Hi,

On 14/04/2016 1:28 AM, Yasumasa Suenaga wrote:

Hi David,

Thanks for your comment.

I exported new JVM function to set native thread name, and JLI
uses it
in new webrev.


First the launcher belongs to another team so core-libs will need to
review and approve this (in particular Kumar) - now cc'd.


Thanks!
I'm waiting to review :-)



Personally I would have used a Java upcall to Thread.setName rather
than exporting JVM_SetNativeThreadName. No hotspot changes needed in
that case.


As I wrote [1] in JBS, I changed to use Thread#setName() in
Thread#init(),
but I could not change native thread name.


At Thread.init time the thread is not alive, which is why the native
name is not set.


I guess that caller of main() is JNI attached thread.


That is an interesting question which I haven't had time to check -
sorry. If the main thread is considered a JNI-attached thread then
my suggestion wont work. If it isn't then my suggestion should work
(but it means we have an inconsistency in our treatment of
JNI-attached threads :( )

I'll wait to see what Kumar thinks about this. I don't like exposing
a new JVM function this way.

Thanks,
David
-


Thus I think that we have to provide a function to set native
thread name.


Thanks,

Yasumasa


[1]
https://bugs.openjdk.java.net/browse/JDK-8152690?focusedCommentId=13926851=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel#comment-13926851




Thanks,
David


Could you review again?

   - hotspot:

http://cr.openjdk.java.net/~ysuenaga/JDK-8152690/webrev.02/hotspot/

   - jdk:
http://cr.openjdk.java.net/~ysuenaga/JDK-8152690/webrev.02/jdk/


Thanks,

Yasumasa


On 2016/04/13 22:00, David Holmes wrote:

I'll answer on this original thread as well ...

Hi Yasumasa,

Please see my updates to the bug (sorry have been on vacation).
This
needs to be done in the launcher to be correct as we do not set the
name of threads that attach via JNI, which includes the "main"
thread.

David

On 31/03/2016 9:49 AM, Yasumasa Suenaga wrote:

Thanks Robbin,

I'm waiting a sponsor and more reviewer 

Re: RFR: JDK-8152690: main thread does not have native thread name

[Yasumasa Suenaga]

Roger,
Thanks for your comment!

David,


I'll wait to see what Kumar thinks about this. I don't like exposing a new JVM 
function this way.


I tried to call Thread#setName() after initializing VM (before calling main 
method),
I could set native thread name.
However, DestroyJavaVM() calls AttachCurrentThread(). So we can't set native 
thread name
for DestroyJavaVM.


Thanks,

Yasumasa


On 2016/04/14 23:24, Roger Riggs wrote:

Hi,

Comments:

jvm.h:  The function names are too similar but perform different functions:

-JVM_SetNativeThreadName0 vs JVM_SetNativeThreadName

-  The first function applies to the current thread, the second one a specific 
java thread.
  It would seem useful for there to be a comment somewhere about what the new 
function does.

windows/native/libjli/java_md.c: line 408 casts to (void*) instead of 
(SetNativeThreadName0_t)
as is done on unix and mac.

- macosx/native/libjli/java_md_macosx.c:
   - 737: looks wrong to overwriteifn->GetCreatedJavaVMs used at line 730
   - 738  Incorrect indentation; if possible keep the cast on the same line as 
dlsym...

$.02, Roger


On 4/14/2016 9:32 AM, Yasumasa Suenaga wrote:

That is an interesting question which I haven't had time to check - sorry. If 
the main thread is considered a JNI-attached thread then my suggestion wont 
work. If it isn't then my suggestion should work (but it means we have an 
inconsistency in our treatment of JNI-attached threads :( )


I ran following program on JDK 9 EA b112, and I confirmed native thread name 
(test) was set.
-
public class Sleep{
  public static void main(String[] args) throws Exception{
Thread.currentThread().setName("test");
Thread.sleep(360);
  }
}
-



I'll wait to see what Kumar thinks about this. I don't like exposing a new JVM 
function this way.


I will update webrev after hearing Kumar's comment.


Thanks,

Yasumasa


On 2016/04/14 21:32, David Holmes wrote:

On 14/04/2016 1:52 PM, Yasumasa Suenaga wrote:

Hi,

On 2016/04/14 9:34, David Holmes wrote:

Hi,

On 14/04/2016 1:28 AM, Yasumasa Suenaga wrote:

Hi David,

Thanks for your comment.

I exported new JVM function to set native thread name, and JLI uses it
in new webrev.


First the launcher belongs to another team so core-libs will need to
review and approve this (in particular Kumar) - now cc'd.


Thanks!
I'm waiting to review :-)



Personally I would have used a Java upcall to Thread.setName rather
than exporting JVM_SetNativeThreadName. No hotspot changes needed in
that case.


As I wrote [1] in JBS, I changed to use Thread#setName() in Thread#init(),
but I could not change native thread name.


At Thread.init time the thread is not alive, which is why the native name is 
not set.


I guess that caller of main() is JNI attached thread.


That is an interesting question which I haven't had time to check - sorry. If 
the main thread is considered a JNI-attached thread then my suggestion wont 
work. If it isn't then my suggestion should work (but it means we have an 
inconsistency in our treatment of JNI-attached threads :( )

I'll wait to see what Kumar thinks about this. I don't like exposing a new JVM 
function this way.

Thanks,
David
-


Thus I think that we have to provide a function to set native thread name.


Thanks,

Yasumasa


[1]
https://bugs.openjdk.java.net/browse/JDK-8152690?focusedCommentId=13926851=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel#comment-13926851



Thanks,
David


Could you review again?

   - hotspot:

http://cr.openjdk.java.net/~ysuenaga/JDK-8152690/webrev.02/hotspot/

   - jdk:
http://cr.openjdk.java.net/~ysuenaga/JDK-8152690/webrev.02/jdk/


Thanks,

Yasumasa


On 2016/04/13 22:00, David Holmes wrote:

I'll answer on this original thread as well ...

Hi Yasumasa,

Please see my updates to the bug (sorry have been on vacation). This
needs to be done in the launcher to be correct as we do not set the
name of threads that attach via JNI, which includes the "main" thread.

David

On 31/03/2016 9:49 AM, Yasumasa Suenaga wrote:

Thanks Robbin,

I'm waiting a sponsor and more reviewer :-)

Yasumasa
2016/03/31 5:58 "Robbin Ehn" :


FYI: I'm not a Reviewer.

/Robbin

On 03/30/2016 10:55 PM, Robbin Ehn wrote:


Thanks, looks good.

/Robbin

On 03/30/2016 03:47 PM, Yasumasa Suenaga wrote:


Hi,

I uploaded new webrev.
Could you review it?

http://cr.openjdk.java.net/~ysuenaga/JDK-8152690/webrev.01/


Thanks,

Yasumasa


On 2016/03/30 19:10, Robbin Ehn wrote:


Hi,

On 03/30/2016 11:41 AM, Yasumasa Suenaga wrote:


Hi Robbin,

2016/03/30 18:22 "Robbin Ehn" >:
  >
  > Hi Yasumasa,
  >
  >
  > On 03/25/2016 12:48 AM, Yasumasa Suenaga wrote:
  >>
  >> Hi Robbin,
  >> 2016/03/25 1:51 "Robbin Ehn" 
  >> >>:
  >>
  >>  >
  >>  > Hi Yasumasa,
  >>  >
  >>  > I'm not sure 

Re: RFR: JDK-8152690: main thread does not have native thread name

[Roger Riggs]

Hi,

Comments:

jvm.h:  The function names are too similar but perform different functions:

-JVM_SetNativeThreadName0 vs JVM_SetNativeThreadName

-  The first function applies to the current thread, the second one a 
specific java thread.
 It would seem useful for there to be a comment somewhere about what 
the new function does.


windows/native/libjli/java_md.c: line 408 casts to (void*) instead of 
(SetNativeThreadName0_t)

   as is done on unix and mac.

- macosx/native/libjli/java_md_macosx.c:
  - 737: looks wrong to overwriteifn->GetCreatedJavaVMs used at line 730
  - 738  Incorrect indentation; if possible keep the cast on the same 
line as dlsym...


$.02, Roger


On 4/14/2016 9:32 AM, Yasumasa Suenaga wrote:
That is an interesting question which I haven't had time to check - 
sorry. If the main thread is considered a JNI-attached thread then my 
suggestion wont work. If it isn't then my suggestion should work (but 
it means we have an inconsistency in our treatment of JNI-attached 
threads :( )


I ran following program on JDK 9 EA b112, and I confirmed native 
thread name (test) was set.

-
public class Sleep{
  public static void main(String[] args) throws Exception{
Thread.currentThread().setName("test");
Thread.sleep(360);
  }
}
-


I'll wait to see what Kumar thinks about this. I don't like exposing 
a new JVM function this way.


I will update webrev after hearing Kumar's comment.


Thanks,

Yasumasa


On 2016/04/14 21:32, David Holmes wrote:

On 14/04/2016 1:52 PM, Yasumasa Suenaga wrote:

Hi,

On 2016/04/14 9:34, David Holmes wrote:

Hi,

On 14/04/2016 1:28 AM, Yasumasa Suenaga wrote:

Hi David,

Thanks for your comment.

I exported new JVM function to set native thread name, and JLI 
uses it

in new webrev.


First the launcher belongs to another team so core-libs will need to
review and approve this (in particular Kumar) - now cc'd.


Thanks!
I'm waiting to review :-)



Personally I would have used a Java upcall to Thread.setName rather
than exporting JVM_SetNativeThreadName. No hotspot changes needed in
that case.


As I wrote [1] in JBS, I changed to use Thread#setName() in 
Thread#init(),

but I could not change native thread name.


At Thread.init time the thread is not alive, which is why the native 
name is not set.



I guess that caller of main() is JNI attached thread.


That is an interesting question which I haven't had time to check - 
sorry. If the main thread is considered a JNI-attached thread then my 
suggestion wont work. If it isn't then my suggestion should work (but 
it means we have an inconsistency in our treatment of JNI-attached 
threads :( )


I'll wait to see what Kumar thinks about this. I don't like exposing 
a new JVM function this way.


Thanks,
David
-

Thus I think that we have to provide a function to set native thread 
name.



Thanks,

Yasumasa


[1]
https://bugs.openjdk.java.net/browse/JDK-8152690?focusedCommentId=13926851=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel#comment-13926851 





Thanks,
David


Could you review again?

   - hotspot:

http://cr.openjdk.java.net/~ysuenaga/JDK-8152690/webrev.02/hotspot/

   - jdk:
http://cr.openjdk.java.net/~ysuenaga/JDK-8152690/webrev.02/jdk/


Thanks,

Yasumasa


On 2016/04/13 22:00, David Holmes wrote:

I'll answer on this original thread as well ...

Hi Yasumasa,

Please see my updates to the bug (sorry have been on vacation). This
needs to be done in the launcher to be correct as we do not set the
name of threads that attach via JNI, which includes the "main" 
thread.


David

On 31/03/2016 9:49 AM, Yasumasa Suenaga wrote:

Thanks Robbin,

I'm waiting a sponsor and more reviewer :-)

Yasumasa
2016/03/31 5:58 "Robbin Ehn" :


FYI: I'm not a Reviewer.

/Robbin

On 03/30/2016 10:55 PM, Robbin Ehn wrote:


Thanks, looks good.

/Robbin

On 03/30/2016 03:47 PM, Yasumasa Suenaga wrote:


Hi,

I uploaded new webrev.
Could you review it?

http://cr.openjdk.java.net/~ysuenaga/JDK-8152690/webrev.01/


Thanks,

Yasumasa


On 2016/03/30 19:10, Robbin Ehn wrote:


Hi,

On 03/30/2016 11:41 AM, Yasumasa Suenaga wrote:


Hi Robbin,

2016/03/30 18:22 "Robbin Ehn" >:
  >
  > Hi Yasumasa,
  >
  >
  > On 03/25/2016 12:48 AM, Yasumasa Suenaga wrote:
  >>
  >> Hi Robbin,
  >> 2016/03/25 1:51 "Robbin Ehn" 
  >> >>:
  >>
  >>  >
  >>  > Hi Yasumasa,
  >>  >
  >>  > I'm not sure why you don't set it:
  >>  >
  >>  > diff -r ded6ef79c770 src/share/vm/runtime/thread.cpp
  >>  > --- a/src/share/vm/runtime/thread.cpp   Thu Mar 24
13:09:16 2016
+
  >>  > +++ b/src/share/vm/runtime/thread.cpp   Thu Mar 24
17:40:09 2016
+0100
  >>  > @@ -3584,6 +3584,7 @@
  >>  >JavaThread* main_thread = new JavaThread();
  >>  > main_thread->set_thread_state(_thread_in_vm);
  >>  > 

Re: RFR: JDK-8152690: main thread does not have native thread name

[Yasumasa Suenaga]

That is an interesting question which I haven't had time to check - sorry. If 
the main thread is considered a JNI-attached thread then my suggestion wont 
work. If it isn't then my suggestion should work (but it means we have an 
inconsistency in our treatment of JNI-attached threads :( )


I ran following program on JDK 9 EA b112, and I confirmed native thread name 
(test) was set.
-
public class Sleep{
  public static void main(String[] args) throws Exception{
Thread.currentThread().setName("test");
Thread.sleep(360);
  }
}
-



I'll wait to see what Kumar thinks about this. I don't like exposing a new JVM 
function this way.


I will update webrev after hearing Kumar's comment.


Thanks,

Yasumasa


On 2016/04/14 21:32, David Holmes wrote:

On 14/04/2016 1:52 PM, Yasumasa Suenaga wrote:

Hi,

On 2016/04/14 9:34, David Holmes wrote:

Hi,

On 14/04/2016 1:28 AM, Yasumasa Suenaga wrote:

Hi David,

Thanks for your comment.

I exported new JVM function to set native thread name, and JLI uses it
in new webrev.


First the launcher belongs to another team so core-libs will need to
review and approve this (in particular Kumar) - now cc'd.


Thanks!
I'm waiting to review :-)



Personally I would have used a Java upcall to Thread.setName rather
than exporting JVM_SetNativeThreadName. No hotspot changes needed in
that case.


As I wrote [1] in JBS, I changed to use Thread#setName() in Thread#init(),
but I could not change native thread name.


At Thread.init time the thread is not alive, which is why the native name is 
not set.


I guess that caller of main() is JNI attached thread.


That is an interesting question which I haven't had time to check - sorry. If 
the main thread is considered a JNI-attached thread then my suggestion wont 
work. If it isn't then my suggestion should work (but it means we have an 
inconsistency in our treatment of JNI-attached threads :( )

I'll wait to see what Kumar thinks about this. I don't like exposing a new JVM 
function this way.

Thanks,
David
-


Thus I think that we have to provide a function to set native thread name.


Thanks,

Yasumasa


[1]
https://bugs.openjdk.java.net/browse/JDK-8152690?focusedCommentId=13926851=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel#comment-13926851



Thanks,
David


Could you review again?

   - hotspot:

http://cr.openjdk.java.net/~ysuenaga/JDK-8152690/webrev.02/hotspot/

   - jdk:
   http://cr.openjdk.java.net/~ysuenaga/JDK-8152690/webrev.02/jdk/


Thanks,

Yasumasa


On 2016/04/13 22:00, David Holmes wrote:

I'll answer on this original thread as well ...

Hi Yasumasa,

Please see my updates to the bug (sorry have been on vacation). This
needs to be done in the launcher to be correct as we do not set the
name of threads that attach via JNI, which includes the "main" thread.

David

On 31/03/2016 9:49 AM, Yasumasa Suenaga wrote:

Thanks Robbin,

I'm waiting a sponsor and more reviewer :-)

Yasumasa
2016/03/31 5:58 "Robbin Ehn" :


FYI: I'm not a Reviewer.

/Robbin

On 03/30/2016 10:55 PM, Robbin Ehn wrote:


Thanks, looks good.

/Robbin

On 03/30/2016 03:47 PM, Yasumasa Suenaga wrote:


Hi,

I uploaded new webrev.
Could you review it?

http://cr.openjdk.java.net/~ysuenaga/JDK-8152690/webrev.01/


Thanks,

Yasumasa


On 2016/03/30 19:10, Robbin Ehn wrote:


Hi,

On 03/30/2016 11:41 AM, Yasumasa Suenaga wrote:


Hi Robbin,

2016/03/30 18:22 "Robbin Ehn" >:
  >
  > Hi Yasumasa,
  >
  >
  > On 03/25/2016 12:48 AM, Yasumasa Suenaga wrote:
  >>
  >> Hi Robbin,
  >> 2016/03/25 1:51 "Robbin Ehn" 
  >> >>:
  >>
  >>  >
  >>  > Hi Yasumasa,
  >>  >
  >>  > I'm not sure why you don't set it:
  >>  >
  >>  > diff -r ded6ef79c770 src/share/vm/runtime/thread.cpp
  >>  > --- a/src/share/vm/runtime/thread.cpp   Thu Mar 24
13:09:16 2016
+
  >>  > +++ b/src/share/vm/runtime/thread.cpp   Thu Mar 24
17:40:09 2016
+0100
  >>  > @@ -3584,6 +3584,7 @@
  >>  >JavaThread* main_thread = new JavaThread();
  >>  >main_thread->set_thread_state(_thread_in_vm);
  >>  >main_thread->initialize_thread_current();
  >>  > +  main_thread->set_native_thread_name("main");
  >>  >// must do this before set_active_handles
  >>  >main_thread->record_stack_base_and_size();
  >>  >
main_thread->set_active_handles(JNIHandleBlock::allocate_block());

  >>  >
  >>  > here instead? Am I missing something?
  >>
  >> Native thread name is the same to thread name in Thread
class.
  >> It is set in c'tor in Thread or setName().
  >> If you create new thread in Java app, native thread name
will be
set at
  >> startup. However, main thread is already starte in VM.
  >> Thread name for "main" is set in create_initial_thread().
  >> I think that the place of setting thrrad name should be the
same.
  >
  >
  > Yes, I see 

Re: RFR: JDK-8152690: main thread does not have native thread name

[David Holmes]

On 14/04/2016 1:52 PM, Yasumasa Suenaga wrote:

Hi,

On 2016/04/14 9:34, David Holmes wrote:

Hi,

On 14/04/2016 1:28 AM, Yasumasa Suenaga wrote:

Hi David,

Thanks for your comment.

I exported new JVM function to set native thread name, and JLI uses it
in new webrev.


First the launcher belongs to another team so core-libs will need to
review and approve this (in particular Kumar) - now cc'd.


Thanks!
I'm waiting to review :-)



Personally I would have used a Java upcall to Thread.setName rather
than exporting JVM_SetNativeThreadName. No hotspot changes needed in
that case.


As I wrote [1] in JBS, I changed to use Thread#setName() in Thread#init(),
but I could not change native thread name.


At Thread.init time the thread is not alive, which is why the native 
name is not set.



I guess that caller of main() is JNI attached thread.


That is an interesting question which I haven't had time to check - 
sorry. If the main thread is considered a JNI-attached thread then my 
suggestion wont work. If it isn't then my suggestion should work (but it 
means we have an inconsistency in our treatment of JNI-attached threads :( )


I'll wait to see what Kumar thinks about this. I don't like exposing a 
new JVM function this way.


Thanks,
David
-


Thus I think that we have to provide a function to set native thread name.


Thanks,

Yasumasa


[1]
https://bugs.openjdk.java.net/browse/JDK-8152690?focusedCommentId=13926851=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel#comment-13926851



Thanks,
David


Could you review again?

   - hotspot:

http://cr.openjdk.java.net/~ysuenaga/JDK-8152690/webrev.02/hotspot/

   - jdk:
   http://cr.openjdk.java.net/~ysuenaga/JDK-8152690/webrev.02/jdk/


Thanks,

Yasumasa


On 2016/04/13 22:00, David Holmes wrote:

I'll answer on this original thread as well ...

Hi Yasumasa,

Please see my updates to the bug (sorry have been on vacation). This
needs to be done in the launcher to be correct as we do not set the
name of threads that attach via JNI, which includes the "main" thread.

David

On 31/03/2016 9:49 AM, Yasumasa Suenaga wrote:

Thanks Robbin,

I'm waiting a sponsor and more reviewer :-)

Yasumasa
2016/03/31 5:58 "Robbin Ehn" :


FYI: I'm not a Reviewer.

/Robbin

On 03/30/2016 10:55 PM, Robbin Ehn wrote:


Thanks, looks good.

/Robbin

On 03/30/2016 03:47 PM, Yasumasa Suenaga wrote:


Hi,

I uploaded new webrev.
Could you review it?

http://cr.openjdk.java.net/~ysuenaga/JDK-8152690/webrev.01/


Thanks,

Yasumasa


On 2016/03/30 19:10, Robbin Ehn wrote:


Hi,

On 03/30/2016 11:41 AM, Yasumasa Suenaga wrote:


Hi Robbin,

2016/03/30 18:22 "Robbin Ehn" >:
  >
  > Hi Yasumasa,
  >
  >
  > On 03/25/2016 12:48 AM, Yasumasa Suenaga wrote:
  >>
  >> Hi Robbin,
  >> 2016/03/25 1:51 "Robbin Ehn" 
  >> >>:
  >>
  >>  >
  >>  > Hi Yasumasa,
  >>  >
  >>  > I'm not sure why you don't set it:
  >>  >
  >>  > diff -r ded6ef79c770 src/share/vm/runtime/thread.cpp
  >>  > --- a/src/share/vm/runtime/thread.cpp   Thu Mar 24
13:09:16 2016
+
  >>  > +++ b/src/share/vm/runtime/thread.cpp   Thu Mar 24
17:40:09 2016
+0100
  >>  > @@ -3584,6 +3584,7 @@
  >>  >JavaThread* main_thread = new JavaThread();
  >>  >main_thread->set_thread_state(_thread_in_vm);
  >>  >main_thread->initialize_thread_current();
  >>  > +  main_thread->set_native_thread_name("main");
  >>  >// must do this before set_active_handles
  >>  >main_thread->record_stack_base_and_size();
  >>  >
main_thread->set_active_handles(JNIHandleBlock::allocate_block());

  >>  >
  >>  > here instead? Am I missing something?
  >>
  >> Native thread name is the same to thread name in Thread
class.
  >> It is set in c'tor in Thread or setName().
  >> If you create new thread in Java app, native thread name
will be
set at
  >> startup. However, main thread is already starte in VM.
  >> Thread name for "main" is set in create_initial_thread().
  >> I think that the place of setting thrrad name should be the
same.
  >
  >
  > Yes, I see your point. But then something like this is
nicer, no?
  >
  > --- a/src/share/vm/runtime/thread.cpp   Tue Mar 29 09:43:05
2016
+0200
  > +++ b/src/share/vm/runtime/thread.cpp   Wed Mar 30 10:51:12
2016
+0200
  > @@ -981,6 +981,7 @@
  >  // Creates the initial Thread
  >  static oop create_initial_thread(Handle thread_group,
JavaThread*
thread,
  >   TRAPS) {
  > +  static const char* initial_thread_name = "main";
  >Klass* k =
SystemDictionary::resolve_or_fail(vmSymbols::java_lang_Thread(),
true,
CHECK_NULL);
  >instanceKlassHandle klass (THREAD, k);
  >instanceHandle thread_oop =
klass->allocate_instance_handle(CHECK_NULL);
  > @@ -988,8 +989,10 @@
  >java_lang_Thread::set_thread(thread_oop(), thread);
  >

Re: RFR: JDK-8152690: main thread does not have native thread name

[Yasumasa Suenaga]

Hi,

On 2016/04/14 9:34, David Holmes wrote:

Hi,

On 14/04/2016 1:28 AM, Yasumasa Suenaga wrote:

Hi David,

Thanks for your comment.

I exported new JVM function to set native thread name, and JLI uses it
in new webrev.


First the launcher belongs to another team so core-libs will need to review and 
approve this (in particular Kumar) - now cc'd.


Thanks!
I'm waiting to review :-)



Personally I would have used a Java upcall to Thread.setName rather than 
exporting JVM_SetNativeThreadName. No hotspot changes needed in that case.


As I wrote [1] in JBS, I changed to use Thread#setName() in Thread#init(),
but I could not change native thread name.
I guess that caller of main() is JNI attached thread.

Thus I think that we have to provide a function to set native thread name.


Thanks,

Yasumasa


[1] 
https://bugs.openjdk.java.net/browse/JDK-8152690?focusedCommentId=13926851=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel#comment-13926851


Thanks,
David


Could you review again?

   - hotspot:
   http://cr.openjdk.java.net/~ysuenaga/JDK-8152690/webrev.02/hotspot/

   - jdk:
   http://cr.openjdk.java.net/~ysuenaga/JDK-8152690/webrev.02/jdk/


Thanks,

Yasumasa


On 2016/04/13 22:00, David Holmes wrote:

I'll answer on this original thread as well ...

Hi Yasumasa,

Please see my updates to the bug (sorry have been on vacation). This
needs to be done in the launcher to be correct as we do not set the
name of threads that attach via JNI, which includes the "main" thread.

David

On 31/03/2016 9:49 AM, Yasumasa Suenaga wrote:

Thanks Robbin,

I'm waiting a sponsor and more reviewer :-)

Yasumasa
2016/03/31 5:58 "Robbin Ehn" :


FYI: I'm not a Reviewer.

/Robbin

On 03/30/2016 10:55 PM, Robbin Ehn wrote:


Thanks, looks good.

/Robbin

On 03/30/2016 03:47 PM, Yasumasa Suenaga wrote:


Hi,

I uploaded new webrev.
Could you review it?

http://cr.openjdk.java.net/~ysuenaga/JDK-8152690/webrev.01/


Thanks,

Yasumasa


On 2016/03/30 19:10, Robbin Ehn wrote:


Hi,

On 03/30/2016 11:41 AM, Yasumasa Suenaga wrote:


Hi Robbin,

2016/03/30 18:22 "Robbin Ehn" >:
  >
  > Hi Yasumasa,
  >
  >
  > On 03/25/2016 12:48 AM, Yasumasa Suenaga wrote:
  >>
  >> Hi Robbin,
  >> 2016/03/25 1:51 "Robbin Ehn" 
  >> >>:
  >>
  >>  >
  >>  > Hi Yasumasa,
  >>  >
  >>  > I'm not sure why you don't set it:
  >>  >
  >>  > diff -r ded6ef79c770 src/share/vm/runtime/thread.cpp
  >>  > --- a/src/share/vm/runtime/thread.cpp   Thu Mar 24
13:09:16 2016
+
  >>  > +++ b/src/share/vm/runtime/thread.cpp   Thu Mar 24
17:40:09 2016
+0100
  >>  > @@ -3584,6 +3584,7 @@
  >>  >JavaThread* main_thread = new JavaThread();
  >>  >main_thread->set_thread_state(_thread_in_vm);
  >>  >main_thread->initialize_thread_current();
  >>  > +  main_thread->set_native_thread_name("main");
  >>  >// must do this before set_active_handles
  >>  >main_thread->record_stack_base_and_size();
  >>  >
main_thread->set_active_handles(JNIHandleBlock::allocate_block());
  >>  >
  >>  > here instead? Am I missing something?
  >>
  >> Native thread name is the same to thread name in Thread class.
  >> It is set in c'tor in Thread or setName().
  >> If you create new thread in Java app, native thread name
will be
set at
  >> startup. However, main thread is already starte in VM.
  >> Thread name for "main" is set in create_initial_thread().
  >> I think that the place of setting thrrad name should be the
same.
  >
  >
  > Yes, I see your point. But then something like this is
nicer, no?
  >
  > --- a/src/share/vm/runtime/thread.cpp   Tue Mar 29 09:43:05
2016
+0200
  > +++ b/src/share/vm/runtime/thread.cpp   Wed Mar 30 10:51:12
2016
+0200
  > @@ -981,6 +981,7 @@
  >  // Creates the initial Thread
  >  static oop create_initial_thread(Handle thread_group,
JavaThread*
thread,
  >   TRAPS) {
  > +  static const char* initial_thread_name = "main";
  >Klass* k =
SystemDictionary::resolve_or_fail(vmSymbols::java_lang_Thread(),
true,
CHECK_NULL);
  >instanceKlassHandle klass (THREAD, k);
  >instanceHandle thread_oop =
klass->allocate_instance_handle(CHECK_NULL);
  > @@ -988,8 +989,10 @@
  >java_lang_Thread::set_thread(thread_oop(), thread);
  >java_lang_Thread::set_priority(thread_oop(), NormPriority);
  >thread->set_threadObj(thread_oop());
  > -
  > -  Handle string = java_lang_String::create_from_str("main",
CHECK_NULL);
  > +
  > +  thread->set_native_thread_name(initial_thread_name);
  > +
  > +  Handle string =
java_lang_String::create_from_str(initial_thread_name, CHECK_NULL);
  >
  >JavaValue result(T_VOID);
  >JavaCalls::call_special(, thread_oop,

Okay, I will upload new webrev later.



Thanks!



  >>  > The launcher seem to name itself 'java' and naming this
thread
just

Re: RFR: JDK-8152690: main thread does not have native thread name

[David Holmes]

Hi,

On 14/04/2016 1:28 AM, Yasumasa Suenaga wrote:

Hi David,

Thanks for your comment.

I exported new JVM function to set native thread name, and JLI uses it
in new webrev.


First the launcher belongs to another team so core-libs will need to 
review and approve this (in particular Kumar) - now cc'd.


Personally I would have used a Java upcall to Thread.setName rather than 
exporting JVM_SetNativeThreadName. No hotspot changes needed in that case.


Thanks,
David


Could you review again?

   - hotspot:
   http://cr.openjdk.java.net/~ysuenaga/JDK-8152690/webrev.02/hotspot/

   - jdk:
   http://cr.openjdk.java.net/~ysuenaga/JDK-8152690/webrev.02/jdk/


Thanks,

Yasumasa


On 2016/04/13 22:00, David Holmes wrote:

I'll answer on this original thread as well ...

Hi Yasumasa,

Please see my updates to the bug (sorry have been on vacation). This
needs to be done in the launcher to be correct as we do not set the
name of threads that attach via JNI, which includes the "main" thread.

David

On 31/03/2016 9:49 AM, Yasumasa Suenaga wrote:

Thanks Robbin,

I'm waiting a sponsor and more reviewer :-)

Yasumasa
2016/03/31 5:58 "Robbin Ehn" :


FYI: I'm not a Reviewer.

/Robbin

On 03/30/2016 10:55 PM, Robbin Ehn wrote:


Thanks, looks good.

/Robbin

On 03/30/2016 03:47 PM, Yasumasa Suenaga wrote:


Hi,

I uploaded new webrev.
Could you review it?

http://cr.openjdk.java.net/~ysuenaga/JDK-8152690/webrev.01/


Thanks,

Yasumasa


On 2016/03/30 19:10, Robbin Ehn wrote:


Hi,

On 03/30/2016 11:41 AM, Yasumasa Suenaga wrote:


Hi Robbin,

2016/03/30 18:22 "Robbin Ehn" >:
  >
  > Hi Yasumasa,
  >
  >
  > On 03/25/2016 12:48 AM, Yasumasa Suenaga wrote:
  >>
  >> Hi Robbin,
  >> 2016/03/25 1:51 "Robbin Ehn" 
  >> >>:
  >>
  >>  >
  >>  > Hi Yasumasa,
  >>  >
  >>  > I'm not sure why you don't set it:
  >>  >
  >>  > diff -r ded6ef79c770 src/share/vm/runtime/thread.cpp
  >>  > --- a/src/share/vm/runtime/thread.cpp   Thu Mar 24
13:09:16 2016
+
  >>  > +++ b/src/share/vm/runtime/thread.cpp   Thu Mar 24
17:40:09 2016
+0100
  >>  > @@ -3584,6 +3584,7 @@
  >>  >JavaThread* main_thread = new JavaThread();
  >>  >main_thread->set_thread_state(_thread_in_vm);
  >>  >main_thread->initialize_thread_current();
  >>  > +  main_thread->set_native_thread_name("main");
  >>  >// must do this before set_active_handles
  >>  >main_thread->record_stack_base_and_size();
  >>  >
main_thread->set_active_handles(JNIHandleBlock::allocate_block());
  >>  >
  >>  > here instead? Am I missing something?
  >>
  >> Native thread name is the same to thread name in Thread class.
  >> It is set in c'tor in Thread or setName().
  >> If you create new thread in Java app, native thread name
will be
set at
  >> startup. However, main thread is already starte in VM.
  >> Thread name for "main" is set in create_initial_thread().
  >> I think that the place of setting thrrad name should be the
same.
  >
  >
  > Yes, I see your point. But then something like this is
nicer, no?
  >
  > --- a/src/share/vm/runtime/thread.cpp   Tue Mar 29 09:43:05
2016
+0200
  > +++ b/src/share/vm/runtime/thread.cpp   Wed Mar 30 10:51:12
2016
+0200
  > @@ -981,6 +981,7 @@
  >  // Creates the initial Thread
  >  static oop create_initial_thread(Handle thread_group,
JavaThread*
thread,
  >   TRAPS) {
  > +  static const char* initial_thread_name = "main";
  >Klass* k =
SystemDictionary::resolve_or_fail(vmSymbols::java_lang_Thread(),
true,
CHECK_NULL);
  >instanceKlassHandle klass (THREAD, k);
  >instanceHandle thread_oop =
klass->allocate_instance_handle(CHECK_NULL);
  > @@ -988,8 +989,10 @@
  >java_lang_Thread::set_thread(thread_oop(), thread);
  >java_lang_Thread::set_priority(thread_oop(), NormPriority);
  >thread->set_threadObj(thread_oop());
  > -
  > -  Handle string = java_lang_String::create_from_str("main",
CHECK_NULL);
  > +
  > +  thread->set_native_thread_name(initial_thread_name);
  > +
  > +  Handle string =
java_lang_String::create_from_str(initial_thread_name, CHECK_NULL);
  >
  >JavaValue result(T_VOID);
  >JavaCalls::call_special(, thread_oop,

Okay, I will upload new webrev later.



Thanks!



  >>  > The launcher seem to name itself 'java' and naming this
thread
just
  >>  > 'main' is confusing to me.
  >>  >
  >>  > E.g. so main thread of the process (and thus the
process) is
'java' but
  >>  > first JavaThread is 'main'.
  >>
  >> The native main thread in the process is not JavaThread. It is
waiting
  >> for ending of Java main thread with pthread_join().
  >> set_native_thread_name() is for JavaThread. So I think that
we do
not
  >> need to call it for native main thread.
  >
  >
  > Not sure if we can change it anyhow, since we want java and
native
name to be the same