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 <[email protected]> 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 +0000 >>> +++ 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.c Wed Apr 13 14:19:30 >>> 2016 +0000 >>> +++ 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 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 change in the launcher would >>>>>> not. >>>>>> >>>>>> Sorry. >>>>>> >>>>>> David >>>>>> ----- >>>>>> >>>>>>> --------------- >>>>>>> --- a/src/share/vm/prims/jvm.cpp Thu Apr 14 13:31:11 2016 +0200 >>>>>>> +++ b/src/share/vm/prims/jvm.cpp Fri 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(3600000); >>>>>>>>>>> } >>>>>>>>>>> } >>>>>>>>>>> --------- >>>>>>>>>>> >>>>>>>>>>> >>>>>>>>>>>> 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&page=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" <[email protected]>: >>>>>>>>>>>>>>>>> >>>>>>>>>>>>>>>>>> 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" <[email protected] >>>>>>>>>>>>>>>>>>>>>> <mailto:[email protected]>>: >>>>>>>>>>>>>>>>>>>>>> > >>>>>>>>>>>>>>>>>>>>>> > Hi Yasumasa, >>>>>>>>>>>>>>>>>>>>>> > >>>>>>>>>>>>>>>>>>>>>> > >>>>>>>>>>>>>>>>>>>>>> > On 03/25/2016 12:48 AM, Yasumasa Suenaga wrote: >>>>>>>>>>>>>>>>>>>>>> >> >>>>>>>>>>>>>>>>>>>>>> >> Hi Robbin, >>>>>>>>>>>>>>>>>>>>>> >> 2016/03/25 1:51 "Robbin Ehn" >>>>>>>>>>>>>>>>>>>>>> <[email protected] >>>>>>>>>>>>>>>>>>>>>> <mailto:[email protected]> >>>>>>>>>>>>>>>>>>>>>> >> <mailto:[email protected] >>>>>>>>>>>>>>>>>>>>>> <mailto:[email protected]>>>: >>>>>>>>>>>>>>>>>>>>>> >> >>>>>>>>>>>>>>>>>>>>>> >> > >>>>>>>>>>>>>>>>>>>>>> >> > 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 >>>>>>>>>>>>>>>>>>>>>> +0000 >>>>>>>>>>>>>>>>>>>>>> >> > +++ 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(&result, 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 and java thread name might have >>>>>>>>>>>>>>>>>>>>>> some >>>>>>>>>>>>>>>>>>>>>> dependents. >>>>>>>>>>>>>>>>>>>>>> > >>>>>>>>>>>>>>>>>>>>>> > The name is visible in e.g. /proc. >>>>>>>>>>>>>>>>>>>>>> > >>>>>>>>>>>>>>>>>>>>>> > $ ps H -C java -o 'pid tid comm' | head -4 >>>>>>>>>>>>>>>>>>>>>> > PID TID COMMAND >>>>>>>>>>>>>>>>>>>>>> > 6423 6423 java >>>>>>>>>>>>>>>>>>>>>> > 6423 6424 main >>>>>>>>>>>>>>>>>>>>>> > 6423 6425 GC Thread#0 >>>>>>>>>>>>>>>>>>>>>> > >>>>>>>>>>>>>>>>>>>>>> > It would be nice with something like 'Java Main >>>>>>>>>>>>>>>>>>>>>> Thread'. >>>>>>>>>>>>>>>>>>>>>> >>>>>>>>>>>>>>>>>>>>>> I do not think so. >>>>>>>>>>>>>>>>>>>>>> Native main thread might not be a Java launcher - e.g. >>>>>>>>>>>>>>>>>>>>>> Apache >>>>>>>>>>>>>>>>>>>>>> commons-daemon, JNI application, etc. >>>>>>>>>>>>>>>>>>>>>> >>>>>>>>>>>>>>>>>>>>>> If you want to change native main thread name, I think >>>>>>>>>>>>>>>>>>>>>> that we >>>>>>>>>>>>>>>>>>>>>> have to >>>>>>>>>>>>>>>>>>>>>> change Java launcher code. >>>>>>>>>>>>>>>>>>>>>> Should I include it in new webrev? >>>>>>>>>>>>>>>>>>>>>> >>>>>>>>>>>>>>>>>>>>> >>>>>>>>>>>>>>>>>>>>> No >>>>>>>>>>>>>>>>>>>>> >>>>>>>>>>>>>>>>>>>>> Thanks again! >>>>>>>>>>>>>>>>>>>>> >>>>>>>>>>>>>>>>>>>>> /Robbin >>>>>>>>>>>>>>>>>>>>> >>>>>>>>>>>>>>>>>>>>> >>>>>>>>>>>>>>>>>>>>>> Thanks, >>>>>>>>>>>>>>>>>>>>>> >>>>>>>>>>>>>>>>>>>>>> Yasumasa >>>>>>>>>>>>>>>>>>>>>> >>>>>>>>>>>>>>>>>>>>>> > Thanks >>>>>>>>>>>>>>>>>>>>>> > >>>>>>>>>>>>>>>>>>>>>> > /Robbin >>>>>>>>>>>>>>>>>>>>>> > >>>>>>>>>>>>>>>>>>>>>> > >>>>>>>>>>>>>>>>>>>>>> >> >>>>>>>>>>>>>>>>>>>>>> >> Thanks, >>>>>>>>>>>>>>>>>>>>>> >> >>>>>>>>>>>>>>>>>>>>>> >> Yasumasa >>>>>>>>>>>>>>>>>>>>>> >> >>>>>>>>>>>>>>>>>>>>>> >> > Thanks! >>>>>>>>>>>>>>>>>>>>>> >> > >>>>>>>>>>>>>>>>>>>>>> >> > /Robbin >>>>>>>>>>>>>>>>>>>>>> >> > >>>>>>>>>>>>>>>>>>>>>> >> > On 03/24/2016 03:26 PM, Yasumasa Suenaga wrote: >>>>>>>>>>>>>>>>>>>>>> >> > > Hi all, >>>>>>>>>>>>>>>>>>>>>> >> > > >>>>>>>>>>>>>>>>>>>>>> >> > > HotSpot for Linux will set thread name via >>>>>>>>>>>>>>>>>>>>>> pthread_setname_np(). >>>>>>>>>>>>>>>>>>>>>> >> > > However, main thread does not have it. >>>>>>>>>>>>>>>>>>>>>> >> > > >>>>>>>>>>>>>>>>>>>>>> >> > > All JavaThread have native name, and main >>>>>>>>>>>>>>>>>>>>>> thread is >>>>>>>>>>>>>>>>>>>>>> JavaThread. >>>>>>>>>>>>>>>>>>>>>> >> > > For consistency, main thread should have >>>>>>>>>>>>>>>>>>>>>> native >>>>>>>>>>>>>>>>>>>>>> thread >>>>>>>>>>>>>>>>>>>>>> name. >>>>>>>>>>>>>>>>>>>>>> >> > > >>>>>>>>>>>>>>>>>>>>>> >> > > I uploaded a webrev. Could you review it? >>>>>>>>>>>>>>>>>>>>>> >> > > >>>>>>>>>>>>>>>>>>>>>> >> > > >>>>>>>>>>>>>>>>>>>>>> http://cr.openjdk.java.net/~ysuenaga/JDK-8152690/webrev.00/ >>>>>>>>>>>>>>>>>>>>>> >>>>>>>>>>>>>>>>>>>>>> >>>>>>>>>>>>>>>>>>>>>> >> > > >>>>>>>>>>>>>>>>>>>>>> >> > > I cannot access JPRT. >>>>>>>>>>>>>>>>>>>>>> >> > > So I need a sponsor. >>>>>>>>>>>>>>>>>>>>>> >> > > >>>>>>>>>>>>>>>>>>>>>> >> > > >>>>>>>>>>>>>>>>>>>>>> >> > > Thanks, >>>>>>>>>>>>>>>>>>>>>> >> > > >>>>>>>>>>>>>>>>>>>>>> >> > > Yasumasa >>>>>>>>>>>>>>>>>>>>>> >> > > >>>>>>>>>>>>>>>>>>>>>> >> >>>>>>>>>>>>>>>>>>>>>> >>>>>>>>>>>>>>>>>>>>>> >>>>>>>>>>
