Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1950313005:
[tracing] Move and rename TRACE_EVENT_API_TASK_EXECUTION_EVENT (Closed)
Description[tracing] Move and rename TRACE_EVENT_API_TASK_EXECUTION_EVENT
The heap profiler specific macros are moved to heap_profiler.h. This CL
also gives a better name to the api. Note that the trace_event.h now
includes heap_profiler.h instead of context_tracker.h and contains the
implementation of TRACE_TASK_EXECUTION because it depends on
base::Location and AllocationContextTracker.
BUG=609882
Committed: https://crrev.com/abb35f9934c2950cc4f8e0f67afb43e632773018
Cr-Commit-Position: refs/heads/master@{#393616}
Patch Set 1 : #Patch Set 2 : #
Total comments: 5
Patch Set 3 : Rename. #
Total comments: 2
Patch Set 4 : Add a redirection from the common file. #Patch Set 5 : fix unittest. #
Messages
Total messages: 26 (12 generated)
Description was changed from ========== [tracing] Move and rename TRACE_EVENT_API_TASK_EXECUTION_EVENT The heap profiler specific macros are moved to heap_profiler.h. This CL also gives a better name to the api. Note that the trace_event.h now includes heap_profiler.h instead of context_tracker.h. This is because the trace_event_common.h uses the heap profiler macro. BUG=609882 ========== to ========== [tracing] Move and rename TRACE_EVENT_API_TASK_EXECUTION_EVENT The heap profiler specific macro is moved to heap_profiler.h. This CL also gives a better name to the api. Note that the trace_event.h now includes heap_profiler.h instead of context_tracker.h. This is because the trace_event_common.h uses the heap profiler macro. BUG=609882 ==========
Patchset #1 (id:1) has been deleted
ssid@chromium.org changed reviewers: + primiano@chromium.org
ptal, thanks.
https://codereview.chromium.org/1950313005/diff/30001/base/trace_event/common... File base/trace_event/common/trace_event_common.h (right): https://codereview.chromium.org/1950313005/diff/30001/base/trace_event/common... base/trace_event/common/trace_event_common.h:935: HEAP_PROFILER_API_SCOPED_TASK_EXECUTION_EVENT INTERNAL_TRACE_EVENT_UID( \ I'd still name it with TRACE_ prefix so folks realize that's still about tracing. e.g. TRACE_HEAP_PROFILER_SCOPED_TASK_EXECUTION (without event, as this is not an event) https://codereview.chromium.org/1950313005/diff/30001/base/trace_event/trace_... File base/trace_event/trace_event.h (right): https://codereview.chromium.org/1950313005/diff/30001/base/trace_event/trace_... base/trace_event/trace_event.h:21: #include "base/trace_event/heap_profiler.h" // For SCOPED_TASK_EXECUTION_EVENT Hmm this should go to base/trace_event/common/trace_event_common.h, not here right?
Thanks, replies inline. https://codereview.chromium.org/1950313005/diff/30001/base/trace_event/common... File base/trace_event/common/trace_event_common.h (right): https://codereview.chromium.org/1950313005/diff/30001/base/trace_event/common... base/trace_event/common/trace_event_common.h:935: HEAP_PROFILER_API_SCOPED_TASK_EXECUTION_EVENT INTERNAL_TRACE_EVENT_UID( \ On 2016/05/09 17:36:45, Primiano Tucci wrote: > I'd still name it with TRACE_ prefix so folks realize that's still about > tracing. e.g. > TRACE_HEAP_PROFILER_SCOPED_TASK_EXECUTION (without event, as this is not an > event) Done. https://codereview.chromium.org/1950313005/diff/30001/base/trace_event/trace_... File base/trace_event/trace_event.h (right): https://codereview.chromium.org/1950313005/diff/30001/base/trace_event/trace_... base/trace_event/trace_event.h:21: #include "base/trace_event/heap_profiler.h" // For SCOPED_TASK_EXECUTION_EVENT On 2016/05/09 17:36:45, Primiano Tucci wrote: > Hmm this should go to base/trace_event/common/trace_event_common.h, not here > right? trace_event_common.h cannot include any file form base since it is included in other projects like v8. If any other projects want to use the TASK_EXEC api then they should define SCOPED_TASK_EXECUTION_EVENT. This is how trace_event currently works. Can you suggest better alternatives? Or should i ask oysteine@?
primiano@chromium.org changed reviewers: + oysteine@chromium.org
+oysteine: does it make sense to you to move TRACE_TASK_EXECUTION into trace_Event.h (see below for more context). Today is never used outside of chrome (and assumes knowledge of base::location which is really chrome specific) https://codereview.chromium.org/1950313005/diff/30001/base/trace_event/trace_... File base/trace_event/trace_event.h (right): https://codereview.chromium.org/1950313005/diff/30001/base/trace_event/trace_... base/trace_event/trace_event.h:21: #include "base/trace_event/heap_profiler.h" // For SCOPED_TASK_EXECUTION_EVENT On 2016/05/09 23:49:11, ssid wrote: > On 2016/05/09 17:36:45, Primiano Tucci wrote: > > Hmm this should go to base/trace_event/common/trace_event_common.h, not here > > right? > > trace_event_common.h cannot include any file form base since it is included in > other projects like v8. If any other projects want to use the TASK_EXEC api then > they should define SCOPED_TASK_EXECUTION_EVENT. Oh you are right about trace_event_common.h But this seems to suggests that the entire TRACE_TASK_EXECUTION should be defined in trace_event.h and not in common.h, As that macro can't be possibly used outside of chrome.. And in turn trace_event.h should include heap_profiler.h Right now you are adding an include which is not needed, but you put there just because you know that this file is going to be pulled by trace_event_common.h Let's just put both pieces in trace_event.h at this point.
On 2016/05/10 16:03:53, Primiano Tucci wrote: > +oysteine: does it make sense to you to move TRACE_TASK_EXECUTION into > trace_Event.h (see below for more context). > Today is never used outside of chrome (and assumes knowledge of base::location > which is really chrome specific) > Ping. > https://codereview.chromium.org/1950313005/diff/30001/base/trace_event/trace_... > File base/trace_event/trace_event.h (right): > > https://codereview.chromium.org/1950313005/diff/30001/base/trace_event/trace_... > base/trace_event/trace_event.h:21: #include "base/trace_event/heap_profiler.h" > // For SCOPED_TASK_EXECUTION_EVENT > On 2016/05/09 23:49:11, ssid wrote: > > On 2016/05/09 17:36:45, Primiano Tucci wrote: > > > Hmm this should go to base/trace_event/common/trace_event_common.h, not here > > > right? > > > > trace_event_common.h cannot include any file form base since it is included in > > other projects like v8. If any other projects want to use the TASK_EXEC api > then > > they should define SCOPED_TASK_EXECUTION_EVENT. > > Oh you are right about trace_event_common.h > But this seems to suggests that the entire TRACE_TASK_EXECUTION should be > defined in trace_event.h and not in common.h, As that macro can't be possibly > used outside of chrome.. > And in turn trace_event.h should include heap_profiler.h > Right now you are adding an include which is not needed, but you put there just > because you know that this file is going to be pulled by trace_event_common.h > Let's just put both pieces in trace_event.h at this point.
On 2016/05/10 16:03:53, Primiano Tucci wrote: > +oysteine: does it make sense to you to move TRACE_TASK_EXECUTION into > trace_Event.h (see below for more context). > Today is never used outside of chrome (and assumes knowledge of base::location > which is really chrome specific) > > https://codereview.chromium.org/1950313005/diff/30001/base/trace_event/trace_... > File base/trace_event/trace_event.h (right): > > https://codereview.chromium.org/1950313005/diff/30001/base/trace_event/trace_... > base/trace_event/trace_event.h:21: #include "base/trace_event/heap_profiler.h" > // For SCOPED_TASK_EXECUTION_EVENT > On 2016/05/09 23:49:11, ssid wrote: > > On 2016/05/09 17:36:45, Primiano Tucci wrote: > > > Hmm this should go to base/trace_event/common/trace_event_common.h, not here > > > right? > > > > trace_event_common.h cannot include any file form base since it is included in > > other projects like v8. If any other projects want to use the TASK_EXEC api > then > > they should define SCOPED_TASK_EXECUTION_EVENT. > > Oh you are right about trace_event_common.h > But this seems to suggests that the entire TRACE_TASK_EXECUTION should be > defined in trace_event.h and not in common.h, As that macro can't be possibly > used outside of chrome.. > And in turn trace_event.h should include heap_profiler.h > Right now you are adding an include which is not needed, but you put there just > because you know that this file is going to be pulled by trace_event_common.h > Let's just put both pieces in trace_event.h at this point. Hmm I think the outer macro should still be defined in trace_event_common.h, to avoid the signature/usage itself going out of sync between implementations. But I agree that it shouldn't be directly using base/location.h stuff as it is now. How about having an outer macro in trace_event_common.h which just directly passes all of its parameters to an inner one in trace_event.h? The extra level of indirection might seem awkward, but lets us keep the shared top-level API still. https://codereview.chromium.org/1950313005/diff/40001/base/trace_event/trace_... File base/trace_event/trace_event.h (right): https://codereview.chromium.org/1950313005/diff/40001/base/trace_event/trace_... base/trace_event/trace_event.h:21: #include "base/trace_event/heap_profiler.h" // For SCOPED_TASK_EXECUTION I don't think we normally specify reasons for inclusions like this.
Description was changed from ========== [tracing] Move and rename TRACE_EVENT_API_TASK_EXECUTION_EVENT The heap profiler specific macro is moved to heap_profiler.h. This CL also gives a better name to the api. Note that the trace_event.h now includes heap_profiler.h instead of context_tracker.h. This is because the trace_event_common.h uses the heap profiler macro. BUG=609882 ========== to ========== [tracing] Move and rename TRACE_EVENT_API_TASK_EXECUTION_EVENT The heap profiler specific macros are moved to heap_profiler.h. This CL also gives a better name to the api. Note that the trace_event.h now includes heap_profiler.h instead of context_tracker.h and contains the implementation of TRACE_TASK_EXECUTION because it depends on base::Location and AllocationContextTracker. BUG=609882 ==========
Thanks, made suggested changes. PTAL. https://codereview.chromium.org/1950313005/diff/40001/base/trace_event/trace_... File base/trace_event/trace_event.h (right): https://codereview.chromium.org/1950313005/diff/40001/base/trace_event/trace_... base/trace_event/trace_event.h:21: #include "base/trace_event/heap_profiler.h" // For SCOPED_TASK_EXECUTION On 2016/05/12 19:56:50, Oystein wrote: > I don't think we normally specify reasons for inclusions like this. I only added this because in this patch the file is included but the other file uses it.
On 2016/05/12 22:09:42, ssid wrote: > Thanks, made suggested changes. PTAL. > > https://codereview.chromium.org/1950313005/diff/40001/base/trace_event/trace_... > File base/trace_event/trace_event.h (right): > > https://codereview.chromium.org/1950313005/diff/40001/base/trace_event/trace_... > base/trace_event/trace_event.h:21: #include "base/trace_event/heap_profiler.h" > // For SCOPED_TASK_EXECUTION > On 2016/05/12 19:56:50, Oystein wrote: > > I don't think we normally specify reasons for inclusions like this. > > I only added this because in this patch the file is included but the other file > uses it. LGTM from my viewpoint (heap profiler stuff) if Oystein is happy with the header layout.
Great, lgtm!
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/1950313005/60001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1950313005/60001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: cast_shell_linux on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/cast_shell_linu...)
Patchset #5 (id:80001) has been deleted
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, oysteine@chromium.org Link to the patchset: https://codereview.chromium.org/1950313005/#ps100001 (title: "fix unittest.")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1950313005/100001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1950313005/100001
Message was sent while issue was closed.
Description was changed from ========== [tracing] Move and rename TRACE_EVENT_API_TASK_EXECUTION_EVENT The heap profiler specific macros are moved to heap_profiler.h. This CL also gives a better name to the api. Note that the trace_event.h now includes heap_profiler.h instead of context_tracker.h and contains the implementation of TRACE_TASK_EXECUTION because it depends on base::Location and AllocationContextTracker. BUG=609882 ========== to ========== [tracing] Move and rename TRACE_EVENT_API_TASK_EXECUTION_EVENT The heap profiler specific macros are moved to heap_profiler.h. This CL also gives a better name to the api. Note that the trace_event.h now includes heap_profiler.h instead of context_tracker.h and contains the implementation of TRACE_TASK_EXECUTION because it depends on base::Location and AllocationContextTracker. BUG=609882 ==========
Message was sent while issue was closed.
Committed patchset #5 (id:100001)
Message was sent while issue was closed.
Description was changed from ========== [tracing] Move and rename TRACE_EVENT_API_TASK_EXECUTION_EVENT The heap profiler specific macros are moved to heap_profiler.h. This CL also gives a better name to the api. Note that the trace_event.h now includes heap_profiler.h instead of context_tracker.h and contains the implementation of TRACE_TASK_EXECUTION because it depends on base::Location and AllocationContextTracker. BUG=609882 ========== to ========== [tracing] Move and rename TRACE_EVENT_API_TASK_EXECUTION_EVENT The heap profiler specific macros are moved to heap_profiler.h. This CL also gives a better name to the api. Note that the trace_event.h now includes heap_profiler.h instead of context_tracker.h and contains the implementation of TRACE_TASK_EXECUTION because it depends on base::Location and AllocationContextTracker. BUG=609882 Committed: https://crrev.com/abb35f9934c2950cc4f8e0f67afb43e632773018 Cr-Commit-Position: refs/heads/master@{#393616} ==========
Message was sent while issue was closed.
Patchset 5 (id:??) landed as https://crrev.com/abb35f9934c2950cc4f8e0f67afb43e632773018 Cr-Commit-Position: refs/heads/master@{#393616} |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
