Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1814083002:
[tracing] Add thread name to the pseudo stack of heap profiler (Closed)
Description[tracing] Add thread name to the pseudo stack of heap profiler
The thread name that allocated the memory is very useful to understand
the memory usage of the process. This CL adds it to the top of the
pseudo stack trace.
The name is set from ThreadIdNameManager since it leaks the thread name.
Since getting the name from AllocationContextTracker can cause rentrancy
into ThreadIdNameManager, the name is passed from ThreadIdNameManager.
BUG=594803
Committed: https://crrev.com/b3fabc66203904dd110597b11ebb89b89697f1a9
Cr-Commit-Position: refs/heads/master@{#384484}
Patch Set 1 : nit. #
Total comments: 7
Patch Set 2 : rebase. #
Total comments: 12
Patch Set 3 : nits. #
Total comments: 5
Patch Set 4 : Add comment. #
Depends on Patchset: Dependent Patchsets: Messages
Total messages: 55 (17 generated)
ssid@chromium.org changed reviewers: + primiano@chromium.org
I have split the CL https://codereview.chromium.org/1784133002/ and this one just adds thread name.
Good choice about splitting the CL, thanks. Adding the thread name to the top of the heap profiler stack SGTM. But I have a question about the execution here: why do we do any change to thread_id_name_manager.cc ? Can't we just query the TINM when we construct the ContextTracker by doing: ThreadIDNameManager::GetInstance()::GetName(GetCurrentThreadID()) ? and push that into the pseudo stack?
On 2016/03/21 20:26:17, Primiano (traveling) wrote: > Good choice about splitting the CL, thanks. > Adding the thread name to the top of the heap profiler stack SGTM. > But I have a question about the execution here: why do we do any change to > thread_id_name_manager.cc ? > Can't we just query the TINM when we construct the ContextTracker by doing: > ThreadIDNameManager::GetInstance()::GetName(GetCurrentThreadID()) ? > and push that into the pseudo stack? Trying to call ThreadIdNameManager::getName during AllocationTracker initialization may cause a deadlock since thread registration causes the first allocation allocation. Both registration and GetName holds a lock.
On 2016/03/21 21:01:54, ssid wrote: > On 2016/03/21 20:26:17, Primiano (traveling) wrote: > > Good choice about splitting the CL, thanks. > > Adding the thread name to the top of the heap profiler stack SGTM. > > But I have a question about the execution here: why do we do any change to > > thread_id_name_manager.cc ? > > Can't we just query the TINM when we construct the ContextTracker by doing: > > ThreadIDNameManager::GetInstance()::GetName(GetCurrentThreadID()) ? > > and push that into the pseudo stack? > > Trying to call ThreadIdNameManager::getName during AllocationTracker > initialization may cause a deadlock since thread registration causes the first > allocation allocation. Both registration and GetName holds a lock. Does it happen also after my last CL (https://codereview.chromium.org/1675183006/) where the AllocationTracker deals with reentrancy?
On 2016/03/22 13:46:23, Primiano (traveling) wrote: > On 2016/03/21 21:01:54, ssid wrote: > > On 2016/03/21 20:26:17, Primiano (traveling) wrote: > > > Good choice about splitting the CL, thanks. > > > Adding the thread name to the top of the heap profiler stack SGTM. > > > But I have a question about the execution here: why do we do any change to > > > thread_id_name_manager.cc ? > > > Can't we just query the TINM when we construct the ContextTracker by doing: > > > ThreadIDNameManager::GetInstance()::GetName(GetCurrentThreadID()) ? > > > and push that into the pseudo stack? > > > > Trying to call ThreadIdNameManager::getName during AllocationTracker > > initialization may cause a deadlock since thread registration causes the first > > allocation allocation. Both registration and GetName holds a lock. > > Does it happen also after my last CL > (https://codereview.chromium.org/1675183006/) where the AllocationTracker deals > with reentrancy? It is not an issue with reentrancy in AllocationTracker, but it is an issue with ThreadIdNameManager. ThreadIdNameManager uses lock and allocates memory, which causes AllocationTracker initialization. If I callback into ThreadIdNameManager again for getting name during initialization, it causes a deadlock. ThreadIdNameManager makes the first memory allocation. Flow is: ThreadIdNameManager::Register() - holds thread_id_lock. -> new manager() -> AllocationTracker::Insert() -> AllocationTracker::Initialize() -> ThreadIdNameManager::GetName() - tries to hold thread_id_lock.
Description was changed from ========== [tracing] Add thread name to the pseudo stack of heap profiler The thread name that allocated the memory is very useful to understand the memory usage of the process. This CL adds it to the top of the pseudo stack trace. BUG=594803 ========== to ========== [tracing] Add thread name to the pseudo stack of heap profiler The thread name that allocated the memory is very useful to understand the memory usage of the process. This CL adds it to the top of the pseudo stack trace. BUG=594803 ==========
ssid@chromium.org changed reviewers: + dskiba@chromium.org
Ok there are two less invasive approaches that come to my mind:
1) - In AllocationContext add a "ThreadId thread_id" field (keep only the int,
not the full name)
- In HeapDumpWriter::Summarize do the id -> name resolution, ad treat the
name as an actual stack level
PRO: does not require any change in base and is pretty clean
CON: If a thread disappears before you stop tracing, you will lose the name
of that thread (realistically should be pretty rate)
2) - Add a char thread_name[32] to the AllocationContextTracker.
- In the GetContextSnapshot(), copy the pseudo-stack at offset +1, push
the pointer to thread_name at offset 0 in the context
- Change PlatformThread::SetName (in platform_thread_linux, win, mac etc) To
add a call to ContextTracker::GetInstanceForCurrentThread()->SetName. There is
precent (The object tracker) to do that, so should be fine to add one line to
those files.
PRO: even if the thread dies before dumping you still get the name
CON: might require some base owners dance.
I'd go for 2 first, and fall back to 1 if you get pushback
ssid@chromium.org changed reviewers: + petrcermak@chromium.org
Hi Petr, can I ask you for a few reviews when primiano@ is ooo? We have already discussed about this change and we agreed on adding the thread name to the stack. Can you please take a look at trace event changes? thanks.
https://codereview.chromium.org/1814083002/diff/80001/base/threading/thread_i... File base/threading/thread_id_name_manager.cc (right): https://codereview.chromium.org/1814083002/diff/80001/base/threading/thread_i... base/threading/thread_id_name_manager.cc:80: trace_event::AllocationContextTracker::SetCurrentThreadName( can you rebase this on top of my reland crrev.com/1831763003?
https://codereview.chromium.org/1814083002/diff/80001/base/trace_event/heap_p... File base/trace_event/heap_profiler_allocation_context_tracker.cc (right): https://codereview.chromium.org/1814083002/diff/80001/base/trace_event/heap_p... base/trace_event/heap_profiler_allocation_context_tracker.cc:58: void AllocationContextTracker::SetCurrentThreadName(const char* name) { after me cl lands this should be just sa call to te method instance https://codereview.chromium.org/1814083002/diff/80001/base/trace_event/heap_p... base/trace_event/heap_profiler_allocation_context_tracker.cc:115: *src++ = thread_name_; are you not clobbering the first valid stack entry by doing this?
https://codereview.chromium.org/1814083002/diff/80001/base/trace_event/heap_p... File base/trace_event/heap_profiler_allocation_context_tracker.cc (right): https://codereview.chromium.org/1814083002/diff/80001/base/trace_event/heap_p... base/trace_event/heap_profiler_allocation_context_tracker.cc:58: void AllocationContextTracker::SetCurrentThreadName(const char* name) { On 2016/03/25 02:14:30, Primiano (traveling) wrote: > after me cl lands this should be just sa call to te method instance Yes i did not want this to be a member because i would have to move this check outside. so at thread id manager i will write, if capture_enabled then get instance and set name. More code in threadIdManager and it need not be there. Also, if i have to move this part into Platformthread if base owners didn't agree then i will have to copy this if condition to 5 places. So, better to have the if here. If I do not check for capture_enabled before getting the instance, then i will end up creating the tracker even if heap profiling is not enabled, causing all the allocations(comes out ~1Kib per thread). https://codereview.chromium.org/1814083002/diff/80001/base/trace_event/heap_p... base/trace_event/heap_profiler_allocation_context_tracker.cc:115: *src++ = thread_name_; On 2016/03/25 02:14:31, Primiano (traveling) wrote: > are you not clobbering the first valid stack entry by doing this? Sorry I meant dst++ here!
//base/trace_event LGTM with comment addressed. Also would be nice to cover this new feature in the unittest https://codereview.chromium.org/1814083002/diff/80001/base/trace_event/heap_p... File base/trace_event/heap_profiler_allocation_context_tracker.cc (right): https://codereview.chromium.org/1814083002/diff/80001/base/trace_event/heap_p... base/trace_event/heap_profiler_allocation_context_tracker.cc:58: void AllocationContextTracker::SetCurrentThreadName(const char* name) { On 2016/03/25 05:57:09, ssid wrote: > On 2016/03/25 02:14:30, Primiano (traveling) wrote: > > after me cl lands this should be just sa call to te method instance > > Yes i did not want this to be a member because i would have to move this check > outside. so at thread id manager i will write, > if capture_enabled then get instance and set name. > More code in threadIdManager and it need not be there. Also, if i have to move > this part into Platformthread if base owners didn't agree then i will have to > copy this if condition to 5 places. So, better to have the if here. > > If I do not check for capture_enabled before getting the instance, then i will > end up creating the tracker even if heap profiling is not enabled, causing all > the allocations(comes out ~1Kib per thread). Ahh ok makes sense. Although our code style [1] seems to state "For iterators and other template types, use pre-increment", So I think you want to split that into two lines. [1] https://google.github.io/styleguide/cppguide.html#Preincrement_and_Predecrement https://codereview.chromium.org/1814083002/diff/80001/base/trace_event/heap_p... base/trace_event/heap_profiler_allocation_context_tracker.cc:115: *src++ = thread_name_; On 2016/03/25 05:57:09, ssid wrote: > On 2016/03/25 02:14:31, Primiano (traveling) wrote: > > are you not clobbering the first valid stack entry by doing this? > > Sorry I meant dst++ here! Ah right, that makes more sense :)
ssid@chromium.org changed reviewers: + danakj@chromium.org
+danakj for base/threading/thread_id_name_manager.cc. 3 lines added. Firstly I need a string that is long lived and not owned by the allocation tracker. Usually we use const string literals. Here I am using the thread name that is leaked by ThreadIdNameManager. Other option would be to create one more string at the tracker and leak it. I would prefer already leaked string. I could also use the string from tracked_objects::ThreadData, but ThreadIdNameManager works better with the testing threads. I cannot call into the ThreadIdNameManager from AllocationContextTracker and get the name because it can cause deadlocks since ThreadIdNameManager uses a lock. So, I have to set the name from ThreadIdNameManager to tracker. But the ThreadIdNameManager::SetName need not be called on the same thread. Today it is always called by the same thread only in PlatformThread::SetName. So I could just set it from ThreadIdNameManager. If you prefer it to be moved to Platformthread::SetName I would have to change 5 platform specific files. So, i am chaning here. WDYT?
https://codereview.chromium.org/1814083002/diff/100001/base/threading/thread_... File base/threading/thread_id_name_manager.cc (right): https://codereview.chromium.org/1814083002/diff/100001/base/threading/thread_... base/threading/thread_id_name_manager.cc:80: trace_event::AllocationContextTracker::SetCurrentThreadName( Why not call GetName() when you need the name?
On 2016/03/29 00:47:36, danakj wrote: > https://codereview.chromium.org/1814083002/diff/100001/base/threading/thread_... > File base/threading/thread_id_name_manager.cc (right): > > https://codereview.chromium.org/1814083002/diff/100001/base/threading/thread_... > base/threading/thread_id_name_manager.cc:80: > trace_event::AllocationContextTracker::SetCurrentThreadName( > Why not call GetName() when you need the name? I cannot call GetName from AllocationTracker because it could cause a deadlock at RegisterThread. AllocationTracker gets initialized at first allocation in the thread (where I should try to call GetName). The first allocation more often happens in RegisterThread. So, calling GetName tries to hold same lock as RegisterThread.
On 2016/03/29 00:50:26, ssid wrote: > On 2016/03/29 00:47:36, danakj wrote: > > > https://codereview.chromium.org/1814083002/diff/100001/base/threading/thread_... > > File base/threading/thread_id_name_manager.cc (right): > > > > > https://codereview.chromium.org/1814083002/diff/100001/base/threading/thread_... > > base/threading/thread_id_name_manager.cc:80: > > trace_event::AllocationContextTracker::SetCurrentThreadName( > > Why not call GetName() when you need the name? > > I cannot call GetName from AllocationTracker because it could cause a deadlock > at RegisterThread. > AllocationTracker gets initialized at first allocation in the thread (where I > should try to call GetName). The first allocation more often happens in > RegisterThread. > So, calling GetName tries to hold same lock as RegisterThread. See also comment #17. Thanks. (https://codereview.chromium.org/1814083002/#msg17)
A couple of comments. Thanks, Petr https://codereview.chromium.org/1814083002/diff/100001/base/threading/thread_... File base/threading/thread_id_name_manager.cc (right): https://codereview.chromium.org/1814083002/diff/100001/base/threading/thread_... base/threading/thread_id_name_manager.cc:78: // Add the leaked thread name to heap profiler context tracker. The name added Shouldn't you also do this for the main thread? https://codereview.chromium.org/1814083002/diff/100001/base/trace_event/heap_... File base/trace_event/heap_profiler_allocation_context_tracker.cc (right): https://codereview.chromium.org/1814083002/diff/100001/base/trace_event/heap_... base/trace_event/heap_profiler_allocation_context_tracker.cc:115: *dst = thread_name_; I think that you should DCHECK that dst < dst_end here. https://codereview.chromium.org/1814083002/diff/100001/base/trace_event/heap_... File base/trace_event/heap_profiler_allocation_context_tracker.h (right): https://codereview.chromium.org/1814083002/diff/100001/base/trace_event/heap_... base/trace_event/heap_profiler_allocation_context_tracker.h:51: // Set the thread name in the AllocationContextTracker of current thread if nit: s/current/the current/ https://codereview.chromium.org/1814083002/diff/100001/content/browser/browse... File content/browser/browser_main_runner.cc (right): https://codereview.chromium.org/1814083002/diff/100001/content/browser/browse... content/browser/browser_main_runner.cc:69: base::trace_event::AllocationContextTracker::SetCurrentThreadName( I think that it would be better to do this in ThreadIdNameManager::SetName. After all, you already pass the thread name on line 68. That way, the code path for setting the current thread name through AllocationContextTracker will be the same on all processes.
thanks, made changes / replied inline. https://codereview.chromium.org/1814083002/diff/100001/base/threading/thread_... File base/threading/thread_id_name_manager.cc (right): https://codereview.chromium.org/1814083002/diff/100001/base/threading/thread_... base/threading/thread_id_name_manager.cc:78: // Add the leaked thread name to heap profiler context tracker. The name added On 2016/03/29 13:07:46, petrcermak wrote: > Shouldn't you also do this for the main thread? replied below. https://codereview.chromium.org/1814083002/diff/100001/base/threading/thread_... base/threading/thread_id_name_manager.cc:80: trace_event::AllocationContextTracker::SetCurrentThreadName( On 2016/03/29 00:47:36, danakj wrote: > Why not call GetName() when you need the name? I cannot call GetName from AllocationTracker because it could cause a deadlock at RegisterThread. AllocationTracker gets initialized at first allocation in the thread (where I should try to call GetName). The first allocation more often happens in RegisterThread. So, calling GetName tries to hold same lock as RegisterThread. https://codereview.chromium.org/1814083002/diff/100001/base/trace_event/heap_... File base/trace_event/heap_profiler_allocation_context_tracker.cc (right): https://codereview.chromium.org/1814083002/diff/100001/base/trace_event/heap_... base/trace_event/heap_profiler_allocation_context_tracker.cc:115: *dst = thread_name_; On 2016/03/29 13:07:46, petrcermak wrote: > I think that you should DCHECK that dst < dst_end here. Done. https://codereview.chromium.org/1814083002/diff/100001/base/trace_event/heap_... File base/trace_event/heap_profiler_allocation_context_tracker.h (right): https://codereview.chromium.org/1814083002/diff/100001/base/trace_event/heap_... base/trace_event/heap_profiler_allocation_context_tracker.h:51: // Set the thread name in the AllocationContextTracker of current thread if On 2016/03/29 13:07:46, petrcermak wrote: > nit: s/current/the current/ Done. https://codereview.chromium.org/1814083002/diff/100001/content/browser/browse... File content/browser/browser_main_runner.cc (right): https://codereview.chromium.org/1814083002/diff/100001/content/browser/browse... content/browser/browser_main_runner.cc:69: base::trace_event::AllocationContextTracker::SetCurrentThreadName( On 2016/03/29 13:07:47, petrcermak wrote: > I think that it would be better to do this in ThreadIdNameManager::SetName. > After all, you already pass the thread name on line 68. That way, the code path > for setting the current thread name through AllocationContextTracker will be the > same on all processes. Hm no. The main thread name is never set in ThreadIdNameManager. The line 68 refers to ThreadData which is different from ThreadIdNameManager. I cannot use ThreadData because it doesn't get set in tests, so easier to use ThreadIdNameManager (prefered by primiano@). So, I do not want to register the main thread in ThreadIdNameManager for this reason, so setting it separately.
LGTM (assuming you're not expecting me to parse the categories in Trace Viewer until they are properly in the trace, i.e. without piggybacking on type names). Thanks, PEtr
On 2016/03/29 18:37:17, petrcermak wrote: > LGTM (assuming you're not expecting me to parse the categories in Trace Viewer > until they are properly in the trace, i.e. without piggybacking on type names). > > Thanks, > PEtr I think you got commented on wrong CL. The parsing is done in https://codereview.chromium.org/1784133002/ And yes, no TV change is required for any of these changes till we get the context dimension.
On 2016/03/29 18:39:42, ssid wrote: > On 2016/03/29 18:37:17, petrcermak wrote: > > LGTM (assuming you're not expecting me to parse the categories in Trace Viewer > > until they are properly in the trace, i.e. without piggybacking on type > names). > > > > Thanks, > > PEtr > > I think you got commented on wrong CL. You are absolutely right O:-) Nevertheless, still LGTM ;-)
https://codereview.chromium.org/1814083002/diff/100001/base/threading/thread_... File base/threading/thread_id_name_manager.cc (right): https://codereview.chromium.org/1814083002/diff/100001/base/threading/thread_... base/threading/thread_id_name_manager.cc:80: trace_event::AllocationContextTracker::SetCurrentThreadName( On 2016/03/29 18:19:11, ssid wrote: > On 2016/03/29 00:47:36, danakj wrote: > > Why not call GetName() when you need the name? > > I cannot call GetName from AllocationTracker because it could cause a deadlock > at RegisterThread. > AllocationTracker gets initialized at first allocation in the thread (where I > should try to call GetName). The first allocation more often happens in > RegisterThread. > So, calling GetName tries to hold same lock as RegisterThread. Sorry I'm not understanding this, but why is it a deadlock? If RegisterThread and GetName happen at the same time, then one will wait for the other and then continue. Deadlocks occur when 2 locks are held in different orders and one lock ends up waiting on the other lock which waits on the first lock. I only see 1 lock in your explanation tho, which would just provide single access at a time? It sounded like you're worried about RegisterThread and GetName happening from the same thread at the same time? One does not call the other, right? Register should happen before GetName for any given thread.
On 2016/03/29 20:12:41, danakj wrote: > https://codereview.chromium.org/1814083002/diff/100001/base/threading/thread_... > File base/threading/thread_id_name_manager.cc (right): > > https://codereview.chromium.org/1814083002/diff/100001/base/threading/thread_... > base/threading/thread_id_name_manager.cc:80: > trace_event::AllocationContextTracker::SetCurrentThreadName( > On 2016/03/29 18:19:11, ssid wrote: > > On 2016/03/29 00:47:36, danakj wrote: > > > Why not call GetName() when you need the name? > > > > I cannot call GetName from AllocationTracker because it could cause a deadlock > > at RegisterThread. > > AllocationTracker gets initialized at first allocation in the thread (where I > > should try to call GetName). The first allocation more often happens in > > RegisterThread. > > So, calling GetName tries to hold same lock as RegisterThread. > > Sorry I'm not understanding this, but why is it a deadlock? If RegisterThread > and GetName happen at the same time, then one will wait for the other and then > continue. Deadlocks occur when 2 locks are held in different orders and one lock > ends up waiting on the other lock which waits on the first lock. > > I only see 1 lock in your explanation tho, which would just provide single > access at a time? > > It sounded like you're worried about RegisterThread and GetName happening from > the same thread at the same time? One does not call the other, right? Register > should happen before GetName for any given thread. Sorry, I will try to explain better. Any function in ThreadIdManager that holds lock and allocates memory can cause deadlock. I just realized RegiterThread was bad example because it doesn't allocate memory as I thought. Flow is: ThreadIdNameManager::SetName() - holds thread_id_lock. -> new string() - new operator is overriden by to call AllocationTracker::Insert -> AllocationTracker::Insert() -> AllocationTracker::Initialize() -> ThreadIdNameManager::GetName() - tries to hold thread_id_lock. This happens if I try to GetName from AllocationTracker.
ssid@chromium.org changed reviewers: + sievers@chromium.org
+sievers for small change in content/browser. ptal, thanks.
On 2016/03/29 23:09:58, ssid wrote: > +sievers for small change in content/browser. ptal, thanks. lgtm
https://codereview.chromium.org/1814083002/diff/100001/base/threading/thread_... File base/threading/thread_id_name_manager.cc (right): https://codereview.chromium.org/1814083002/diff/100001/base/threading/thread_... base/threading/thread_id_name_manager.cc:80: trace_event::AllocationContextTracker::SetCurrentThreadName( On 2016/03/29 20:12:41, danakj wrote: > On 2016/03/29 18:19:11, ssid wrote: > > On 2016/03/29 00:47:36, danakj wrote: > > > Why not call GetName() when you need the name? Yeah that was my initial proposal, unfortunately won't work for this very special case (more below). > > > > I cannot call GetName from AllocationTracker because it could cause a deadlock > > at RegisterThread. > > AllocationTracker gets initialized at first allocation in the thread (where I > > should try to call GetName). The first allocation more often happens in > > RegisterThread. > > So, calling GetName tries to hold same lock as RegisterThread. > > Sorry I'm not understanding this, but why is it a deadlock? If RegisterThread > and GetName happen at the same time, then one will wait for the other and then > continue. Deadlocks occur when 2 locks are held in different orders and one lock > ends up waiting on the other lock which waits on the first lock. > > I only see 1 lock in your explanation tho, which would just provide single > access at a time? I think what he means here is: deadlock by re-entrancy. The magic bit here is that if he needs the name in the heap profiler code under malloc hook. So if he tries to use GetName() he'd end up in the current situation: (some code) -> malloc -> TIDNameMgr::GetName(). Now, this would work most of the times, but won't work in the case where (some code) == (some code in TIDNamemgr which has is holding the lock). In other words, if the first malloc seen on one thread is due to some method ThreadIdNameManager (which unfortunately seems to be the case), GetName would re-entrantly try to re-acquire the lock. The lock is irrelevant to be honest, the real problem is that he'd re-entrantly call within ThreadIdNameManager, which should not happen even if it was lock-free, as re-entrant code is bad. I think this is the reason why he's opting for a push-model. I agree this is less nice from a layering viewpoint, but not sure is there any other way around this (I'd love if there was a way to not touch base code outside of trace_event). > It sounded like you're worried about RegisterThread and GetName happening from > the same thread at the same time? One does not call the other, right? Unless one calls malloc and the malloc hook calls GetName, which is the catch here :)
BTW as a general pattern a CL description should always have some comment to explain why are you changing existing code. The current description says why you want a new feature, but doesn't really give any clue why you are reshuffling that code in tid_name_manager. Assume existing code should not be edited unless you have a good reason to, in which case you want to make it explicit and understandable :) On 2016/03/31 15:48:28, Primiano Tucci wrote: > https://codereview.chromium.org/1814083002/diff/100001/base/threading/thread_... > File base/threading/thread_id_name_manager.cc (right): > > https://codereview.chromium.org/1814083002/diff/100001/base/threading/thread_... > base/threading/thread_id_name_manager.cc:80: > trace_event::AllocationContextTracker::SetCurrentThreadName( > On 2016/03/29 20:12:41, danakj wrote: > > On 2016/03/29 18:19:11, ssid wrote: > > > On 2016/03/29 00:47:36, danakj wrote: > > > > Why not call GetName() when you need the name? > > Yeah that was my initial proposal, unfortunately won't work for this very > special case (more below). > > > > > > > I cannot call GetName from AllocationTracker because it could cause a > deadlock > > > at RegisterThread. > > > AllocationTracker gets initialized at first allocation in the thread (where > I > > > should try to call GetName). The first allocation more often happens in > > > RegisterThread. > > > So, calling GetName tries to hold same lock as RegisterThread. > > > > Sorry I'm not understanding this, but why is it a deadlock? If RegisterThread > > and GetName happen at the same time, then one will wait for the other and then > > continue. Deadlocks occur when 2 locks are held in different orders and one > lock > > ends up waiting on the other lock which waits on the first lock. > > > > I only see 1 lock in your explanation tho, which would just provide single > > access at a time? > > I think what he means here is: deadlock by re-entrancy. > The magic bit here is that if he needs the name in the heap profiler code under > malloc hook. So if he tries to use GetName() he'd end up in the current > situation: > > (some code) -> malloc -> TIDNameMgr::GetName(). > Now, this would work most of the times, but won't work in the case where (some > code) == (some code in TIDNamemgr which has is holding the lock). > In other words, if the first malloc seen on one thread is due to some method > ThreadIdNameManager (which unfortunately seems to be the case), GetName would > re-entrantly try to re-acquire the lock. The lock is irrelevant to be honest, > the real problem is that he'd re-entrantly call within ThreadIdNameManager, > which should not happen even if it was lock-free, as re-entrant code is bad. > > I think this is the reason why he's opting for a push-model. > I agree this is less nice from a layering viewpoint, but not sure is there any > other way around this (I'd love if there was a way to not touch base code > outside of trace_event). > > > > It sounded like you're worried about RegisterThread and GetName happening from > > the same thread at the same time? One does not call the other, right? > Unless one calls malloc and the malloc hook calls GetName, which is the catch > here :)
https://codereview.chromium.org/1814083002/diff/120001/base/threading/thread_... File base/threading/thread_id_name_manager.cc (right): https://codereview.chromium.org/1814083002/diff/120001/base/threading/thread_... base/threading/thread_id_name_manager.cc:79: // is valid for the lifetime of the process. We should document this then in the code. Something about how this method is doing allocations, which would cause the AllocationContextTracker to request the name of the thread, but we're in the middle of setting it. But.. if we're setting the name down here, but allocating above, won't those allocations be missing a name?
Description was changed from ========== [tracing] Add thread name to the pseudo stack of heap profiler The thread name that allocated the memory is very useful to understand the memory usage of the process. This CL adds it to the top of the pseudo stack trace. BUG=594803 ========== to ========== [tracing] Add thread name to the pseudo stack of heap profiler The thread name that allocated the memory is very useful to understand the memory usage of the process. This CL adds it to the top of the pseudo stack trace. The name is set from ThreadIdNameManager since it leaks the thread name. Since getting the name from AllocationContextTracker can cause rentrancy into ThreadIdNameManager, the name is passed from ThreadIdNameManager. BUG=594803 ==========
https://codereview.chromium.org/1814083002/diff/120001/base/threading/thread_... File base/threading/thread_id_name_manager.cc (right): https://codereview.chromium.org/1814083002/diff/120001/base/threading/thread_... base/threading/thread_id_name_manager.cc:79: // is valid for the lifetime of the process. On 2016/03/31 21:03:13, danakj wrote: > We should document this then in the code. Something about how this method is > doing allocations, which would cause the AllocationContextTracker to request the > name of the thread, but we're in the middle of setting it. > > But.. if we're setting the name down here, but allocating above, won't those > allocations be missing a name? I added a comment here. wdyt? Yes, if we set it here we will miss some allocations, but we cannot name them when we don't even have a thread name yet. This is very less, so it is ok to miss them.
https://codereview.chromium.org/1814083002/diff/120001/base/threading/thread_... File base/threading/thread_id_name_manager.cc (right): https://codereview.chromium.org/1814083002/diff/120001/base/threading/thread_... base/threading/thread_id_name_manager.cc:79: // is valid for the lifetime of the process. On 2016/03/31 22:08:21, ssid wrote: > On 2016/03/31 21:03:13, danakj wrote: > > We should document this then in the code. Something about how this method is > > doing allocations, which would cause the AllocationContextTracker to request > the > > name of the thread, but we're in the middle of setting it. > > > > But.. if we're setting the name down here, but allocating above, won't those > > allocations be missing a name? > > I added a comment here. wdyt? > > Yes, if we set it here we will miss some allocations, but we cannot name them > when we don't even have a thread name yet. This is very less, so it is ok to > miss them. Could you just pass |name.c_str()| to it?
https://codereview.chromium.org/1814083002/diff/120001/base/threading/thread_... File base/threading/thread_id_name_manager.cc (right): https://codereview.chromium.org/1814083002/diff/120001/base/threading/thread_... base/threading/thread_id_name_manager.cc:79: // is valid for the lifetime of the process. On 2016/03/31 22:16:01, danakj wrote: > On 2016/03/31 22:08:21, ssid wrote: > > On 2016/03/31 21:03:13, danakj wrote: > > > We should document this then in the code. Something about how this method is > > > doing allocations, which would cause the AllocationContextTracker to request > > the > > > name of the thread, but we're in the middle of setting it. > > > > > > But.. if we're setting the name down here, but allocating above, won't those > > > allocations be missing a name? > > > > I added a comment here. wdyt? > > > > Yes, if we set it here we will miss some allocations, but we cannot name them > > when we don't even have a thread name yet. This is very less, so it is ok to > > miss them. > > Could you just pass |name.c_str()| to it? Oh, you store the pointer, I guess not. nvm!
LGTM
https://codereview.chromium.org/1814083002/diff/120001/base/threading/thread_... File base/threading/thread_id_name_manager.cc (right): https://codereview.chromium.org/1814083002/diff/120001/base/threading/thread_... base/threading/thread_id_name_manager.cc:79: // is valid for the lifetime of the process. On 2016/03/31 22:18:17, danakj wrote: > On 2016/03/31 22:16:01, danakj wrote: > > On 2016/03/31 22:08:21, ssid wrote: > > > On 2016/03/31 21:03:13, danakj wrote: > > > > We should document this then in the code. Something about how this method > is > > > > doing allocations, which would cause the AllocationContextTracker to > request > > > the > > > > name of the thread, but we're in the middle of setting it. > > > > > > > > But.. if we're setting the name down here, but allocating above, won't > those > > > > allocations be missing a name? > > > > > > I added a comment here. wdyt? > > > > > > Yes, if we set it here we will miss some allocations, but we cannot name > them > > > when we don't even have a thread name yet. This is very less, so it is ok to > > > miss them. > > > > Could you just pass |name.c_str()| to it? > > Oh, you store the pointer, I guess not. nvm! I also tried to see if this really needs to take a std::string and not just a const char*.. but sometimes we're constructing the names dynamically FBOFW. https://code.google.com/p/chromium/codesearch#chromium/src/base/threading/sim...
On 2016/03/31 22:20:08, danakj wrote: > https://codereview.chromium.org/1814083002/diff/120001/base/threading/thread_... > File base/threading/thread_id_name_manager.cc (right): > > https://codereview.chromium.org/1814083002/diff/120001/base/threading/thread_... > base/threading/thread_id_name_manager.cc:79: // is valid for the lifetime of the > process. > On 2016/03/31 22:18:17, danakj wrote: > > On 2016/03/31 22:16:01, danakj wrote: > > > On 2016/03/31 22:08:21, ssid wrote: > > > > On 2016/03/31 21:03:13, danakj wrote: > > > > > We should document this then in the code. Something about how this > method > > is > > > > > doing allocations, which would cause the AllocationContextTracker to > > request > > > > the > > > > > name of the thread, but we're in the middle of setting it. > > > > > > > > > > But.. if we're setting the name down here, but allocating above, won't > > those > > > > > allocations be missing a name? > > > > > > > > I added a comment here. wdyt? > > > > > > > > Yes, if we set it here we will miss some allocations, but we cannot name > > them > > > > when we don't even have a thread name yet. This is very less, so it is ok > to > > > > miss them. > > > > > > Could you just pass |name.c_str()| to it? > > > > Oh, you store the pointer, I guess not. nvm! > > I also tried to see if this really needs to take a std::string and not just a > const char*.. but sometimes we're constructing the names dynamically FBOFW. > https://code.google.com/p/chromium/codesearch#chromium/src/base/threading/sim... On 2016/03/31 22:18:17, danakj wrote: > On 2016/03/31 22:16:01, danakj wrote: > > On 2016/03/31 22:08:21, ssid wrote: > > > On 2016/03/31 21:03:13, danakj wrote: > > > > We should document this then in the code. Something about how this method > is > > > > doing allocations, which would cause the AllocationContextTracker to > request > > > the > > > > name of the thread, but we're in the middle of setting it. > > > > > > > > But.. if we're setting the name down here, but allocating above, won't > those > > > > allocations be missing a name? > > > > > > I added a comment here. wdyt? > > > > > > Yes, if we set it here we will miss some allocations, but we cannot name > them > > > when we don't even have a thread name yet. This is very less, so it is ok to > > > miss them. > > > > Could you just pass |name.c_str()| to it? > > Oh, you store the pointer, I guess not. nvm! I cannot pass name.c_str because thread name could be a constructed string that is passed by any class. I am using the leaked_str since it is long lived and leaked. The issue was that the name is required even after thread dies, since memory allocated by that thread lives after thread dies. I also did not want to pass std::string since I would have to leak one more string in tracker. So, reusing the already leaked string. yes most thread names are just strings but some are constructed.
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/1814083002/140001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1814083002/140001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
The CQ bit was checked by ssid@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from primiano@chromium.org, sievers@chromium.org, petrcermak@chromium.org Link to the patchset: https://codereview.chromium.org/1814083002/#ps140001 (title: "Add comment.")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1814083002/140001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1814083002/140001
Message was sent while issue was closed.
Description was changed from ========== [tracing] Add thread name to the pseudo stack of heap profiler The thread name that allocated the memory is very useful to understand the memory usage of the process. This CL adds it to the top of the pseudo stack trace. The name is set from ThreadIdNameManager since it leaks the thread name. Since getting the name from AllocationContextTracker can cause rentrancy into ThreadIdNameManager, the name is passed from ThreadIdNameManager. BUG=594803 ========== to ========== [tracing] Add thread name to the pseudo stack of heap profiler The thread name that allocated the memory is very useful to understand the memory usage of the process. This CL adds it to the top of the pseudo stack trace. The name is set from ThreadIdNameManager since it leaks the thread name. Since getting the name from AllocationContextTracker can cause rentrancy into ThreadIdNameManager, the name is passed from ThreadIdNameManager. BUG=594803 ==========
Message was sent while issue was closed.
Committed patchset #8 (id:140001)
Message was sent while issue was closed.
Description was changed from ========== [tracing] Add thread name to the pseudo stack of heap profiler The thread name that allocated the memory is very useful to understand the memory usage of the process. This CL adds it to the top of the pseudo stack trace. The name is set from ThreadIdNameManager since it leaks the thread name. Since getting the name from AllocationContextTracker can cause rentrancy into ThreadIdNameManager, the name is passed from ThreadIdNameManager. BUG=594803 ========== to ========== [tracing] Add thread name to the pseudo stack of heap profiler The thread name that allocated the memory is very useful to understand the memory usage of the process. This CL adds it to the top of the pseudo stack trace. The name is set from ThreadIdNameManager since it leaks the thread name. Since getting the name from AllocationContextTracker can cause rentrancy into ThreadIdNameManager, the name is passed from ThreadIdNameManager. BUG=594803 Committed: https://crrev.com/b3fabc66203904dd110597b11ebb89b89697f1a9 Cr-Commit-Position: refs/heads/master@{#384484} ==========
Message was sent while issue was closed.
Patchset 8 (id:??) landed as https://crrev.com/b3fabc66203904dd110597b11ebb89b89697f1a9 Cr-Commit-Position: refs/heads/master@{#384484}
Message was sent while issue was closed.
Patchset #2 (id:20001) has been deleted
Message was sent while issue was closed.
Patchset #3 (id:60001) has been deleted
Message was sent while issue was closed.
Patchset #2 (id:40001) has been deleted
Message was sent while issue was closed.
Patchset #1 (id:1) has been deleted |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
