Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1958703002:
[tracing] Fix mising thread names in heap profiler (Closed)
Description[tracing] Fix mising thread names in heap profiler
The thread name goes missing in cases where the heap profiling is
enabled after SetThreadName is called. To fix this, the tracker now
tries to get the thread name from prctl sys call. Both Linux and Android
sets the thread name using prctl. The string allocated for thread name
in tracker needs to be guarded for rentracncy.
This CL also makes thread name as default first entry in the pseudo
stack with thread id if unable to get thread name.
Removes thread name unittest since other unittests cover these cases.
BUG=609912
Committed: https://crrev.com/94291d6f25efbd4c28db5d6ba859cfcec61dc664
Cr-Commit-Position: refs/heads/master@{#394416}
Patch Set 1 : #Patch Set 2 : Use prctl. #Patch Set 3 : fix unittest. #
Total comments: 6
Patch Set 4 : Use strdup #Patch Set 5 : Use thread id. #
Total comments: 4
Patch Set 6 : Array size and number format. #
Total comments: 8
Patch Set 7 : Rename and annotate. #
Messages
Total messages: 33 (15 generated)
Patchset #1 (id:1) has been deleted
Description was changed from ========== [tracing] Store thread name in different TLS slot The thread name goes missing in cases where the heap profiling is enabled after SetThreadName is called. This happens to all the threads that start before receiving the enable tracing ipc in renderers and a few threads in browser as well. To initialize the thread name and avoid having to construct the whole ContextTracker object, a new tls slot is defined that stores just the thread name. This gets initialized irrespective of heap profiler being enabled. BUG=609912 ========== to ========== [tracing] Store thread name in separate TLS slot for heap profiler The thread name goes missing in cases where the heap profiling is enabled after SetThreadName is called. This happens to all the threads that start before receiving the enable tracing ipc in renderers and a few threads in browser as well. To initialize the thread name and avoid having to construct the whole ContextTracker object, a new tls slot is defined that stores just the thread name. This gets initialized irrespective of heap profiler being enabled. BUG=609912 ==========
ssid@chromium.org changed reviewers: + primiano@chromium.org
ptal, thanks.
Hmm I'd try to avoid adding too many TLS. THey are a scarce resource and we should not abusing them. I am missing something, the heap profiler is enabled via command line at startup, how come we don't see it? Isn't the right fix just a matter of anticipating the cmdline parsing before the thread get created? P.S: Also this patch seems out of sync with master
On 2016/05/09 17:21:54, Primiano Tucci wrote: > Hmm I'd try to avoid adding too many TLS. THey are a scarce resource and we > should not abusing them. Ok, I thought more about it. Maybe we should fix when capture is enabled. > I am missing something, the heap profiler is enabled via command line at > startup, how come we don't see it? > Isn't the right fix just a matter of anticipating the cmdline parsing before the > thread get created? So this is the sequence of events that is usually seen on android: Browser: thread created CrBrowserMain, capture_mode 0 thread created DnsConfigService, capture_mode 0 thread created WorkerPool/4517, capture_mode 0 mdm init mode=0 - set capture mode enabled Renderer: thread created CrRendererMain, capture_mode 0 thread created Chrome_ChildIOThread, capture_mode 0 thread created Compositor,adb logca capture_mode 0 setting trace filter 0 thread created CompositorTileWorker1/4741 capture_mode 0 thread created CompositorTileWorkerBackground/4742 capture_mode 0 mdm init mode=0 - set capture mode enabled This happens because the heap profiling is enabled late on android. On linux it starts with the initialization of TraceLog (at ChromeMainDelegate::BasicStartupComplete). But on android command line is initialized only when library load happens. So capturing is enabled late - after creating 4 threads. See https://chromium.googlesource.com/chromium/src/+blame/master/content/app/cont.... I think the issue here is that the command line is initialized little late, and heap profiling is enabled even later when MDM::Initialize happens. Maybe the solution here is to initialize mdm earlier like ContentMainRunnerImpl::Initialize instead of waiting for ChildMemoryDumpManagerDelegateImpl::SetChildTraceMessageFilter. > P.S: Also this patch seems out of sync with master Sorry, will fix this.
Patchset #2 (id:40001) has been deleted
Description was changed from ========== [tracing] Store thread name in separate TLS slot for heap profiler The thread name goes missing in cases where the heap profiling is enabled after SetThreadName is called. This happens to all the threads that start before receiving the enable tracing ipc in renderers and a few threads in browser as well. To initialize the thread name and avoid having to construct the whole ContextTracker object, a new tls slot is defined that stores just the thread name. This gets initialized irrespective of heap profiler being enabled. BUG=609912 ========== to ========== [tracing] Fix mising thread names in heap profiler The thread name goes missing in cases where the heap profiling is enabled after SetThreadName is called. To fix this, the tracker now tries to get the thread name from prctl sys call. Both Linux and Android sets the thread name using prctl. The string allocated for thread name in tracker needs to be guarded for rentracncy. This CL also makes thread name as default first entry in the pseudo stack with "Unknown" if unable to get thread name. Removes thread name unittest since other unittests cover these cases. BUG=609912 ==========
So, I updated the CL to get the name from prctl calls. It is not possible to enable heap profiling earlier. It is possible to store the thread names even when heap profiling is not enabled in 2 ways: 1. Use tls. But tls is scarce resource. 2. Use a map to store in global (maybe in MDM). But, this should be a tid -> name map and this should be kept alive. Also the tid can be reused by the system and it will start displaying the wrong names. For same reasons we can't store a map of tracker -> thread name. To fix this issue I could also store a copy of the pointer in tracker and initialize when needed from MDM: this would mean a member per tracker and a map kept alive forever in MDM and a lock for the map (which is scary). The other option would be to not care about setting the thread names and always get from prctl - which would mean we are leaking one more string for thread name per thread. So, this current version sounds like the better option. WDYT?
dskiba@google.com changed reviewers: + dskiba@google.com
https://codereview.chromium.org/1958703002/diff/80001/base/trace_event/heap_p... File base/trace_event/heap_profiler_allocation_context_tracker.cc (right): https://codereview.chromium.org/1958703002/diff/80001/base/trace_event/heap_p... base/trace_event/heap_profiler_allocation_context_tracker.cc:154: // Ignore the string allocation and allocations from prctl to avoid prctl is a syscall, there can be no reentrancy. https://codereview.chromium.org/1958703002/diff/80001/base/trace_event/heap_p... base/trace_event/heap_profiler_allocation_context_tracker.cc:164: thread_name_ = "Unknown"; Let's format thread id instead. Hmm, and I think snprintf() can actually allocate, so you might need reentrancy check anyway. https://codereview.chromium.org/1958703002/diff/80001/base/trace_event/heap_p... base/trace_event/heap_profiler_allocation_context_tracker.cc:165: free(leaked_str); Use 'delete []' here since you allocated with new. Better approach might be to start with stack array and then strdup() it on assignment to thread_name_.
Thanks, see replies inline. https://codereview.chromium.org/1958703002/diff/80001/base/trace_event/heap_p... File base/trace_event/heap_profiler_allocation_context_tracker.cc (right): https://codereview.chromium.org/1958703002/diff/80001/base/trace_event/heap_p... base/trace_event/heap_profiler_allocation_context_tracker.cc:154: // Ignore the string allocation and allocations from prctl to avoid On 2016/05/13 18:34:06, Dmitry Skiba wrote: > prctl is a syscall, there can be no reentrancy. Ah yes, I thought it was causing reentrancy too. Fixed comment https://codereview.chromium.org/1958703002/diff/80001/base/trace_event/heap_p... base/trace_event/heap_profiler_allocation_context_tracker.cc:164: thread_name_ = "Unknown"; On 2016/05/13 18:34:06, Dmitry Skiba wrote: > Let's format thread id instead. Hmm, and I think snprintf() can actually > allocate, so you might need reentrancy check anyway. Hm, do you think it is useful to see a thread id instead of unknown? This also causes an extra string leak and branching in the view. https://codereview.chromium.org/1958703002/diff/80001/base/trace_event/heap_p... base/trace_event/heap_profiler_allocation_context_tracker.cc:165: free(leaked_str); On 2016/05/13 18:34:06, Dmitry Skiba wrote: > Use 'delete []' here since you allocated with new. Better approach might be to > start with stack array and then strdup() it on assignment to thread_name_. Yes, fixed.
Added thread ids if we dont have names.
Patchset #5 (id:120001) has been deleted
https://codereview.chromium.org/1958703002/diff/140001/base/trace_event/heap_... File base/trace_event/heap_profiler_allocation_context_tracker.cc (right): https://codereview.chromium.org/1958703002/diff/140001/base/trace_event/heap_... base/trace_event/heap_profiler_allocation_context_tracker.cc:45: char name[50]; Why 50? PRCTL needs 16 and longest thing %d (%lld) can print is 22 (with null). https://codereview.chromium.org/1958703002/diff/140001/base/trace_event/heap_... base/trace_event/heap_profiler_allocation_context_tracker.cc:57: snprintf(name, 50, "%x", PlatformThread::CurrentId()); Better use sizeof(name) instead of 50. Also, I think the convention for TIDs is that they are decimal, I've never seen them formatted as hex. Also I would explicitly cast CurrentId() to some certain type, like long. Right now you're fine only on platforms where CurrentId() is int.
Description was changed from ========== [tracing] Fix mising thread names in heap profiler The thread name goes missing in cases where the heap profiling is enabled after SetThreadName is called. To fix this, the tracker now tries to get the thread name from prctl sys call. Both Linux and Android sets the thread name using prctl. The string allocated for thread name in tracker needs to be guarded for rentracncy. This CL also makes thread name as default first entry in the pseudo stack with "Unknown" if unable to get thread name. Removes thread name unittest since other unittests cover these cases. BUG=609912 ========== to ========== [tracing] Fix mising thread names in heap profiler The thread name goes missing in cases where the heap profiling is enabled after SetThreadName is called. To fix this, the tracker now tries to get the thread name from prctl sys call. Both Linux and Android sets the thread name using prctl. The string allocated for thread name in tracker needs to be guarded for rentracncy. This CL also makes thread name as default first entry in the pseudo stack with thread id if unable to get thread name. Removes thread name unittest since other unittests cover these cases. BUG=609912 ==========
Patchset #6 (id:160001) has been deleted
https://codereview.chromium.org/1958703002/diff/140001/base/trace_event/heap_... File base/trace_event/heap_profiler_allocation_context_tracker.cc (right): https://codereview.chromium.org/1958703002/diff/140001/base/trace_event/heap_... base/trace_event/heap_profiler_allocation_context_tracker.cc:45: char name[50]; On 2016/05/16 21:11:10, Dmitry Skiba wrote: > Why 50? PRCTL needs 16 and longest thing %d (%lld) can print is 22 (with null). Used unsigned long - maximum is 12 chars. https://codereview.chromium.org/1958703002/diff/140001/base/trace_event/heap_... base/trace_event/heap_profiler_allocation_context_tracker.cc:57: snprintf(name, 50, "%x", PlatformThread::CurrentId()); On 2016/05/16 21:11:10, Dmitry Skiba wrote: > Better use sizeof(name) instead of 50. Also, I think the convention for TIDs is > that they are decimal, I've never seen them formatted as hex. Also I would > explicitly cast CurrentId() to some certain type, like long. Right now you're > fine only on platforms where CurrentId() is int. I was thinking dword was int32 so %x should still work. Fixed. I thought %x is more compact, so used.
Remind me one thing, why don't we use ThreadIdNameManager::GetName ? I think we had this discussion in the past, but I don't remember anymore what was the conclusion. Also, do you still ned the SetCurrentThreadName after this? https://codereview.chromium.org/1958703002/diff/180001/base/trace_event/heap_... File base/trace_event/heap_profiler_allocation_context_tracker.cc (right): https://codereview.chromium.org/1958703002/diff/180001/base/trace_event/heap_... base/trace_event/heap_profiler_allocation_context_tracker.cc:44: const char* GetThreadName() { can you make this a void GetThreadName(char* out_name, size_t size) and move the strdup down where you use it? I can be ok with the strdup but I fear that in future somebody else calls this and we multiply the leaks. https://codereview.chromium.org/1958703002/diff/180001/base/trace_event/heap_... base/trace_event/heap_profiler_allocation_context_tracker.cc:52: return strdup(name); once you move this, make sure you include leak_annotations.h and use ANNOTATE_LEAKING_OBJECT_PTR(duped_str_ptr). Otherwise your patch will be reverted as soon as it hits the asan bots :) https://codereview.chromium.org/1958703002/diff/180001/base/trace_event/heap_... base/trace_event/heap_profiler_allocation_context_tracker.cc:59: return strdup(name); instead of strduping twice what you want to do below is: if(!thread_name_) { thread_name = new char[16]; ANNOTATE_LEAKING_OBJECT_PTR(thread_name_); GetThreadName(&thread_name, sizeof(thread_name)) }
https://codereview.chromium.org/1958703002/diff/180001/base/trace_event/heap_... File base/trace_event/heap_profiler_allocation_context_tracker.cc (right): https://codereview.chromium.org/1958703002/diff/180001/base/trace_event/heap_... base/trace_event/heap_profiler_allocation_context_tracker.cc:44: const char* GetThreadName() { On 2016/05/17 19:38:43, Primiano Tucci wrote: > can you make this a void GetThreadName(char* out_name, size_t size) and move the > strdup down where you use it? > I can be ok with the strdup but I fear that in future somebody else calls this > and we multiply the leaks. Let's simply call function LeakThreadName(), and put annotation there? Right now function knows how much buffer space it needs, by adding name/size arguments you require callers to care about function's internals (to justify 16 in your example below).
https://codereview.chromium.org/1958703002/diff/180001/base/trace_event/heap_... File base/trace_event/heap_profiler_allocation_context_tracker.cc (right): https://codereview.chromium.org/1958703002/diff/180001/base/trace_event/heap_... base/trace_event/heap_profiler_allocation_context_tracker.cc:44: const char* GetThreadName() { On 2016/05/17 20:05:18, Dmitry Skiba wrote: > On 2016/05/17 19:38:43, Primiano Tucci wrote: > > can you make this a void GetThreadName(char* out_name, size_t size) and move > the > > strdup down where you use it? > > I can be ok with the strdup but I fear that in future somebody else calls this > > and we multiply the leaks. > > Let's simply call function LeakThreadName(), and put annotation there? Right now > function knows how much buffer space it needs, by adding name/size arguments you > require callers to care about function's internals (to justify 16 in your > example below). You could pass a std::string* as arugment and leak that. Anyways LGTM both ways (either with the Leak in the name or leaking a std::string). You guys figure out. Just remember the annotation :)
Thanks for the review. Added leak in the name and added leak annotation. https://codereview.chromium.org/1958703002/diff/180001/base/trace_event/heap_... File base/trace_event/heap_profiler_allocation_context_tracker.cc (right): https://codereview.chromium.org/1958703002/diff/180001/base/trace_event/heap_... base/trace_event/heap_profiler_allocation_context_tracker.cc:44: const char* GetThreadName() { On 2016/05/17 20:46:12, Primiano Tucci wrote: > On 2016/05/17 20:05:18, Dmitry Skiba wrote: > > On 2016/05/17 19:38:43, Primiano Tucci wrote: > > > can you make this a void GetThreadName(char* out_name, size_t size) and move > > the > > > strdup down where you use it? > > > I can be ok with the strdup but I fear that in future somebody else calls > this > > > and we multiply the leaks. > > > > Let's simply call function LeakThreadName(), and put annotation there? Right > now > > function knows how much buffer space it needs, by adding name/size arguments > you > > require callers to care about function's internals (to justify 16 in your > > example below). > > You could pass a std::string* as arugment and leak that. > Anyways LGTM both ways (either with the Leak in the name or leaking a > std::string). > You guys figure out. Just remember the annotation :) Changed the name and added ANNOTATE_LEAKING_OBJECT_PTR. https://codereview.chromium.org/1958703002/diff/180001/base/trace_event/heap_... base/trace_event/heap_profiler_allocation_context_tracker.cc:52: return strdup(name); On 2016/05/17 19:38:44, Primiano Tucci wrote: > once you move this, make sure you include leak_annotations.h and use > ANNOTATE_LEAKING_OBJECT_PTR(duped_str_ptr). > Otherwise your patch will be reverted as soon as it hits the asan bots :) Done. https://codereview.chromium.org/1958703002/diff/180001/base/trace_event/heap_... base/trace_event/heap_profiler_allocation_context_tracker.cc:59: return strdup(name); On 2016/05/17 19:38:43, Primiano Tucci wrote: > instead of strduping twice what you want to do below is: > > if(!thread_name_) { > thread_name = new char[16]; > ANNOTATE_LEAKING_OBJECT_PTR(thread_name_); > GetThreadName(&thread_name, sizeof(thread_name)) > } Keepin strdup twice.
The CQ bit was checked by ssid@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1958703002/200001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1958703002/200001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: win_chromium_compile_dbg_ng on tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_comp...)
The CQ bit was checked by primiano@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from primiano@chromium.org Link to the patchset: https://codereview.chromium.org/1958703002/#ps200001 (title: "Rename and annotate.")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1958703002/200001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1958703002/200001
Message was sent while issue was closed.
Description was changed from ========== [tracing] Fix mising thread names in heap profiler The thread name goes missing in cases where the heap profiling is enabled after SetThreadName is called. To fix this, the tracker now tries to get the thread name from prctl sys call. Both Linux and Android sets the thread name using prctl. The string allocated for thread name in tracker needs to be guarded for rentracncy. This CL also makes thread name as default first entry in the pseudo stack with thread id if unable to get thread name. Removes thread name unittest since other unittests cover these cases. BUG=609912 ========== to ========== [tracing] Fix mising thread names in heap profiler The thread name goes missing in cases where the heap profiling is enabled after SetThreadName is called. To fix this, the tracker now tries to get the thread name from prctl sys call. Both Linux and Android sets the thread name using prctl. The string allocated for thread name in tracker needs to be guarded for rentracncy. This CL also makes thread name as default first entry in the pseudo stack with thread id if unable to get thread name. Removes thread name unittest since other unittests cover these cases. BUG=609912 ==========
Message was sent while issue was closed.
Committed patchset #7 (id:200001)
Message was sent while issue was closed.
Description was changed from ========== [tracing] Fix mising thread names in heap profiler The thread name goes missing in cases where the heap profiling is enabled after SetThreadName is called. To fix this, the tracker now tries to get the thread name from prctl sys call. Both Linux and Android sets the thread name using prctl. The string allocated for thread name in tracker needs to be guarded for rentracncy. This CL also makes thread name as default first entry in the pseudo stack with thread id if unable to get thread name. Removes thread name unittest since other unittests cover these cases. BUG=609912 ========== to ========== [tracing] Fix mising thread names in heap profiler The thread name goes missing in cases where the heap profiling is enabled after SetThreadName is called. To fix this, the tracker now tries to get the thread name from prctl sys call. Both Linux and Android sets the thread name using prctl. The string allocated for thread name in tracker needs to be guarded for rentracncy. This CL also makes thread name as default first entry in the pseudo stack with thread id if unable to get thread name. Removes thread name unittest since other unittests cover these cases. BUG=609912 Committed: https://crrev.com/94291d6f25efbd4c28db5d6ba859cfcec61dc664 Cr-Commit-Position: refs/heads/master@{#394416} ==========
Message was sent while issue was closed.
Patchset 7 (id:??) landed as https://crrev.com/94291d6f25efbd4c28db5d6ba859cfcec61dc664 Cr-Commit-Position: refs/heads/master@{#394416} |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
