Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1784133002:
[tracing] Adding task information to heap profiler (Closed)
|
Created:
4 years, 9 months ago by ssid Modified:
4 years, 8 months ago Reviewers:
Primiano Tucci (use gerrit), jochen (gone - plz use gerrit), Sami, Dmitry Skiba, petrcermak, Nico, oystein (OOO til 10th of July) CC:
chirantan+watch_chromium.org, chromium-reviews, sadrul, scheduler-bugs_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
Description[tracing] Adding task information to heap profiler
This CL adds more information to the malloc heap profiler added in
crrev.com/1675183006. The heap profiler only shows trace events and all
the tasks show up as "MessageLoop::Runtask".
This CL does:
- Moves TRACE_TASK_EXECUTION macro from task_annotator to
trace_event.h.
- Adds extra scoped trace event to track the task context in heap
profiler.
- The folder where the task was posted from is used to categorize
allocations.
- Uses the type_name for context temporarily till we define a new
context dimension for allocations.
BUG=594803
Committed: https://crrev.com/533c402f8b21e503ebcaf3f0de427f41076b4de1
Cr-Commit-Position: refs/heads/master@{#384521}
Patch Set 1 : fix file name as type. #Patch Set 2 : Use tracked_objects. #Patch Set 3 : use SetThreadName function. #
Total comments: 12
Patch Set 4 : Use ThreadIdNameManager #
Total comments: 2
Patch Set 5 : Stack of categories. #Patch Set 6 : With hack to get posted_by. #Patch Set 7 : Without posted_by. #Patch Set 8 : Just add file name #Patch Set 9 : Only add file name. #Patch Set 10 : nits. #
Total comments: 14
Patch Set 11 : Fixes. #
Total comments: 18
Patch Set 12 : Petr's comments #
Total comments: 10
Patch Set 13 : Fix macros and add unittest. #Patch Set 14 : Fix macros again and the builds. #
Total comments: 25
Patch Set 15 : Renames, parentheses and fix win build #Patch Set 16 : nit. #Depends on Patchset: Messages
Total messages: 70 (36 generated)
Description was changed from ========== [NOT FOR REVIEW] Adding thread name and file to profiler. BUG= ========== to ========== [NOT FOR REVIEW] Adding thread name and file to profiler. Depends on https://codereview.chromium.org/1675183006/ It is also good to have https://codereview.chromium.org/1783223002/ for better type info. BUG= ==========
Description was changed from ========== [NOT FOR REVIEW] Adding thread name and file to profiler. Depends on https://codereview.chromium.org/1675183006/ It is also good to have https://codereview.chromium.org/1783223002/ for better type info. BUG= ========== to ========== [NOT FOR REVIEW] Adding thread name and file to profiler. Depends on https://codereview.chromium.org/1675183006/ It is also good to have catapult CL https://codereview.chromium.org/1783223002/ for better type info. BUG= ==========
Patchset #5 (id:80001) has been deleted
Patchset #2 (id:20001) has been deleted
Patchset #1 (id:1) has been deleted
Patchset #5 (id:140001) has been deleted
Patchset #4 (id:120001) has been deleted
Description was changed from ========== [NOT FOR REVIEW] Adding thread name and file to profiler. Depends on https://codereview.chromium.org/1675183006/ It is also good to have catapult CL https://codereview.chromium.org/1783223002/ for better type info. BUG= ========== to ========== Adding thread name and task information to heap profiler This CL adds more information to the malloc heap profiler added in crrev.com/1675183006. - Add the thread name as the first stack frame in pseudo stack trace. - Adds the task name (function) to the pseudo stack trace at the start of each task. - Adds the file name where the task was posted from as default type information. This is used to categorize the allocations with the folder which started the tasks. See crrev.com/1783223002 for trace viewer side change. BUG=594803 ==========
Patchset #5 (id:180001) has been deleted
ssid@chromium.org changed reviewers: + primiano@chromium.org
This adds the file name as the type for malloc. Do you think I should introduce a new member called file_name in the AllocationContext and new field in json? this would require a new visualization in TV. So, added as type for now, and can be changed to file name when we change the viewer. ptal, thanks.
Thanks so much, see comments below. I think my major concern is: not sure we should push the TaskName as part of the stack trace. That will de-root the stack traces making them look as if they are all independent. https://codereview.chromium.org/1784133002/diff/200001/base/debug/task_annota... File base/debug/task_annotator.h (right): https://codereview.chromium.org/1784133002/diff/200001/base/debug/task_annota... base/debug/task_annotator.h:48: base::trace_event::AllocationContextTracker::SetCurrentTaskFileName( why are these inline? Is it for perf reasons? Tasks are pretty coarse grained, not sure we need this level of perf obsession here https://codereview.chromium.org/1784133002/diff/200001/base/debug/task_annota... base/debug/task_annotator.h:50: base::trace_event::AllocationContextTracker::PushPseudoStackFrame( Hmm I think that this will screw the pseudo-stack, right? Right now everything starts with RunLoop -> RunTask -> bla bla. With this change you are now making each task have a different root. I don't think this PushPseudoStackFrame should happen at all https://codereview.chromium.org/1784133002/diff/200001/base/trace_event/heap_... File base/trace_event/heap_profiler_allocation_context_tracker.cc (right): https://codereview.chromium.org/1784133002/diff/200001/base/trace_event/heap_... base/trace_event/heap_profiler_allocation_context_tracker.cc:120: CHECK(tracker); NO need for this CHECK. If tracker is nullptr, this will naturally die on the next statement while trying to insert. https://codereview.chromium.org/1784133002/diff/200001/base/trace_event/heap_... base/trace_event/heap_profiler_allocation_context_tracker.cc:156: // Set the default type name to file name where the task was posted from. Can you add a TODO: this should be a 3rd dimension (component name) in the heap profiler, not piggy back on the type. https://codereview.chromium.org/1784133002/diff/200001/base/tracked_objects.cc File base/tracked_objects.cc (right): https://codereview.chromium.org/1784133002/diff/200001/base/tracked_objects.c... base/tracked_objects.cc:369: // even if thread is dead. Bad news: I took a look to this and figured out that tracked_objects is a bit dodgy, in the sense that the thread name is leaked, but not in tests (which might cause flakiness). On the good side, I figured out that thread_id_name_manager.cc actually leaks the string with the thread name (See leaked_str in ThreadIdNameManager::SetName). Good news: So I think you can achieve this by doing no change in base (outside of trace_event) at all. Ideally (*there is a catch, see below) in the AllocationContexTracker CTOR, you can just do: ::AllocationContexTracker() { PushPseudoStackFrame(ThreadIdNameManager::GetName(GetCurrentThreadID())) } Bad news: If you do this you will cause a re-entrancy in the heap profiler that will deadlock. The pattern will be: the first time you use the ContextTracker -> Ctor() -> PushPseudoStackFrame will call malloc -> malloc will be intercepted -> you are back in ContextTracker -> BOOM Good news: After my CL (crrev.com/1675183006) lands I think you can put that piece of code in ::InitializeForCurrentThread() which has malloc re-entrancy protection https://www.youtube.com/watch?v=Krbl911ZPBA
dskiba@google.com changed reviewers: + dskiba@google.com
https://codereview.chromium.org/1784133002/diff/200001/base/debug/task_annota... File base/debug/task_annotator.h (right): https://codereview.chromium.org/1784133002/diff/200001/base/debug/task_annota... base/debug/task_annotator.h:47: if (base::trace_event::AllocationContextTracker::capture_enabled()) { BTW, since this is inside base too, you can omit base:: from namespaces - this will make lines shorter.
I don't understand when u say stack traces might look independent. Do you have an example? I don't think I am causing any more branching here, instead I am only adding an extra layer of information in between RunTask and function. https://codereview.chromium.org/1784133002/diff/200001/base/debug/task_annota... File base/debug/task_annotator.h (right): https://codereview.chromium.org/1784133002/diff/200001/base/debug/task_annota... base/debug/task_annotator.h:47: if (base::trace_event::AllocationContextTracker::capture_enabled()) { On 2016/03/15 19:38:31, Dmitry Skiba wrote: > BTW, since this is inside base too, you can omit base:: from namespaces - this > will make lines shorter. Done. https://codereview.chromium.org/1784133002/diff/200001/base/debug/task_annota... base/debug/task_annotator.h:48: base::trace_event::AllocationContextTracker::SetCurrentTaskFileName( On 2016/03/15 16:44:33, Primiano wrote: > why are these inline? Is it for perf reasons? > > Tasks are pretty coarse grained, not sure we need this level of perf obsession > here See TRACE_TASK_EXECUTION below. TRACE_TASK_EXECUTION already adds a trace event. But the trace event has 2 extra params as task posted file and function. With current infra only the main function of the trace event is recorded and the arguments are ignored. So, it will just show up as MessageLoop::RunLoop or similar. By adding this manually here it actually shows the task file name and posting function in the stack trace. I cannot add this inside the trace_event2 because there are other use cases of trace_event2 which does not add task information. So, TRACE_TASK_EXECUTION is a special macro that handles task execution tracing. Is there another way to get this? Am I missing something? https://codereview.chromium.org/1784133002/diff/200001/base/debug/task_annota... base/debug/task_annotator.h:50: base::trace_event::AllocationContextTracker::PushPseudoStackFrame( On 2016/03/15 16:44:33, Primiano wrote: > Hmm I think that this will screw the pseudo-stack, right? > Right now everything starts with RunLoop -> RunTask -> bla bla. > With this change you are now making each task have a different root. > I don't think this PushPseudoStackFrame should happen at all What I was imagining was, The function that posted the task is added in a stack frame before the task is called. In some sense, the pseudo stack shows who posted it instead of who called it. This happens inside RunLoop. But, this would show up as if RunLoop called the posting function then the real function. Now it will look like: RunLoop -> RunTask -> posted_task_name -> actual_task_name -> bla bla. Do you suggest to add this as a special field? How are we going to display this? https://codereview.chromium.org/1784133002/diff/200001/base/trace_event/heap_... File base/trace_event/heap_profiler_allocation_context_tracker.cc (right): https://codereview.chromium.org/1784133002/diff/200001/base/trace_event/heap_... base/trace_event/heap_profiler_allocation_context_tracker.cc:120: CHECK(tracker); On 2016/03/15 16:44:33, Primiano wrote: > NO need for this CHECK. If tracker is nullptr, this will naturally die on the > next statement while trying to insert. Done. https://codereview.chromium.org/1784133002/diff/200001/base/trace_event/heap_... base/trace_event/heap_profiler_allocation_context_tracker.cc:156: // Set the default type name to file name where the task was posted from. On 2016/03/15 16:44:33, Primiano wrote: > Can you add a TODO: this should be a 3rd dimension (component name) in the heap > profiler, not piggy back on the type. Done. https://codereview.chromium.org/1784133002/diff/200001/base/tracked_objects.cc File base/tracked_objects.cc (right): https://codereview.chromium.org/1784133002/diff/200001/base/tracked_objects.c... base/tracked_objects.cc:369: // even if thread is dead. On 2016/03/15 16:44:33, Primiano wrote: > Bad news: > I took a look to this and figured out that tracked_objects is a bit dodgy, in > the sense that the thread name is leaked, but not in tests (which might cause > flakiness). > On the good side, I figured out that thread_id_name_manager.cc actually leaks > the string with the thread name (See leaked_str in > ThreadIdNameManager::SetName). > > Good news: > So I think you can achieve this by doing no change in base (outside of > trace_event) at all. > Ideally (*there is a catch, see below) in the AllocationContexTracker CTOR, you > can just do: > > ::AllocationContexTracker() { > PushPseudoStackFrame(ThreadIdNameManager::GetName(GetCurrentThreadID())) > } > > Bad news: > If you do this you will cause a re-entrancy in the heap profiler that will > deadlock. The pattern will be: > the first time you use the ContextTracker -> Ctor() -> PushPseudoStackFrame will > call malloc -> malloc will be intercepted -> you are back in ContextTracker -> > BOOM > > > Good news: > After my CL (crrev.com/1675183006) lands I think you can put that piece of code > in ::InitializeForCurrentThread() which has malloc re-entrancy protection > > https://www.youtube.com/watch?v=Krbl911ZPBA I think I should continue with good news and bad news here :P Calling base::ThreadIdNameManager::GetName causes deadlock because the first allocation that happens (which also initializes tracker) in the threads is from base::ThreadIdNameManager::RegisterThread which holds the lock needed to get name. I am setting the name from ThreadIdNameManager::SetName. Ideally I should be doing it form PlatformThread::Setname which is a static function that is called in a thread to set it's own name. Where as ThreadIdNameManager::SetName is a member of singleton ThreadIdNameManager which can be called from any thread, which will reset the tls of another thread if it happens. But the PlatformThread::SetName is defined for each platform so I should change 6-7 files for that. Instead I have now put it in ThreadIdNameManager because the only place that calls ThreadIdNameManager::SetName is PlatformThread::SetName which is always called in the same thread. So, I prefer changing just ThreadIdNameManager instead of all the files. If you think it is safer to set it from PlatformThread then I can change.
Patchset #6 (id:220001) has been deleted
skyostil@chromium.org changed reviewers: + skyostil@chromium.org
https://codereview.chromium.org/1784133002/diff/240001/base/debug/task_annota... File base/debug/task_annotator.h (right): https://codereview.chromium.org/1784133002/diff/240001/base/debug/task_annota... base/debug/task_annotator.h:47: if (trace_event::AllocationContextTracker::capture_enabled()) { Might want to add a throw an UNLIKELY() around this here and below. https://codereview.chromium.org/1784133002/diff/240001/base/debug/task_annota... base/debug/task_annotator.h:48: trace_event::AllocationContextTracker::SetCurrentTaskFileName(file_name); Why isn't the file name also part of the pseudo stack? The fact that we reset it to null in the destructor might make for some confusing traces when nested tasks are involved.
Thanks for reviews. I think I understand the issue better now.
1. I cannot add the posting function to stack trace as it is since it can
confuse the user.
2. Nested tasks are possible.
3. For IPC message handling the category is always ipc/
4. Task observers are missed in trace events. I will add a todo for this. This
could add to the missing memory pointed by Dmitri.
We can change the StackFrame into a struct:
struct StackFrame {
const char* function_name;
const char* posted_reason;
const char* category_name;
};
This works for all the cases:
1. While adding to trace, posted_reason is added by appending a string
("initiated_by %s", posted_reason) before adding the function_name. This will
work for nested tasks as well.
2. For category name (which is currently the type_name), we take the last
category_name and use the folder to categorize.
3. For IPC messages I can add posted_reason as the message name (this is
available in release builds as well. So for all the memory allocated while
handling ipc messages, the category name will be ipc/ and when you open ipc you
will see "initiated_by<message name>"
I think I should move this discussion to a doc. If it grows from here i will do
that.
On 2016/03/17 21:52:05, ssid wrote:
> Thanks for reviews. I think I understand the issue better now.
>
> 1. I cannot add the posting function to stack trace as it is since it can
> confuse the user.
> 2. Nested tasks are possible.
> 3. For IPC message handling the category is always ipc/
> 4. Task observers are missed in trace events. I will add a todo for this. This
> could add to the missing memory pointed by Dmitri.
>
> We can change the StackFrame into a struct:
>
> struct StackFrame {
> const char* function_name;
> const char* posted_reason;
> const char* category_name;
> };
>
> This works for all the cases:
> 1. While adding to trace, posted_reason is added by appending a string
> ("initiated_by %s", posted_reason) before adding the function_name. This will
> work for nested tasks as well.
> 2. For category name (which is currently the type_name), we take the last
> category_name and use the folder to categorize.
> 3. For IPC messages I can add posted_reason as the message name (this is
> available in release builds as well. So for all the memory allocated while
> handling ipc messages, the category name will be ipc/ and when you open ipc
you
> will see "initiated_by<message name>"
>
> I think I should move this discussion to a doc. If it grows from here i will
do
> that.
I have copied this discussion with few more points to the heading at
https://docs.google.com/document/d/18M_v2nCmb0Scd6eMSu3dX961I0HPPhCSgL_h3Ln3m...
feel free to comment. Thanks.
Description was changed from ========== Adding thread name and task information to heap profiler This CL adds more information to the malloc heap profiler added in crrev.com/1675183006. - Add the thread name as the first stack frame in pseudo stack trace. - Adds the task name (function) to the pseudo stack trace at the start of each task. - Adds the file name where the task was posted from as default type information. This is used to categorize the allocations with the folder which started the tasks. See crrev.com/1783223002 for trace viewer side change. BUG=594803 ========== to ========== Adding task information to heap profiler This CL adds more information to the malloc heap profiler added in crrev.com/1675183006. - Adds the posted task name (function) to the pseudo stack trace at the start of each task. - Adds the posted file name where the task was posted from as default type information. This is used to categorize the allocations with the folder which started the tasks. BUG=594803 ==========
Description was changed from ========== Adding task information to heap profiler This CL adds more information to the malloc heap profiler added in crrev.com/1675183006. - Adds the posted task name (function) to the pseudo stack trace at the start of each task. - Adds the posted file name where the task was posted from as default type information. This is used to categorize the allocations with the folder which started the tasks. BUG=594803 ========== to ========== Adding task information to heap profiler This CL adds more information to the malloc heap profiler added in crrev.com/1675183006. The file name where the task was posted is set as the default type for allocations. We categorize the allocations using the folder which started the tasks. BUG=594803 ==========
Patchset #10 (id:320001) has been deleted
Description was changed from ========== Adding task information to heap profiler This CL adds more information to the malloc heap profiler added in crrev.com/1675183006. The file name where the task was posted is set as the default type for allocations. We categorize the allocations using the folder which started the tasks. BUG=594803 ========== to ========== [tracing] Using file name to categorize allocations This CL adds more information to the malloc heap profiler added in crrev.com/1675183006. The file name where the task was posted is set as the default type for allocations. We categorize the allocations using the folder which started the tasks. BUG=594803 ==========
ssid@chromium.org changed reviewers: + petrcermak@chromium.org
+petr One more review for you. background context: We are not yet sure how the 3rd dimension is going to look like. Lets chat about that in a while. In meantime we (I and primiano) agreed that instead of showing empty type we can as well show the category as types which is more useful. I am not sure if it's good to do the extract part in the c++ or in the trace viewer. I saw that the other extraction lies in TV, I felt doing it in native code will be faster than js. So, added it here. PTAL, Thanks.
Patchset #12 (id:380001) has been deleted
Some comments here and there, approach sounds reasonable % implementation details. https://codereview.chromium.org/1784133002/diff/400001/base/debug/task_annota... File base/debug/task_annotator.h (right): https://codereview.chromium.org/1784133002/diff/400001/base/debug/task_annota... base/debug/task_annotator.h:47: base::trace_event::AllocationContextTracker::ScopedTaskExecutionEvent event( \ I'd use a name which tries to avoid conflics. Also, to be honest I think that either: - this entire macro should really be inside trace_event.h. There you could use INTERNAL_TRACE_EVENT_UID to mangle the name for the event variable. or - ScopedTaskExecutionEvent should be a class inside task_annotator.h, and make the push methods public in the tracker https://codereview.chromium.org/1784133002/diff/400001/base/debug/task_annota... base/debug/task_annotator.h:48: (task).posted_from.file_name(), (task).posted_from.function_name()); Looks like you never use the second argument. Why bothering passing that? https://codereview.chromium.org/1784133002/diff/400001/base/trace_event/heap_... File base/trace_event/heap_profiler_allocation_context_tracker.cc (right): https://codereview.chromium.org/1784133002/diff/400001/base/trace_event/heap_... base/trace_event/heap_profiler_allocation_context_tracker.cc:155: DCHECK(category != nullptr); I think as a general stylistic pattern we don't bother with != nullptr, just DCHECK(category) should be file. Also rename the arg :) https://codereview.chromium.org/1784133002/diff/400001/base/trace_event/heap_... File base/trace_event/heap_profiler_allocation_context_tracker.h (right): https://codereview.chromium.org/1784133002/diff/400001/base/trace_event/heap_... base/trace_event/heap_profiler_allocation_context_tracker.h:29: const char* posted_reason); I think these should be called poster_task_file_name and poster_task_function_name Also, looks like you never use the 2nd argument, so let's remove it https://codereview.chromium.org/1784133002/diff/400001/base/trace_event/heap_... base/trace_event/heap_profiler_allocation_context_tracker.h:81: void PushCurrentCategoryName(const char* category); I'm a bit confused by the "Category" name here. Can we call these just PushPosterTaskName https://codereview.chromium.org/1784133002/diff/400001/base/trace_event/heap_... File base/trace_event/heap_profiler_type_name_deduplicator.cc (right): https://codereview.chromium.org/1784133002/diff/400001/base/trace_event/heap_... base/trace_event/heap_profiler_type_name_deduplicator.cc:23: std::string ExtractTypeInfo(const std::string& type_name) { I think the arg of this should be const char*, if you really need the char* to string conversion you want to make it explicit, not hiding in the cast operator Also I think this function could be simplified a bit as follows: - Convert the const char* into a StringPiece (which is a very cheap operation) - Use find_first_of / find_last_of (/,\) to do the trimming on both side - Return a StringPiece and pass it to the json encoder. Also I'd give this a more explicative (ExtractPostedTaskDirName) name and add a comment to figure out what's going on. https://codereview.chromium.org/1784133002/diff/400001/base/trace_event/heap_... base/trace_event/heap_profiler_type_name_deduplicator.cc:38: for (std::string folder : folder_names) tip for next times, const std::string& would have avoided a copy here. But I think you can avoid any std::string at all in this function as suggested above
Patchset #13 (id:420001) has been deleted
Patchset #13 (id:440001) has been deleted
Description was changed from ========== [tracing] Using file name to categorize allocations This CL adds more information to the malloc heap profiler added in crrev.com/1675183006. The file name where the task was posted is set as the default type for allocations. We categorize the allocations using the folder which started the tasks. BUG=594803 ========== to ========== [tracing] Adding task information to heap profiler This CL adds more information to the malloc heap profiler added in crrev.com/1675183006. The heap profiler only shows trace events and all the tasks show up as "MessageLoop::Runtask". This CL does: - Moves TRACE_TASK_EXECUTION macro from task_annotator to trace_event.h. - Adds extra scoped trace event to track the task context in heap profiler. - The folder where the task was posted from is used to categorize allocations. - Uses the type_name for context temporarily till we define a new context dimension for allocations. BUG=594803 ==========
Thanks comments addressed. https://codereview.chromium.org/1784133002/diff/400001/base/debug/task_annota... File base/debug/task_annotator.h (right): https://codereview.chromium.org/1784133002/diff/400001/base/debug/task_annota... base/debug/task_annotator.h:47: base::trace_event::AllocationContextTracker::ScopedTaskExecutionEvent event( \ On 2016/03/25 01:56:31, Primiano (traveling) wrote: > I'd use a name which tries to avoid conflics. > Also, to be honest I think that either: > - this entire macro should really be inside trace_event.h. There you could use > INTERNAL_TRACE_EVENT_UID to mangle the name for the event variable. > or > - ScopedTaskExecutionEvent should be a class inside task_annotator.h, and make > the push methods public in the tracker yes I think its a good idea to move it to trace_event.h. Lets see what others think about this. If I add it here, I would have to explain about why task execution is special case what is heap profiler and stuff to the base/debug owners, it is easier to explain to trace_event/ folks. https://codereview.chromium.org/1784133002/diff/400001/base/debug/task_annota... base/debug/task_annotator.h:48: (task).posted_from.file_name(), (task).posted_from.function_name()); On 2016/03/25 01:56:31, Primiano (traveling) wrote: > Looks like you never use the second argument. Why bothering passing that? I was hoping I will need it soon and I will have to change this file again, asking other people. Now its inside trace_event/ can change later when needed. https://codereview.chromium.org/1784133002/diff/400001/base/trace_event/heap_... File base/trace_event/heap_profiler_allocation_context_tracker.cc (right): https://codereview.chromium.org/1784133002/diff/400001/base/trace_event/heap_... base/trace_event/heap_profiler_allocation_context_tracker.cc:155: DCHECK(category != nullptr); On 2016/03/25 01:56:31, Primiano (traveling) wrote: > I think as a general stylistic pattern we don't bother with != nullptr, just > DCHECK(category) should be file. > Also rename the arg :) Done. https://codereview.chromium.org/1784133002/diff/400001/base/trace_event/heap_... File base/trace_event/heap_profiler_allocation_context_tracker.h (right): https://codereview.chromium.org/1784133002/diff/400001/base/trace_event/heap_... base/trace_event/heap_profiler_allocation_context_tracker.h:29: const char* posted_reason); On 2016/03/25 01:56:31, Primiano (traveling) wrote: > I think these should be called poster_task_file_name and > poster_task_function_name > Also, looks like you never use the 2nd argument, so let's remove it calling it context, can change if you prefer to be more specific. I think if we decide to use something else in context instead of file name, it need not be changed. https://codereview.chromium.org/1784133002/diff/400001/base/trace_event/heap_... base/trace_event/heap_profiler_allocation_context_tracker.h:81: void PushCurrentCategoryName(const char* category); On 2016/03/25 01:56:31, Primiano (traveling) wrote: > I'm a bit confused by the "Category" name here. > Can we call these just PushPosterTaskName Hm it is not exactly task name. To be specific I should call it TaskPostedFromFilename. Gets too specific and we can't use it in future if we wanted to add some other context here. So, i am renaming it to TaskContext, to keep it generic but still makes enough sense. https://codereview.chromium.org/1784133002/diff/400001/base/trace_event/heap_... File base/trace_event/heap_profiler_type_name_deduplicator.cc (right): https://codereview.chromium.org/1784133002/diff/400001/base/trace_event/heap_... base/trace_event/heap_profiler_type_name_deduplicator.cc:23: std::string ExtractTypeInfo(const std::string& type_name) { On 2016/03/25 01:56:31, Primiano (OOO til Mar 31) wrote: > I think the arg of this should be const char*, if you really need the char* to > string conversion you want to make it explicit, not hiding in the cast operator > > Also I think this function could be simplified a bit as follows: > - Convert the const char* into a StringPiece (which is a very cheap operation) > - Use find_first_of / find_last_of (/,\) to do the trimming on both side > - Return a StringPiece and pass it to the json encoder. > > Also I'd give this a more explicative (ExtractPostedTaskDirName) name and add a > comment to figure out what's going on. Done. https://codereview.chromium.org/1784133002/diff/400001/base/trace_event/heap_... base/trace_event/heap_profiler_type_name_deduplicator.cc:38: for (std::string folder : folder_names) On 2016/03/25 01:56:31, Primiano (OOO til Mar 31) wrote: > tip for next times, const std::string& would have avoided a copy here. > But I think you can avoid any std::string at all in this function as suggested > above Thanks.
ssid@chromium.org changed reviewers: + oysteine@chromium.org
+oysteine Can you please take a look at trace_event.h and tell me if this a reasonable change? I want to move the macro that is in different file and add a new scoped event class for heap profiler. Thanks.
I'm fine with this hack and I added some comments. However, I do NOT want to do any category-specific processing on the Trace Viewer side until we have a proper category dimension in the trace. The reason for this is backwards compatibility: Once we support passing categories through types and actually parsing them, we have to support it forever (even when we do have the category dimension). We already have two cases for type name imports because there was a brief period of time when we used a different trace format. Let's not complicate things further. Petr https://codereview.chromium.org/1784133002/diff/460001/base/trace_event/heap_... File base/trace_event/heap_profiler_type_name_deduplicator.cc (right): https://codereview.chromium.org/1784133002/diff/460001/base/trace_event/heap_... base/trace_event/heap_profiler_type_name_deduplicator.cc:23: StringPiece ExtractDirNameFromTaskContext(const char* type_name) { The comment, method name and argument name all disagree with each other: "file name" vs. "TaskContext" vs. "type_name" "category name" vs. "DirName" https://codereview.chromium.org/1784133002/diff/460001/base/trace_event/heap_... base/trace_event/heap_profiler_type_name_deduplicator.cc:27: // If |type_name| is a not a file path the seperator will not be found. nit: add a comma after "path" https://codereview.chromium.org/1784133002/diff/460001/base/trace_event/heap_... base/trace_event/heap_profiler_type_name_deduplicator.cc:28: // return the type name. This is not a sentence, I suggest merging it with the first sentence: "... not be found, so the whole type name is returned." https://codereview.chromium.org/1784133002/diff/460001/base/trace_event/heap_... File base/trace_event/heap_profiler_type_name_deduplicator_unittest.cc (right): https://codereview.chromium.org/1784133002/diff/460001/base/trace_event/heap_... base/trace_event/heap_profiler_type_name_deduplicator_unittest.cc:27: const char kFolders[] = "base/trace_event"; I'd say kPath would be easier to understand. https://codereview.chromium.org/1784133002/diff/460001/base/trace_event/heap_... base/trace_event/heap_profiler_type_name_deduplicator_unittest.cc:29: const char kFileNameWin[] = "..\\..\\base\\memory\\memory_win.cc"; Why not kFileName as well? https://codereview.chromium.org/1784133002/diff/460001/base/trace_event/heap_... base/trace_event/heap_profiler_type_name_deduplicator_unittest.cc:40: // checks if value gets inserted and the exported value for |type_name| is same nit: s/if value/if the value/ and s/is same/is the same/ https://codereview.chromium.org/1784133002/diff/460001/base/trace_event/heap_... base/trace_event/heap_profiler_type_name_deduplicator_unittest.cc:43: const char* exported_value) { Don't you mean "expected"? https://codereview.chromium.org/1784133002/diff/460001/components/scheduler/b... File components/scheduler/base/task_queue_manager.cc (right): https://codereview.chromium.org/1784133002/diff/460001/components/scheduler/b... components/scheduler/base/task_queue_manager.cc:12: #include "base/trace_event/trace_event.h" Is this intentional?
On 2016/03/29 12:49:05, petrcermak wrote: > I'm fine with this hack and I added some comments. However, I do NOT want to do > any category-specific processing on the Trace Viewer side until we have a proper > category dimension in the trace. The reason for this is backwards compatibility: > Once we support passing categories through types and actually parsing them, we > have to support it forever (even when we do have the category dimension). We > already have two cases for type name imports because there was a brief period of > time when we used a different trace format. Let's not complicate things further. > Yes, that is the reason why I am parsing the category and adding it here and not changing TV. https://codereview.chromium.org/1784133002/diff/460001/base/trace_event/heap_... File base/trace_event/heap_profiler_type_name_deduplicator.cc (right): https://codereview.chromium.org/1784133002/diff/460001/base/trace_event/heap_... base/trace_event/heap_profiler_type_name_deduplicator.cc:23: StringPiece ExtractDirNameFromTaskContext(const char* type_name) { On 2016/03/29 12:49:05, petrcermak wrote: > The comment, method name and argument name all disagree with each other: > > "file name" vs. "TaskContext" vs. "type_name" > "category name" vs. "DirName" wdyt now? https://codereview.chromium.org/1784133002/diff/460001/base/trace_event/heap_... base/trace_event/heap_profiler_type_name_deduplicator.cc:27: // If |type_name| is a not a file path the seperator will not be found. On 2016/03/29 12:49:05, petrcermak wrote: > nit: add a comma after "path" Done. https://codereview.chromium.org/1784133002/diff/460001/base/trace_event/heap_... base/trace_event/heap_profiler_type_name_deduplicator.cc:28: // return the type name. On 2016/03/29 12:49:05, petrcermak wrote: > This is not a sentence, I suggest merging it with the first sentence: > > "... not be found, so the whole type name is returned." Done. https://codereview.chromium.org/1784133002/diff/460001/base/trace_event/heap_... File base/trace_event/heap_profiler_type_name_deduplicator_unittest.cc (right): https://codereview.chromium.org/1784133002/diff/460001/base/trace_event/heap_... base/trace_event/heap_profiler_type_name_deduplicator_unittest.cc:27: const char kFolders[] = "base/trace_event"; On 2016/03/29 12:49:05, petrcermak wrote: > I'd say kPath would be easier to understand. Done. https://codereview.chromium.org/1784133002/diff/460001/base/trace_event/heap_... base/trace_event/heap_profiler_type_name_deduplicator_unittest.cc:29: const char kFileNameWin[] = "..\\..\\base\\memory\\memory_win.cc"; On 2016/03/29 12:49:05, petrcermak wrote: > Why not kFileName as well? Done. https://codereview.chromium.org/1784133002/diff/460001/base/trace_event/heap_... base/trace_event/heap_profiler_type_name_deduplicator_unittest.cc:40: // checks if value gets inserted and the exported value for |type_name| is same On 2016/03/29 12:49:05, petrcermak wrote: > nit: s/if value/if the value/ and s/is same/is the same/ Done. https://codereview.chromium.org/1784133002/diff/460001/base/trace_event/heap_... base/trace_event/heap_profiler_type_name_deduplicator_unittest.cc:43: const char* exported_value) { On 2016/03/29 12:49:05, petrcermak wrote: > Don't you mean "expected"? yeah that makes sense. https://codereview.chromium.org/1784133002/diff/460001/components/scheduler/b... File components/scheduler/base/task_queue_manager.cc (right): https://codereview.chromium.org/1784133002/diff/460001/components/scheduler/b... components/scheduler/base/task_queue_manager.cc:12: #include "base/trace_event/trace_event.h" On 2016/03/29 12:49:05, petrcermak wrote: > Is this intentional? yes, i moved the macro to trace_event.h
Patchset #14 (id:480001) has been deleted
LGTM from my side (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 https://codereview.chromium.org/1784133002/diff/460001/base/trace_event/heap_... File base/trace_event/heap_profiler_type_name_deduplicator.cc (right): https://codereview.chromium.org/1784133002/diff/460001/base/trace_event/heap_... base/trace_event/heap_profiler_type_name_deduplicator.cc:23: StringPiece ExtractDirNameFromTaskContext(const char* type_name) { On 2016/03/29 18:32:45, ssid wrote: > On 2016/03/29 12:49:05, petrcermak wrote: > > The comment, method name and argument name all disagree with each other: > > > > "file name" vs. "TaskContext" vs. "type_name" > > "category name" vs. "DirName" > > wdyt now? I think it's much better, but the comment is now "begging" for an "Otherwise, ..." sentence (what if |type_name| is not task context). Also s/was/is/ (makes it sound less improbable).
https://codereview.chromium.org/1784133002/diff/500001/base/trace_event/heap_... File base/trace_event/heap_profiler_allocation_context_tracker.cc (right): https://codereview.chromium.org/1784133002/diff/500001/base/trace_event/heap_... base/trace_event/heap_profiler_allocation_context_tracker.cc:106: if (task_contexts_.size() < kMaxTaskDepth) Shouldn't this be a DCHECK(task_contexts.size() < kMaxTaskDepth); instead? With only NOTREACHED() in the else fork, that's effectively what it is. https://codereview.chromium.org/1784133002/diff/500001/base/trace_event/trace... File base/trace_event/trace_event.h (right): https://codereview.chromium.org/1784133002/diff/500001/base/trace_event/trace... base/trace_event/trace_event.h:388: #define TRACE_TASK_EXECUTION(run_function, task) \ I don't think this belongs in trace_event.h; typically that file should have implementation code and pure macros should be in trace_event_common.h. I would keep ScopedTaskExecutionEvent there and add a define for it (like TRACE_EVENT_API_SCOPED_TASK_EXECUTION_EVENT) and move TRACE_TASK_EXECUTION to trace_event_common.h. https://codereview.chromium.org/1784133002/diff/500001/base/trace_event/trace... base/trace_event/trace_event.h:389: TRACE_EVENT2("toplevel", (run_function), "src_file", \ Why are the parameter usages all wrapped by ()? We don't do that elsewhere, but maybe there's some reason I'm missing for it :). https://codereview.chromium.org/1784133002/diff/500001/base/trace_event/trace... base/trace_event/trace_event.h:392: trace_event_internal::ScopedTaskExecutionEvent event( \ INTERNAL_TRACE_EVENT_UID(event) to avoid conflicts with 'event' vars in code using this.
Patchset #15 (id:510001) has been deleted
Thanks, made changes. https://codereview.chromium.org/1784133002/diff/460001/base/trace_event/heap_... File base/trace_event/heap_profiler_type_name_deduplicator.cc (right): https://codereview.chromium.org/1784133002/diff/460001/base/trace_event/heap_... base/trace_event/heap_profiler_type_name_deduplicator.cc:23: StringPiece ExtractDirNameFromTaskContext(const char* type_name) { On 2016/03/29 18:42:13, petrcermak wrote: > On 2016/03/29 18:32:45, ssid wrote: > > On 2016/03/29 12:49:05, petrcermak wrote: > > > The comment, method name and argument name all disagree with each other: > > > > > > "file name" vs. "TaskContext" vs. "type_name" > > > "category name" vs. "DirName" > > > > wdyt now? > > I think it's much better, but the comment is now "begging" for an "Otherwise, > ..." sentence (what if |type_name| is not task context). Also s/was/is/ (makes > it sound less improbable). Done. https://codereview.chromium.org/1784133002/diff/500001/base/trace_event/heap_... File base/trace_event/heap_profiler_allocation_context_tracker.cc (right): https://codereview.chromium.org/1784133002/diff/500001/base/trace_event/heap_... base/trace_event/heap_profiler_allocation_context_tracker.cc:106: if (task_contexts_.size() < kMaxTaskDepth) On 2016/03/29 21:28:35, Oystein wrote: > Shouldn't this be a DCHECK(task_contexts.size() < kMaxTaskDepth); instead? With > only NOTREACHED() in the else fork, that's effectively what it is. if the code was: DCHECK(task_contexts.size() < kMaxTaskDepth); task_contexts_.push_back(context); in the release builds, it can try to do a pushback and cause an infite loop because of extra allocation and rentrancy. So, in release builds we just ignore the push_back and show wrong result instead of crashing or deadlock. Also, there is predecessor for this piece of code in AllocationContextTracker::PushPseudoStackFrame line 78. Does the same thing. https://codereview.chromium.org/1784133002/diff/500001/base/trace_event/trace... File base/trace_event/trace_event.h (right): https://codereview.chromium.org/1784133002/diff/500001/base/trace_event/trace... base/trace_event/trace_event.h:388: #define TRACE_TASK_EXECUTION(run_function, task) \ On 2016/03/29 21:28:35, Oystein wrote: > I don't think this belongs in trace_event.h; typically that file should have > implementation code and pure macros should be in trace_event_common.h. I would > keep ScopedTaskExecutionEvent there and add a define for it (like > TRACE_EVENT_API_SCOPED_TASK_EXECUTION_EVENT) and move TRACE_TASK_EXECUTION to > trace_event_common.h. Thanks I was really confused about trace_event.h and trace_event_common.h. I have added an INTERNAL macro instead of API since I am not planning to use this api otherwise. Did I get it correct? https://codereview.chromium.org/1784133002/diff/500001/base/trace_event/trace... base/trace_event/trace_event.h:389: TRACE_EVENT2("toplevel", (run_function), "src_file", \ On 2016/03/29 21:28:35, Oystein wrote: > Why are the parameter usages all wrapped by ()? We don't do that elsewhere, but > maybe there's some reason I'm missing for it :). Ah, it existed in the other file and I had copied it. It isn't required, removed. https://codereview.chromium.org/1784133002/diff/500001/base/trace_event/trace... base/trace_event/trace_event.h:392: trace_event_internal::ScopedTaskExecutionEvent event( \ On 2016/03/29 21:28:35, Oystein wrote: > INTERNAL_TRACE_EVENT_UID(event) to avoid conflicts with 'event' vars in code > using this. Done.
https://codereview.chromium.org/1784133002/diff/500001/base/trace_event/heap_... File base/trace_event/heap_profiler_allocation_context_tracker.cc (right): https://codereview.chromium.org/1784133002/diff/500001/base/trace_event/heap_... base/trace_event/heap_profiler_allocation_context_tracker.cc:106: if (task_contexts_.size() < kMaxTaskDepth) On 2016/03/29 23:04:19, ssid wrote: > On 2016/03/29 21:28:35, Oystein wrote: > > Shouldn't this be a DCHECK(task_contexts.size() < kMaxTaskDepth); instead? > With > > only NOTREACHED() in the else fork, that's effectively what it is. > > if the code was: > DCHECK(task_contexts.size() < kMaxTaskDepth); > task_contexts_.push_back(context); > > in the release builds, it can try to do a pushback and cause an infite loop > because of extra allocation and rentrancy. So, in release builds we just ignore > the push_back and show wrong result instead of crashing or deadlock. > > Also, there is predecessor for this piece of code in > AllocationContextTracker::PushPseudoStackFrame line 78. Does the same thing. Makes sense; thanks! https://codereview.chromium.org/1784133002/diff/500001/base/trace_event/trace... File base/trace_event/trace_event.h (right): https://codereview.chromium.org/1784133002/diff/500001/base/trace_event/trace... base/trace_event/trace_event.h:388: #define TRACE_TASK_EXECUTION(run_function, task) \ On 2016/03/29 23:04:19, ssid wrote: > On 2016/03/29 21:28:35, Oystein wrote: > > I don't think this belongs in trace_event.h; typically that file should have > > implementation code and pure macros should be in trace_event_common.h. I would > > keep ScopedTaskExecutionEvent there and add a define for it (like > > TRACE_EVENT_API_SCOPED_TASK_EXECUTION_EVENT) and move TRACE_TASK_EXECUTION to > > trace_event_common.h. > > Thanks I was really confused about trace_event.h and trace_event_common.h. > I have added an INTERNAL macro instead of API since I am not planning to use > this api otherwise. Did I get it correct? Almost! I mainly don't think trace_event.h should have the macro at all, for consistency (trace_event.h should be as minimal as possible and not contain stuff like hardcoded categories). I'd rather do, in trace_event.h (instead of the macro): #define INTERNAL_TRACE_SCOPED_TASK_EXECUTION_EVENT trace_event_internal::ScopedTaskExecutionEvent and then the macro in trace_event_common.h would be something like this: #define TRACE_TASK_EXECUTION(run_function, task) \ TRACE_EVENT2("toplevel", run_function, "src_file", \ task.posted_from.file_name(), "src_func", \ task.posted_from.function_name()); \ INTERNAL_TRACE_SCOPED_TASK_EXECUTION_EVENT INTERNAL_TRACE_EVENT_UID( \ task_event)(task.posted_from.file_name());
Patchset #15 (id:530001) has been deleted
> Almost! I mainly don't think trace_event.h should have the macro at all, for
> consistency (trace_event.h should be as minimal as possible and not contain
> stuff like hardcoded categories).
>
> I'd rather do, in trace_event.h (instead of the macro):
> #define INTERNAL_TRACE_SCOPED_TASK_EXECUTION_EVENT
> trace_event_internal::ScopedTaskExecutionEvent
>
> and then the macro in trace_event_common.h would be something like this:
> #define TRACE_TASK_EXECUTION(run_function, task) \
> TRACE_EVENT2("toplevel", run_function, "src_file", \
> task.posted_from.file_name(), "src_func", \
> task.posted_from.function_name()); \
> INTERNAL_TRACE_SCOPED_TASK_EXECUTION_EVENT INTERNAL_TRACE_EVENT_UID( \
> task_event)(task.posted_from.file_name());
Thanks for your patience. I was trying to minimize changed in common.h. Made
changes as suggested.
I also made it API since it can be used in other places if written like this.
Patchset #16 (id:570001) has been deleted
On 2016/03/30 00:01:23, ssid wrote:
> > Almost! I mainly don't think trace_event.h should have the macro at all, for
> > consistency (trace_event.h should be as minimal as possible and not contain
> > stuff like hardcoded categories).
> >
> > I'd rather do, in trace_event.h (instead of the macro):
> > #define INTERNAL_TRACE_SCOPED_TASK_EXECUTION_EVENT
> > trace_event_internal::ScopedTaskExecutionEvent
> >
> > and then the macro in trace_event_common.h would be something like this:
> > #define TRACE_TASK_EXECUTION(run_function, task) \
> > TRACE_EVENT2("toplevel", run_function, "src_file", \
> > task.posted_from.file_name(), "src_func", \
> > task.posted_from.function_name()); \
> > INTERNAL_TRACE_SCOPED_TASK_EXECUTION_EVENT INTERNAL_TRACE_EVENT_UID( \
> > task_event)(task.posted_from.file_name());
>
> Thanks for your patience. I was trying to minimize changed in common.h. Made
> changes as suggested.
> I also made it API since it can be used in other places if written like this.
Great, thanks!
trace_event.h and trace_event_common lgtm!
This change is mostly to reflect trace event movement in base and to be consistent all task execution uses the same macro. +jochen for tracing macro changes in components/ +thakis for tracing macro changes in base/ https://codereview.chromium.org/1784133002/diff/590001/components/scheduler/b... File components/scheduler/base/task_queue_manager.cc (right): https://codereview.chromium.org/1784133002/diff/590001/components/scheduler/b... components/scheduler/base/task_queue_manager.cc:12: #include "base/trace_event/trace_event.h" This because the trace event was moved from task_annotator to trace_event.h
ssid@chromium.org changed reviewers: + jochen@chromium.org, thakis@chromium.org
<Actually add reviewers this time> This change is mostly to reflect trace event movement in base and to be consistent all task execution uses the same macro. +jochen for tracing macro changes in components/ +thakis for tracing macro changes in base/
lgtm
//base/trace_event LGTM with some minor comments (plz look at them before landing) https://codereview.chromium.org/1784133002/diff/590001/base/trace_event/heap_... File base/trace_event/heap_profiler_allocation_context_tracker.h (right): https://codereview.chromium.org/1784133002/diff/590001/base/trace_event/heap_... base/trace_event/heap_profiler_allocation_context_tracker.h:63: void PushCurrentTaskContext(const char* context); I aware about the fact that I am a real pain on naming. Since this is either a task name or file path (don't remember which ATM) why don't we make it explicit? "context" makes it cryptic to understand what it really is. It's a string, should be straightforward to tell what the content of such string is when looking at the code. https://codereview.chromium.org/1784133002/diff/590001/base/trace_event/heap_... base/trace_event/heap_profiler_allocation_context_tracker.h:83: // pseudo stack to study the heap. s/to study the heap/to cluster allocations/ https://codereview.chromium.org/1784133002/diff/590001/base/trace_event/heap_... File base/trace_event/heap_profiler_type_name_deduplicator.cc (right): https://codereview.chromium.org/1784133002/diff/590001/base/trace_event/heap_... base/trace_event/heap_profiler_type_name_deduplicator.cc:25: StringPiece result = type_name; StringPiece result(type_name); no need to disturb the StringPiece copy-ctor :) https://codereview.chromium.org/1784133002/diff/590001/base/trace_event/heap_... base/trace_event/heap_profiler_type_name_deduplicator.cc:40: result.remove_prefix(parent_seperator + 1); I think here is fine to assume that .. is always followed by a 1-char separator (either \ or /) so I'd just remove_prefix(kParentDirectory.length + 2) and keep it simple https://codereview.chromium.org/1784133002/diff/590001/base/trace_event/heap_... File base/trace_event/heap_profiler_type_name_deduplicator_unittest.cc (right): https://codereview.chromium.org/1784133002/diff/590001/base/trace_event/heap_... base/trace_event/heap_profiler_type_name_deduplicator_unittest.cc:26: const char kFileName[] = "../../base/trace_event/trace_log.cc"; maybe kTaskFileName and kTaskPath will make it easier to understand what these constants are for if this file grows. https://codereview.chromium.org/1784133002/diff/590001/base/trace_event/heap_... base/trace_event/heap_profiler_type_name_deduplicator_unittest.cc:42: void InsertAndTestExportedValue(const char* type_name, maybe TestInsertTypeAndReadback? https://codereview.chromium.org/1784133002/diff/590001/base/trace_event/heap_... base/trace_event/heap_profiler_type_name_deduplicator_unittest.cc:86: InsertAndTestExportedValue(kNeedsEscape, "\"quotes\""); I guess this can be InsertAndTestExportedValue(kNeedsEscape, kNeedsEscape); :) https://codereview.chromium.org/1784133002/diff/590001/base/trace_event/trace... File base/trace_event/trace_event.h (right): https://codereview.chromium.org/1784133002/diff/590001/base/trace_event/trace... base/trace_event/trace_event.h:1057: ScopedTaskExecutionEvent(const char* task_context) : context_(task_context) { +explicit https://codereview.chromium.org/1784133002/diff/590001/components/timers/alar... File components/timers/alarm_timer_chromeos.cc (right): https://codereview.chromium.org/1784133002/diff/590001/components/timers/alar... components/timers/alarm_timer_chromeos.cc:437: TRACE_TASK_EXECUTION("AlarmTimer::OnTimerFired", (*pending_user_task)); why do we need parentheses here? smells like we need paren in the macro?
https://codereview.chromium.org/1784133002/diff/590001/base/trace_event/heap_... File base/trace_event/heap_profiler_allocation_context_tracker.h (right): https://codereview.chromium.org/1784133002/diff/590001/base/trace_event/heap_... base/trace_event/heap_profiler_allocation_context_tracker.h:63: void PushCurrentTaskContext(const char* context); On 2016/03/31 15:37:16, Primiano Tucci wrote: > I aware about the fact that I am a real pain on naming. > Since this is either a task name or file path (don't remember which ATM) why > don't we make it explicit? > "context" makes it cryptic to understand what it really is. > It's a string, should be straightforward to tell what the content of such string > is when looking at the code. Yes but I feel this will be reused for some other reason very soon. That time there will not be a need to rename it. For example if we want to add IPC names as context instead of stack frame, or even add the posted function name instead of file name, this can still be used. I could also add context like WebKit or skia. Do you strongly prefer that I change it to task_posted_file_name?
https://codereview.chromium.org/1784133002/diff/590001/base/trace_event/heap_... File base/trace_event/heap_profiler_allocation_context_tracker.h (right): https://codereview.chromium.org/1784133002/diff/590001/base/trace_event/heap_... base/trace_event/heap_profiler_allocation_context_tracker.h:63: void PushCurrentTaskContext(const char* context); On 2016/03/31 16:02:49, ssid wrote: > On 2016/03/31 15:37:16, Primiano Tucci wrote: > > I aware about the fact that I am a real pain on naming. > > Since this is either a task name or file path (don't remember which ATM) why > > don't we make it explicit? > > "context" makes it cryptic to understand what it really is. > > It's a string, should be straightforward to tell what the content of such > string > > is when looking at the code. > > Yes but I feel this will be reused for some other reason very soon. That time > there will not be a need to rename it. For example if we want to add IPC names > as context instead of stack frame, or even add the posted function name instead > of file name, this can still be used. I could also add context like WebKit or > skia. Do you strongly prefer that I change it to task_posted_file_name? Ahh ok, if you have upcoming use cases in mind it's ok leaving it as it is (but then you have to figure out something below in the logic that strips the paths)
base/ lgtm https://codereview.chromium.org/1784133002/diff/590001/base/threading/worker_... File base/threading/worker_pool_win.cc (right): https://codereview.chromium.org/1784133002/diff/590001/base/threading/worker_... base/threading/worker_pool_win.cc:24: TRACE_TASK_EXECUTION("WorkerThread::ThreadMain::Run", (*pending_task)); nit: no parens around *pending_task https://codereview.chromium.org/1784133002/diff/590001/components/timers/alar... File components/timers/alarm_timer_chromeos.cc (right): https://codereview.chromium.org/1784133002/diff/590001/components/timers/alar... components/timers/alarm_timer_chromeos.cc:437: TRACE_TASK_EXECUTION("AlarmTimer::OnTimerFired", (*pending_user_task)); On 2016/03/31 15:37:16, Primiano Tucci wrote: > why do we need parentheses here? smells like we need paren in the macro? yup, +1
https://codereview.chromium.org/1784133002/diff/590001/base/threading/worker_... File base/threading/worker_pool_win.cc (right): https://codereview.chromium.org/1784133002/diff/590001/base/threading/worker_... base/threading/worker_pool_win.cc:24: TRACE_TASK_EXECUTION("WorkerThread::ThreadMain::Run", (*pending_task)); On 2016/03/31 16:12:48, Nico wrote: > nit: no parens around *pending_task moved to macro https://codereview.chromium.org/1784133002/diff/590001/base/trace_event/heap_... File base/trace_event/heap_profiler_allocation_context_tracker.h (right): https://codereview.chromium.org/1784133002/diff/590001/base/trace_event/heap_... base/trace_event/heap_profiler_allocation_context_tracker.h:63: void PushCurrentTaskContext(const char* context); On 2016/03/31 16:10:59, Primiano Tucci wrote: > On 2016/03/31 16:02:49, ssid wrote: > > On 2016/03/31 15:37:16, Primiano Tucci wrote: > > > I aware about the fact that I am a real pain on naming. > > > Since this is either a task name or file path (don't remember which ATM) why > > > don't we make it explicit? > > > "context" makes it cryptic to understand what it really is. > > > It's a string, should be straightforward to tell what the content of such > > string > > > is when looking at the code. > > > > Yes but I feel this will be reused for some other reason very soon. That time > > there will not be a need to rename it. For example if we want to add IPC names > > as context instead of stack frame, or even add the posted function name > instead > > of file name, this can still be used. I could also add context like WebKit or > > skia. Do you strongly prefer that I change it to task_posted_file_name? > > Ahh ok, if you have upcoming use cases in mind it's ok leaving it as it is (but > then you have to figure out something below in the logic that strips the paths) I changed the comment and function name. wdyt? https://codereview.chromium.org/1784133002/diff/590001/base/trace_event/heap_... base/trace_event/heap_profiler_allocation_context_tracker.h:83: // pseudo stack to study the heap. On 2016/03/31 15:37:16, Primiano Tucci wrote: > s/to study the heap/to cluster allocations/ Done. https://codereview.chromium.org/1784133002/diff/590001/base/trace_event/heap_... File base/trace_event/heap_profiler_type_name_deduplicator.cc (right): https://codereview.chromium.org/1784133002/diff/590001/base/trace_event/heap_... base/trace_event/heap_profiler_type_name_deduplicator.cc:25: StringPiece result = type_name; On 2016/03/31 15:37:16, Primiano Tucci wrote: > StringPiece result(type_name); > > no need to disturb the StringPiece copy-ctor :) Done. https://codereview.chromium.org/1784133002/diff/590001/base/trace_event/heap_... base/trace_event/heap_profiler_type_name_deduplicator.cc:40: result.remove_prefix(parent_seperator + 1); On 2016/03/31 15:37:16, Primiano Tucci wrote: > I think here is fine to assume that .. is always followed by a 1-char separator > (either \ or /) so I'd just remove_prefix(kParentDirectory.length + 2) and keep > it simple Done. https://codereview.chromium.org/1784133002/diff/590001/base/trace_event/heap_... File base/trace_event/heap_profiler_type_name_deduplicator_unittest.cc (right): https://codereview.chromium.org/1784133002/diff/590001/base/trace_event/heap_... base/trace_event/heap_profiler_type_name_deduplicator_unittest.cc:26: const char kFileName[] = "../../base/trace_event/trace_log.cc"; On 2016/03/31 15:37:16, Primiano Tucci wrote: > maybe kTaskFileName and kTaskPath will make it easier to understand what these > constants are for if this file grows. Done. https://codereview.chromium.org/1784133002/diff/590001/base/trace_event/heap_... base/trace_event/heap_profiler_type_name_deduplicator_unittest.cc:42: void InsertAndTestExportedValue(const char* type_name, On 2016/03/31 15:37:16, Primiano Tucci wrote: > maybe TestInsertTypeAndReadback? Done. https://codereview.chromium.org/1784133002/diff/590001/base/trace_event/heap_... base/trace_event/heap_profiler_type_name_deduplicator_unittest.cc:86: InsertAndTestExportedValue(kNeedsEscape, "\"quotes\""); On 2016/03/31 15:37:16, Primiano Tucci wrote: > I guess this can be > InsertAndTestExportedValue(kNeedsEscape, kNeedsEscape); :) I thought u wanted to make it explicit. fixed. https://codereview.chromium.org/1784133002/diff/590001/base/trace_event/trace... File base/trace_event/trace_event.h (right): https://codereview.chromium.org/1784133002/diff/590001/base/trace_event/trace... base/trace_event/trace_event.h:1057: ScopedTaskExecutionEvent(const char* task_context) : context_(task_context) { On 2016/03/31 15:37:16, Primiano Tucci wrote: > +explicit Done. https://codereview.chromium.org/1784133002/diff/590001/components/timers/alar... File components/timers/alarm_timer_chromeos.cc (right): https://codereview.chromium.org/1784133002/diff/590001/components/timers/alar... components/timers/alarm_timer_chromeos.cc:437: TRACE_TASK_EXECUTION("AlarmTimer::OnTimerFired", (*pending_user_task)); On 2016/03/31 16:12:48, Nico wrote: > On 2016/03/31 15:37:16, Primiano Tucci wrote: > > why do we need parentheses here? smells like we need paren in the macro? > > yup, +1 yes, I was thinking it is better to add here instead of macro. moved it now.
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/1784133002/630001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1784133002/630001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
https://codereview.chromium.org/1784133002/diff/590001/base/trace_event/heap_... File base/trace_event/heap_profiler_allocation_context_tracker.h (right): https://codereview.chromium.org/1784133002/diff/590001/base/trace_event/heap_... base/trace_event/heap_profiler_allocation_context_tracker.h:63: void PushCurrentTaskContext(const char* context); On 2016/04/01 04:34:54, ssid wrote: > On 2016/03/31 16:10:59, Primiano Tucci wrote: > > On 2016/03/31 16:02:49, ssid wrote: > > > On 2016/03/31 15:37:16, Primiano Tucci wrote: > > > > I aware about the fact that I am a real pain on naming. > > > > Since this is either a task name or file path (don't remember which ATM) > why > > > > don't we make it explicit? > > > > "context" makes it cryptic to understand what it really is. > > > > It's a string, should be straightforward to tell what the content of such > > > string > > > > is when looking at the code. > > > > > > Yes but I feel this will be reused for some other reason very soon. That > time > > > there will not be a need to rename it. For example if we want to add IPC > names > > > as context instead of stack frame, or even add the posted function name > > instead > > > of file name, this can still be used. I could also add context like WebKit > or > > > skia. Do you strongly prefer that I change it to task_posted_file_name? > > > > Ahh ok, if you have upcoming use cases in mind it's ok leaving it as it is > (but > > then you have to figure out something below in the logic that strips the > paths) > > I changed the comment and function name. wdyt? Much better, thanks.
The CQ bit was checked by primiano@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from petrcermak@chromium.org, primiano@chromium.org, jochen@chromium.org, thakis@chromium.org, oysteine@chromium.org Link to the patchset: https://codereview.chromium.org/1784133002/#ps630001 (title: "nit.")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1784133002/630001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1784133002/630001
Message was sent while issue was closed.
Description was changed from ========== [tracing] Adding task information to heap profiler This CL adds more information to the malloc heap profiler added in crrev.com/1675183006. The heap profiler only shows trace events and all the tasks show up as "MessageLoop::Runtask". This CL does: - Moves TRACE_TASK_EXECUTION macro from task_annotator to trace_event.h. - Adds extra scoped trace event to track the task context in heap profiler. - The folder where the task was posted from is used to categorize allocations. - Uses the type_name for context temporarily till we define a new context dimension for allocations. BUG=594803 ========== to ========== [tracing] Adding task information to heap profiler This CL adds more information to the malloc heap profiler added in crrev.com/1675183006. The heap profiler only shows trace events and all the tasks show up as "MessageLoop::Runtask". This CL does: - Moves TRACE_TASK_EXECUTION macro from task_annotator to trace_event.h. - Adds extra scoped trace event to track the task context in heap profiler. - The folder where the task was posted from is used to categorize allocations. - Uses the type_name for context temporarily till we define a new context dimension for allocations. BUG=594803 ==========
Message was sent while issue was closed.
Committed patchset #18 (id:630001)
Message was sent while issue was closed.
Description was changed from ========== [tracing] Adding task information to heap profiler This CL adds more information to the malloc heap profiler added in crrev.com/1675183006. The heap profiler only shows trace events and all the tasks show up as "MessageLoop::Runtask". This CL does: - Moves TRACE_TASK_EXECUTION macro from task_annotator to trace_event.h. - Adds extra scoped trace event to track the task context in heap profiler. - The folder where the task was posted from is used to categorize allocations. - Uses the type_name for context temporarily till we define a new context dimension for allocations. BUG=594803 ========== to ========== [tracing] Adding task information to heap profiler This CL adds more information to the malloc heap profiler added in crrev.com/1675183006. The heap profiler only shows trace events and all the tasks show up as "MessageLoop::Runtask". This CL does: - Moves TRACE_TASK_EXECUTION macro from task_annotator to trace_event.h. - Adds extra scoped trace event to track the task context in heap profiler. - The folder where the task was posted from is used to categorize allocations. - Uses the type_name for context temporarily till we define a new context dimension for allocations. BUG=594803 Committed: https://crrev.com/533c402f8b21e503ebcaf3f0de427f41076b4de1 Cr-Commit-Position: refs/heads/master@{#384521} ==========
Message was sent while issue was closed.
Patchset 18 (id:??) landed as https://crrev.com/533c402f8b21e503ebcaf3f0de427f41076b4de1 Cr-Commit-Position: refs/heads/master@{#384521}
Message was sent while issue was closed.
A revert of this CL (patchset #18 id:630001) has been created in https://codereview.chromium.org/1846333002/ by kbr@chromium.org. The reason for reverting is: Caused flakiness of trace_test; see Issue 599794. .
Message was sent while issue was closed.
Patchset #2 (id:60001) has been deleted
Message was sent while issue was closed.
Patchset #2 (id:100001) has been deleted |
