Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
330823004:
Set chrome thread name in JVM. (Closed)
DescriptionSet chrome thread name in JVM.
JVM's AttachCurrentThread() resets thread name if the caller doesn't pass
thread name. Pass it first time call of AttachCurrentThread(). Not call next
times to reduce syscall (prctl) overhead.
BUG=384603
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=278551
Patch Set 1 #
Total comments: 4
Patch Set 2 : Rebased #Patch Set 3 : Define AttachCurrentThreadWithName and call it for all browser threads. #
Total comments: 4
Messages
Total messages: 27 (0 generated)
thanks! one suggestion below, not sure if it'd work :) please let me know your thoughts! https://codereview.chromium.org/330823004/diff/1/base/android/jni_android.cc File base/android/jni_android.cc (right): https://codereview.chromium.org/330823004/diff/1/base/android/jni_android.cc#... base/android/jni_android.cc:87: if (!already_attached.Get().Get()) { hmm... AttachCurrentThread is a somewhat hot function, called on tons of places. how about moving this somewhere higher up, say something like: PlatformThread::SetName? https://code.google.com/p/chromium/codesearch#chromium/src/base/threading/pla... would that be sufficient?
https://codereview.chromium.org/330823004/diff/1/base/android/jni_android.cc File base/android/jni_android.cc (right): https://codereview.chromium.org/330823004/diff/1/base/android/jni_android.cc#... base/android/jni_android.cc:87: if (!already_attached.Get().Get()) { On 2014/06/17 17:26:55, bulach wrote: > hmm... AttachCurrentThread is a somewhat hot function, called on tons of places. > how about moving this somewhere higher up, say something like: > PlatformThread::SetName? > > https://code.google.com/p/chromium/codesearch#chromium/src/base/threading/pla... > > would that be sufficient? Do you mean call AttachCurrentThread() in PlatformThread::SetName()? It creates unnecessary thread objects in jvm. And, DetachFromVM() will undo it though it may be rarely called. To avoid lock in lazy instance, thread local boolean could be initailized in InitVM() below. If you concern about thread local boolean which calls pthread_get_specific(), AttachCurrentThread() also calls pthread_get_specific() in https://cs.corp.google.com/#android/dalvik/vm/Thread.cpp&l=1009 to fetch jvm structure.
https://codereview.chromium.org/330823004/diff/1/base/android/jni_android.cc File base/android/jni_android.cc (right): https://codereview.chromium.org/330823004/diff/1/base/android/jni_android.cc#... base/android/jni_android.cc:87: if (!already_attached.Get().Get()) { On 2014/06/17 17:51:05, byungchul wrote: > On 2014/06/17 17:26:55, bulach wrote: > > hmm... AttachCurrentThread is a somewhat hot function, called on tons of > places. > > how about moving this somewhere higher up, say something like: > > PlatformThread::SetName? > > > > > https://code.google.com/p/chromium/codesearch#chromium/src/base/threading/pla... > > > > would that be sufficient? > > Do you mean call AttachCurrentThread() in PlatformThread::SetName()? It creates > unnecessary thread objects in jvm. tough choice :) how many named threads which are never Attached to the VM there is? vs...how many times is AttachCurrentThread() be called? I suppose the overhead for the unnecessary objects may be smaller than the extra calls here, but I don't have the numbers... any chance we could investigate that? > And, DetachFromVM() will undo it though it > may be rarely called. yeah, that should only be called on thread destruction, so shouldn't be a problem. > To avoid lock in lazy instance, thread local boolean could be initailized in > InitVM() below. > If you concern about thread local boolean which calls pthread_get_specific(), > AttachCurrentThread() also calls pthread_get_specific() in > https://cs.corp.google.com/#android/dalvik/vm/Thread.cpp&l=1009 to fetch jvm > structure. sure, there's all sorts of hidden costs in crossing the boundaries :) but it'd be really nice to minimize it, specially in hot functions such as this... if the SetName turns out being less frequent, it'd better to call at that level.
https://codereview.chromium.org/330823004/diff/1/base/android/jni_android.cc File base/android/jni_android.cc (right): https://codereview.chromium.org/330823004/diff/1/base/android/jni_android.cc#... base/android/jni_android.cc:87: if (!already_attached.Get().Get()) { On 2014/06/17 20:45:33, bulach wrote: > On 2014/06/17 17:51:05, byungchul wrote: > > On 2014/06/17 17:26:55, bulach wrote: > > > hmm... AttachCurrentThread is a somewhat hot function, called on tons of > > places. > > > how about moving this somewhere higher up, say something like: > > > PlatformThread::SetName? > > > > > > > > > https://code.google.com/p/chromium/codesearch#chromium/src/base/threading/pla... > > > > > > would that be sufficient? > > > > Do you mean call AttachCurrentThread() in PlatformThread::SetName()? It > creates > > unnecessary thread objects in jvm. > > tough choice :) > how many named threads which are never Attached to the VM there is? > vs...how many times is AttachCurrentThread() be called? > I suppose the overhead for the unnecessary objects may be smaller than the extra > calls here, but I don't have the numbers... > any chance we could investigate that? > > > And, DetachFromVM() will undo it though it > > may be rarely called. > > yeah, that should only be called on thread destruction, so shouldn't be a > problem. > > > To avoid lock in lazy instance, thread local boolean could be initailized in > > InitVM() below. > > If you concern about thread local boolean which calls pthread_get_specific(), > > AttachCurrentThread() also calls pthread_get_specific() in > > https://cs.corp.google.com/#android/dalvik/vm/Thread.cpp&l=1009 to fetch jvm > > structure. > > sure, there's all sorts of hidden costs in crossing the boundaries :) > but it'd be really nice to minimize it, specially in hot functions such as > this... > if the SetName turns out being less frequent, it'd better to call at that level. I am not working on android chrome, but a different project utilizing it. Here is a snapshot of threads after masking some thread names. Thread-??? is the name assigned by jvm. So, 14 of 52 threads will have benefit for this change while we may create jvm thread object up to 38 more. XXXXXXX Heap Heap Heap Signal JDWP ReferenceQueueD FinalizerDaemon FinalizerWatchd HeapTrimmerDaem GCDaemon Binder_1 Binder_2 AsyncTask pool-1-thread-1 RefQueueWorker@ DnsConfigServic inotify_reader Thread-119 SettingsObserve Chrome_DBThread Thread-114 Chrome_FileUser Thread-115 Chrome_CacheThr Thread-103 IndexedDB XXXXXXX XXXXXXX XXXXXXX Thread-105 Thread-107 File Thread-111 Thread-113 XXXXXXX pool-2-thread-1 pool-3-thread-1 NsdManager Thread-107 Thread-107 Thread-107 Thread-107 Thread-107 Thread-107 ConfigFileIOWor SimpleCacheWork RenderThread Chrome_DevTools AsyncTransferTh JavaBridge Binder_3
On 2014/06/17 21:11:11, byungchul wrote: > https://codereview.chromium.org/330823004/diff/1/base/android/jni_android.cc > File base/android/jni_android.cc (right): > > https://codereview.chromium.org/330823004/diff/1/base/android/jni_android.cc#... > base/android/jni_android.cc:87: if (!already_attached.Get().Get()) { > On 2014/06/17 20:45:33, bulach wrote: > > On 2014/06/17 17:51:05, byungchul wrote: > > > On 2014/06/17 17:26:55, bulach wrote: > > > > hmm... AttachCurrentThread is a somewhat hot function, called on tons of > > > places. > > > > how about moving this somewhere higher up, say something like: > > > > PlatformThread::SetName? > > > > > > > > > > > > > > https://code.google.com/p/chromium/codesearch#chromium/src/base/threading/pla... > > > > > > > > would that be sufficient? > > > > > > Do you mean call AttachCurrentThread() in PlatformThread::SetName()? It > > creates > > > unnecessary thread objects in jvm. > > > > tough choice :) > > how many named threads which are never Attached to the VM there is? > > vs...how many times is AttachCurrentThread() be called? > > I suppose the overhead for the unnecessary objects may be smaller than the > extra > > calls here, but I don't have the numbers... > > any chance we could investigate that? > > > > > And, DetachFromVM() will undo it though it > > > may be rarely called. > > > > yeah, that should only be called on thread destruction, so shouldn't be a > > problem. > > > > > To avoid lock in lazy instance, thread local boolean could be initailized in > > > InitVM() below. > > > If you concern about thread local boolean which calls > pthread_get_specific(), > > > AttachCurrentThread() also calls pthread_get_specific() in > > > https://cs.corp.google.com/#android/dalvik/vm/Thread.cpp&l=1009 to fetch jvm > > > structure. > > > > sure, there's all sorts of hidden costs in crossing the boundaries :) > > but it'd be really nice to minimize it, specially in hot functions such as > > this... > > if the SetName turns out being less frequent, it'd better to call at that > level. > > I am not working on android chrome, but a different project utilizing it. Here > is a snapshot of threads after masking some thread names. Thread-??? is the name > assigned by jvm. So, 14 of 52 threads will have benefit for this change while we > may create jvm thread object up to 38 more. > > XXXXXXX > Heap > Heap > Heap > Signal > JDWP > ReferenceQueueD > FinalizerDaemon > FinalizerWatchd > HeapTrimmerDaem > GCDaemon > Binder_1 > Binder_2 > AsyncTask > pool-1-thread-1 > RefQueueWorker@ > DnsConfigServic > inotify_reader > Thread-119 > SettingsObserve > Chrome_DBThread > Thread-114 > Chrome_FileUser > Thread-115 > Chrome_CacheThr > Thread-103 > IndexedDB > XXXXXXX > XXXXXXX > XXXXXXX > Thread-105 > Thread-107 > File > Thread-111 > Thread-113 > XXXXXXX > pool-2-thread-1 > pool-3-thread-1 > NsdManager > Thread-107 > Thread-107 > Thread-107 > Thread-107 > Thread-107 > Thread-107 > ConfigFileIOWor > SimpleCacheWork > RenderThread > Chrome_DevTools > AsyncTransferTh > JavaBridge > Binder_3 thanks! is there any significant impact in terms of memory for those 38 extra objects? I'm tempted to say "no" compared to the amount of times AttachCurrentThread is called... another alternative: we could do this at the BrowserThreadImpl level: https://cs.corp.google.com/#android/external/chromium_org/content/browser/bro... if you're not on android chrome, perhaps we could expose an utility so then you can initialize it when creating the thread on your component? wdyt?
On 2014/06/17 21:25:03, bulach wrote: > On 2014/06/17 21:11:11, byungchul wrote: > > https://codereview.chromium.org/330823004/diff/1/base/android/jni_android.cc > > File base/android/jni_android.cc (right): > > > > > https://codereview.chromium.org/330823004/diff/1/base/android/jni_android.cc#... > > base/android/jni_android.cc:87: if (!already_attached.Get().Get()) { > > On 2014/06/17 20:45:33, bulach wrote: > > > On 2014/06/17 17:51:05, byungchul wrote: > > > > On 2014/06/17 17:26:55, bulach wrote: > > > > > hmm... AttachCurrentThread is a somewhat hot function, called on tons of > > > > places. > > > > > how about moving this somewhere higher up, say something like: > > > > > PlatformThread::SetName? > > > > > > > > > > > > > > > > > > > > https://code.google.com/p/chromium/codesearch#chromium/src/base/threading/pla... > > > > > > > > > > would that be sufficient? > > > > > > > > Do you mean call AttachCurrentThread() in PlatformThread::SetName()? It > > > creates > > > > unnecessary thread objects in jvm. > > > > > > tough choice :) > > > how many named threads which are never Attached to the VM there is? > > > vs...how many times is AttachCurrentThread() be called? > > > I suppose the overhead for the unnecessary objects may be smaller than the > > extra > > > calls here, but I don't have the numbers... > > > any chance we could investigate that? > > > > > > > And, DetachFromVM() will undo it though it > > > > may be rarely called. > > > > > > yeah, that should only be called on thread destruction, so shouldn't be a > > > problem. > > > > > > > To avoid lock in lazy instance, thread local boolean could be initailized > in > > > > InitVM() below. > > > > If you concern about thread local boolean which calls > > pthread_get_specific(), > > > > AttachCurrentThread() also calls pthread_get_specific() in > > > > https://cs.corp.google.com/#android/dalvik/vm/Thread.cpp&l=1009 to fetch > jvm > > > > structure. > > > > > > sure, there's all sorts of hidden costs in crossing the boundaries :) > > > but it'd be really nice to minimize it, specially in hot functions such as > > > this... > > > if the SetName turns out being less frequent, it'd better to call at that > > level. > > > > I am not working on android chrome, but a different project utilizing it. Here > > is a snapshot of threads after masking some thread names. Thread-??? is the > name > > assigned by jvm. So, 14 of 52 threads will have benefit for this change while > we > > may create jvm thread object up to 38 more. > > > > XXXXXXX > > Heap > > Heap > > Heap > > Signal > > JDWP > > ReferenceQueueD > > FinalizerDaemon > > FinalizerWatchd > > HeapTrimmerDaem > > GCDaemon > > Binder_1 > > Binder_2 > > AsyncTask > > pool-1-thread-1 > > RefQueueWorker@ > > DnsConfigServic > > inotify_reader > > Thread-119 > > SettingsObserve > > Chrome_DBThread > > Thread-114 > > Chrome_FileUser > > Thread-115 > > Chrome_CacheThr > > Thread-103 > > IndexedDB > > XXXXXXX > > XXXXXXX > > XXXXXXX > > Thread-105 > > Thread-107 > > File > > Thread-111 > > Thread-113 > > XXXXXXX > > pool-2-thread-1 > > pool-3-thread-1 > > NsdManager > > Thread-107 > > Thread-107 > > Thread-107 > > Thread-107 > > Thread-107 > > Thread-107 > > ConfigFileIOWor > > SimpleCacheWork > > RenderThread > > Chrome_DevTools > > AsyncTransferTh > > JavaBridge > > Binder_3 > > thanks! is there any significant impact in terms of memory for those 38 extra > objects? > I'm tempted to say "no" compared to the amount of times AttachCurrentThread is > called... > another alternative: we could do this at the BrowserThreadImpl level: > https://cs.corp.google.com/#android/external/chromium_org/content/browser/bro... > > if you're not on android chrome, perhaps we could expose an utility so then you > can initialize it when creating the thread on your component? > wdyt? In our project, we creates thread with base::Thread, and this is targeted on non-android systems as well. Please confirm that gvm thread object is not that big to worry in https://cs.corp.google.com/#android/dalvik/vm/Thread.h&l=105 (doubt if it is okay to link corp urs though). Then, I will make another patch calling AttachCurrentThread in PlatformThread::SetName().
here's a public url :) https://android.googlesource.com/platform/dalvik.git/+/master/vm/Thread.h I haven't counted, looks like a few hundred bytes? what's your estimate? sorry if I wasn't clear: PlatformThread::SetName may also be too lower level. this is only for using the java debugger on your project, right? so how about a new helper function that could be then called by your project when it creates your threads, and optionally by BrowserThreadImpl level? then those other 38 threads that shouldn't be crossing to java anyways wouldn't create this extra object... what do you think?
On 2014/06/17 21:57:16, bulach wrote: > here's a public url :) > https://android.googlesource.com/platform/dalvik.git/+/master/vm/Thread.h > > I haven't counted, looks like a few hundred bytes? what's your estimate? > > sorry if I wasn't clear: PlatformThread::SetName may also be too lower level. > > this is only for using the java debugger on your project, right? > > so how about a new helper function that could be then called by your project > when it creates your threads, and optionally by BrowserThreadImpl level? > then those other 38 threads that shouldn't be crossing to java anyways wouldn't > create this extra object... what do you think? SGTM. Does BrowserThreadImpl cover all other threads except our own threads?
On 2014/06/18 00:07:27, byungchul wrote: > On 2014/06/17 21:57:16, bulach wrote: > > here's a public url :) > > https://android.googlesource.com/platform/dalvik.git/+/master/vm/Thread.h > > > > I haven't counted, looks like a few hundred bytes? what's your estimate? > > > > sorry if I wasn't clear: PlatformThread::SetName may also be too lower level. > > > > this is only for using the java debugger on your project, right? > > > > so how about a new helper function that could be then called by your project > > when it creates your threads, and optionally by BrowserThreadImpl level? > > then those other 38 threads that shouldn't be crossing to java anyways > wouldn't > > create this extra object... what do you think? > > SGTM. Does BrowserThreadImpl cover all other threads except our own threads? nope, that would cover only these threads: https://code.google.com/p/chromium/codesearch#chromium/src/content/public/bro... which I think are mostly the ones that would be interesting? there'd need to be a helper function somewhere to reuse in your project too...
On 2014/06/18 01:12:02, bulach wrote: > On 2014/06/18 00:07:27, byungchul wrote: > > On 2014/06/17 21:57:16, bulach wrote: > > > here's a public url :) > > > https://android.googlesource.com/platform/dalvik.git/+/master/vm/Thread.h > > > > > > I haven't counted, looks like a few hundred bytes? what's your estimate? > > > > > > sorry if I wasn't clear: PlatformThread::SetName may also be too lower > level. > > > > > > this is only for using the java debugger on your project, right? > > > > > > so how about a new helper function that could be then called by your project > > > when it creates your threads, and optionally by BrowserThreadImpl level? > > > then those other 38 threads that shouldn't be crossing to java anyways > > wouldn't > > > create this extra object... what do you think? > > > > SGTM. Does BrowserThreadImpl cover all other threads except our own threads? > > nope, that would cover only these threads: > https://code.google.com/p/chromium/codesearch#chromium/src/content/public/bro... > > which I think are mostly the ones that would be interesting? > there'd need to be a helper function somewhere to reuse in your project too... Done, and for my project, only 2 threads have "Thread-???".
PTAL. Implemented as suggested. And, verified with my project that most useful threads' names are not changed.
lgtm, thanks!! (you'll need OWNERS for browser_thread_impl.cc)
Thank you, Marcus. Scott and Antoine, could you please review this, or add an owner who can review /content files?
lgtm
https://codereview.chromium.org/330823004/diff/40001/content/browser/browser_... File content/browser/browser_thread_impl.cc (right): https://codereview.chromium.org/330823004/diff/40001/content/browser/browser_... content/browser/browser_thread_impl.cc:226: // Not to reset thread name to "Thread-???" by VM, attach VM with thread name. This comment makes no sense. https://codereview.chromium.org/330823004/diff/40001/content/browser/browser_... content/browser/browser_thread_impl.cc:230: base::android::AttachCurrentThreadWithName(thread_name()); I'm not familiar with thread_name(). Isn't it propagated to the underlying thread?
The CQ bit was checked by byungchul@chromium.org
The CQ bit was unchecked by byungchul@chromium.org
https://codereview.chromium.org/330823004/diff/40001/content/browser/browser_... File content/browser/browser_thread_impl.cc (right): https://codereview.chromium.org/330823004/diff/40001/content/browser/browser_... content/browser/browser_thread_impl.cc:226: // Not to reset thread name to "Thread-???" by VM, attach VM with thread name. On 2014/06/19 20:48:17, sky wrote: > This comment makes no sense. Otherwise, VM resets thread name. https://codereview.chromium.org/330823004/diff/40001/content/browser/browser_... content/browser/browser_thread_impl.cc:230: base::android::AttachCurrentThreadWithName(thread_name()); On 2014/06/19 20:48:17, sky wrote: > I'm not familiar with thread_name(). Isn't it propagated to the underlying > thread? Yes, but jvm's AttachCurrentThread() resets thread name to "Thread-<number>". This CL is to address that problem.
The CQ bit was checked by byungchul@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/byungchul@chromium.org/330823004/40001
FYI, CQ is re-trying this CL (attempt #1). The failing builders are: linux_chromium_chromeos_clang_dbg on tryserver.chromium (http://build.chromium.org/p/tryserver.chromium/builders/linux_chromium_chrome...)
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: linux_chromium_chromeos_clang_dbg on tryserver.chromium (http://build.chromium.org/p/tryserver.chromium/builders/linux_chromium_chrome...)
The CQ bit was checked by byungchul@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/byungchul@chromium.org/330823004/40001
Message was sent while issue was closed.
Change committed as 278551 |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
