Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2695833004:
Use TRACK_RUN_IN_THIS_SCOPED_REGION for heap profiler (Closed)
DescriptionUse TRACK_RUN_IN_THIS_SCOPED_REGION for heap profiler
Currently the heap profiler records the current task context using the
posted from location. But for ipc messages all the tasks are posted
from ipc.h. TRACK_RUN_IN_THIS_SCOPED_REGION solves the same problem for
showing the posted from info in about:profiler. Use this macro for heap
profiling as well.
BUG=594803
Review-Url: https://codereview.chromium.org/2695833004
Cr-Commit-Position: refs/heads/master@{#450880}
Committed: https://chromium.googlesource.com/chromium/src/+/27cd72d3ccb96dc69f03b968cb47aae4801a8db1
Patch Set 1 #Patch Set 2 : nit. #
Total comments: 4
Patch Set 3 : Use __COUNTER__. #
Total comments: 2
Patch Set 4 : fix comment. #Messages
Total messages: 36 (25 generated)
ssid@chromium.org changed reviewers: + dcheng@chromium.org
+dcheng ptal, Thanks! The formatting seems to be off and it's the git cl format. and presubmit fails if not.
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/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
I think there might be a typo in the CL description, as I don't understand this line: TRACK_RUN_IN_THIS_SCOPED_REGION for the same reason to show the posted from info in about:profiler. https://codereview.chromium.org/2695833004/diff/20001/base/profiler/scoped_pr... File base/profiler/scoped_profile.h (right): https://codereview.chromium.org/2695833004/diff/20001/base/profiler/scoped_pr... base/profiler/scoped_profile.h:27: #define LINE_BASED_VARIABLE_NAME_FOR_HEAP_PROFILING \ Do we need a new macro for this? Seems like the one on line 24 should work, as long as the macro doesn't actually appear twice on the same line.
https://codereview.chromium.org/2695833004/diff/20001/base/profiler/scoped_pr... File base/profiler/scoped_profile.h (right): https://codereview.chromium.org/2695833004/diff/20001/base/profiler/scoped_pr... base/profiler/scoped_profile.h:27: #define LINE_BASED_VARIABLE_NAME_FOR_HEAP_PROFILING \ On 2017/02/15 08:12:44, dcheng wrote: > Do we need a new macro for this? Seems like the one on line 24 should work, as > long as the macro doesn't actually appear twice on the same line. Um, I can't use the same macro twice in TRACK_RUN_IN_THIS_SCOPED_REGION. The code in the macro gets defined in the same line, even if it looks as multiple line in the definition.
https://codereview.chromium.org/2695833004/diff/20001/base/profiler/scoped_pr... File base/profiler/scoped_profile.h (right): https://codereview.chromium.org/2695833004/diff/20001/base/profiler/scoped_pr... base/profiler/scoped_profile.h:27: #define LINE_BASED_VARIABLE_NAME_FOR_HEAP_PROFILING \ On 2017/02/15 18:59:47, ssid wrote: > On 2017/02/15 08:12:44, dcheng wrote: > > Do we need a new macro for this? Seems like the one on line 24 should work, as > > long as the macro doesn't actually appear twice on the same line. > > Um, I can't use the same macro twice in TRACK_RUN_IN_THIS_SCOPED_REGION. The > code in the macro gets defined in the same line, even if it looks as multiple > line in the definition. Can we use __COUNTER__ to get around that problem?
Made changes, ptal Thanks! https://codereview.chromium.org/2695833004/diff/20001/base/profiler/scoped_pr... File base/profiler/scoped_profile.h (right): https://codereview.chromium.org/2695833004/diff/20001/base/profiler/scoped_pr... base/profiler/scoped_profile.h:27: #define LINE_BASED_VARIABLE_NAME_FOR_HEAP_PROFILING \ On 2017/02/15 19:14:25, dcheng wrote: > On 2017/02/15 18:59:47, ssid wrote: > > On 2017/02/15 08:12:44, dcheng wrote: > > > Do we need a new macro for this? Seems like the one on line 24 should work, > as > > > long as the macro doesn't actually appear twice on the same line. > > > > Um, I can't use the same macro twice in TRACK_RUN_IN_THIS_SCOPED_REGION. The > > code in the macro gets defined in the same line, even if it looks as multiple > > line in the definition. > > Can we use __COUNTER__ to get around that problem? I replaced with __COUNTER__ and then realized that the LINE was already not working. It was just substituting name as variable__LINE__. It needs two level indirection like the case of https://cs.chromium.org/chromium/src/base/trace_event/trace_event.h?rcl=064a2... So replaced and used counter.
On 2017/02/15 19:35:53, ssid wrote: > Made changes, ptal Thanks! > > https://codereview.chromium.org/2695833004/diff/20001/base/profiler/scoped_pr... > File base/profiler/scoped_profile.h (right): > > https://codereview.chromium.org/2695833004/diff/20001/base/profiler/scoped_pr... > base/profiler/scoped_profile.h:27: #define > LINE_BASED_VARIABLE_NAME_FOR_HEAP_PROFILING \ > On 2017/02/15 19:14:25, dcheng wrote: > > On 2017/02/15 18:59:47, ssid wrote: > > > On 2017/02/15 08:12:44, dcheng wrote: > > > > Do we need a new macro for this? Seems like the one on line 24 should > work, > > as > > > > long as the macro doesn't actually appear twice on the same line. > > > > > > Um, I can't use the same macro twice in TRACK_RUN_IN_THIS_SCOPED_REGION. The > > > code in the macro gets defined in the same line, even if it looks as > multiple > > > line in the definition. > > > > Can we use __COUNTER__ to get around that problem? > > I replaced with __COUNTER__ and then realized that the LINE was already not > working. It was just substituting name as variable__LINE__. It needs two level > indirection like the case of > https://cs.chromium.org/chromium/src/base/trace_event/trace_event.h?rcl=064a2... > > So replaced and used counter. 1) Do we expect this macro to work if it's instantiated twice in the same scope? I'm curious if __COUNTER__ is reset per-macro invocation or per translation-unit. 2) Please address the typo in the CL description
Description was changed from ========== Use TRACK_RUN_IN_THIS_SCOPED_REGION for heap profiler Currently the heap profiler records the current task context using the posted form location. But for ipc messages all the tasks are posted from ipc.h. TRACK_RUN_IN_THIS_SCOPED_REGION for the same reason to show the posted from info in about:profiler. Use this macro for heap profiling as well. BUG=594803 ========== to ========== Use TRACK_RUN_IN_THIS_SCOPED_REGION for heap profiler Currently the heap profiler records the current task context using the posted from location. But for ipc messages all the tasks are posted from ipc.h. TRACK_RUN_IN_THIS_SCOPED_REGION solves the same problem for showing the posted from info in about:profiler. Use this macro for heap profiling as well. BUG=594803 ==========
Description was changed from ========== Use TRACK_RUN_IN_THIS_SCOPED_REGION for heap profiler Currently the heap profiler records the current task context using the posted from location. But for ipc messages all the tasks are posted from ipc.h. TRACK_RUN_IN_THIS_SCOPED_REGION solves the same problem for showing the posted from info in about:profiler. Use this macro for heap profiling as well. BUG=594803 ========== to ========== Use TRACK_RUN_IN_THIS_SCOPED_REGION for heap profiler Currently the heap profiler records the current task context using the posted from location. But for ipc messages all the tasks are posted from ipc.h. TRACK_RUN_IN_THIS_SCOPED_REGION solves the same problem for showing the posted from info in about:profiler. Use this macro for heap profiling as well. BUG=594803 ==========
On 2017/02/15 21:37:00, dcheng wrote: > On 2017/02/15 19:35:53, ssid wrote: > > Made changes, ptal Thanks! > > > > > https://codereview.chromium.org/2695833004/diff/20001/base/profiler/scoped_pr... > > File base/profiler/scoped_profile.h (right): > > > > > https://codereview.chromium.org/2695833004/diff/20001/base/profiler/scoped_pr... > > base/profiler/scoped_profile.h:27: #define > > LINE_BASED_VARIABLE_NAME_FOR_HEAP_PROFILING \ > > On 2017/02/15 19:14:25, dcheng wrote: > > > On 2017/02/15 18:59:47, ssid wrote: > > > > On 2017/02/15 08:12:44, dcheng wrote: > > > > > Do we need a new macro for this? Seems like the one on line 24 should > > work, > > > as > > > > > long as the macro doesn't actually appear twice on the same line. > > > > > > > > Um, I can't use the same macro twice in TRACK_RUN_IN_THIS_SCOPED_REGION. > The > > > > code in the macro gets defined in the same line, even if it looks as > > multiple > > > > line in the definition. > > > > > > Can we use __COUNTER__ to get around that problem? > > > > I replaced with __COUNTER__ and then realized that the LINE was already not > > working. It was just substituting name as variable__LINE__. It needs two level > > indirection like the case of > > > https://cs.chromium.org/chromium/src/base/trace_event/trace_event.h?rcl=064a2... > > > > So replaced and used counter. > > 1) Do we expect this macro to work if it's instantiated twice in the same scope? > I'm curious if __COUNTER__ is reset per-macro invocation or per > translation-unit. > 2) Please address the typo in the CL description __COUNTER__ is reset for every translation. When I include the first time in code it shows up as: const ::tracked_objects::Location& location = FROM_HERE_WITH_EXPLICIT_FUNCTION("OnFunction"); TRACE_HEAP_PROFILER_API_SCOPED_TASK_EXECUTION some_profiler_variable_0(location.file_name()); ::tracked_objects::ScopedProfile some_profiler_variable_1( location, ::tracked_objects::ScopedProfile::ENABLED); Sorry about the description. Fixed it.
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/v2/patch-status/codereview.chromium.or...
lgtm https://codereview.chromium.org/2695833004/diff/40001/base/profiler/scoped_pr... File base/profiler/scoped_profile.h (right): https://codereview.chromium.org/2695833004/diff/40001/base/profiler/scoped_pr... base/profiler/scoped_profile.h:22: // Two level indirection is required for correct Macro substitution. Nit: macro
Thanks. https://codereview.chromium.org/2695833004/diff/40001/base/profiler/scoped_pr... File base/profiler/scoped_profile.h (right): https://codereview.chromium.org/2695833004/diff/40001/base/profiler/scoped_pr... base/profiler/scoped_profile.h:22: // Two level indirection is required for correct Macro substitution. On 2017/02/15 21:46:58, dcheng wrote: > Nit: macro Done.
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/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: win_chromium_x64_rel_ng on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_...) win_clang on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_clang/builds/...)
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/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: android_n5x_swarming_rel on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_n5x_...)
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/v2/patch-status/codereview.chromium.or...
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 dcheng@chromium.org Link to the patchset: https://codereview.chromium.org/2695833004/#ps60001 (title: "fix comment.")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch.
Bot data: {"patchset_id": 60001, "attempt_start_ts": 1487219711688920,
"parent_rev": "38dc09d11e83b60a667624a917d944ac8f4812bb", "commit_rev":
"27cd72d3ccb96dc69f03b968cb47aae4801a8db1"}
Message was sent while issue was closed.
Description was changed from ========== Use TRACK_RUN_IN_THIS_SCOPED_REGION for heap profiler Currently the heap profiler records the current task context using the posted from location. But for ipc messages all the tasks are posted from ipc.h. TRACK_RUN_IN_THIS_SCOPED_REGION solves the same problem for showing the posted from info in about:profiler. Use this macro for heap profiling as well. BUG=594803 ========== to ========== Use TRACK_RUN_IN_THIS_SCOPED_REGION for heap profiler Currently the heap profiler records the current task context using the posted from location. But for ipc messages all the tasks are posted from ipc.h. TRACK_RUN_IN_THIS_SCOPED_REGION solves the same problem for showing the posted from info in about:profiler. Use this macro for heap profiling as well. BUG=594803 Review-Url: https://codereview.chromium.org/2695833004 Cr-Commit-Position: refs/heads/master@{#450880} Committed: https://chromium.googlesource.com/chromium/src/+/27cd72d3ccb96dc69f03b968cb47... ==========
Message was sent while issue was closed.
Committed patchset #4 (id:60001) as https://chromium.googlesource.com/chromium/src/+/27cd72d3ccb96dc69f03b968cb47... |
