Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2693313005:
Add dump_count:heap_profiler in memory metric. (Closed)
DescriptionAdd dump_count:heap_profiler in memory metric.
Some detailed memory dumps can contain heap profiler information.
This dump count is a subset of detailed memory dumps.
BUG=chromium:670828
Review-Url: https://codereview.chromium.org/2693313005
Committed: https://chromium.googlesource.com/external/github.com/catapult-project/catapult/+/71c4c9aba898fe6d112646d549cb94de1b41a54f
Patch Set 1 #
Total comments: 14
Patch Set 2 : v2 #Patch Set 3 : nit #
Total comments: 1
Patch Set 4 : nit #
Messages
Total messages: 24 (12 generated)
kraynov@chromium.org changed reviewers: + hjd@chromium.org, primiano@chromium.org
PTAL
mostly looks great! just a couple of small nits https://codereview.chromium.org/2693313005/diff/1/tracing/tracing/metrics/sys... File tracing/tracing/metrics/system_health/memory_metric.html (right): https://codereview.chromium.org/2693313005/diff/1/tracing/tracing/metrics/sys... tracing/tracing/metrics/system_health/memory_metric.html:757: * memory:{chrome, webview}:all_processes:dump_count[:{light, detailed}] Please update the comment to say we may be adding heap_profiler as well. https://codereview.chromium.org/2693313005/diff/1/tracing/tracing/metrics/sys... tracing/tracing/metrics/system_health/memory_metric.html:773: levelOfDetailNameToDumpCount['heap_profiler'] = 0; nit: It would be great to put 'heap_profiler' into a constant just since js is so but at alerting us to missing keys caused by typos etc. https://codereview.chromium.org/2693313005/diff/1/tracing/tracing/metrics/sys... tracing/tracing/metrics/system_health/memory_metric.html:784: if (levelOfDetailName === 'detailed') { nit: We should probably compare with the constant: (e.g. globalDump.levelOfDetail === DETAILED) rather than the name. https://codereview.chromium.org/2693313005/diff/1/tracing/tracing/metrics/sys... tracing/tracing/metrics/system_health/memory_metric.html:808: if (detected) I know of a lot of code in tracing still looks like this but the new style guide is to do either: if (myCondition) doMyThing(); or if (myCondition) { doMyThing(); } and to avoid if (myCondition) doMyThing(); https://codereview.chromium.org/2693313005/diff/1/tracing/tracing/metrics/sys... tracing/tracing/metrics/system_health/memory_metric.html:810: if (processDump.heapDumps && processDump.heapDumps.malloc) { hmm, is there a reason it needs to be malloc? If not perhaps it would be better just to check it any of heapDumps keys has a non-zero length so it doesn't look like malloc is special. https://codereview.chromium.org/2693313005/diff/1/tracing/tracing/metrics/sys... tracing/tracing/metrics/system_health/memory_metric.html:813: detected = true; Dito here. https://codereview.chromium.org/2693313005/diff/1/tracing/tracing/metrics/sys... tracing/tracing/metrics/system_health/memory_metric.html:844: ].join(' ').replace('_', ' '); nit: I would put this on levelOfDetailName just so it's clearer what we are trying to accomplish.
https://codereview.chromium.org/2693313005/diff/1/tracing/tracing/metrics/sys... File tracing/tracing/metrics/system_health/memory_metric.html (right): https://codereview.chromium.org/2693313005/diff/1/tracing/tracing/metrics/sys... tracing/tracing/metrics/system_health/memory_metric.html:757: * memory:{chrome, webview}:all_processes:dump_count[:{light, detailed}] On 2017/02/16 10:58:01, hjd wrote: > Please update the comment to say we may be adding > heap_profiler as well. Done. https://codereview.chromium.org/2693313005/diff/1/tracing/tracing/metrics/sys... tracing/tracing/metrics/system_health/memory_metric.html:773: levelOfDetailNameToDumpCount['heap_profiler'] = 0; On 2017/02/16 10:58:01, hjd wrote: > nit: It would be great to put 'heap_profiler' into a constant just since js is > so but at alerting us to missing keys caused by typos etc. It's not clean to do so. Because heap profiler dumps are subclass of detailed dumps. https://codereview.chromium.org/2693313005/diff/1/tracing/tracing/metrics/sys... tracing/tracing/metrics/system_health/memory_metric.html:784: if (levelOfDetailName === 'detailed') { On 2017/02/16 10:58:01, hjd wrote: > nit: We should probably compare with the constant: > (e.g. globalDump.levelOfDetail === DETAILED) > rather than the name. Done. https://codereview.chromium.org/2693313005/diff/1/tracing/tracing/metrics/sys... tracing/tracing/metrics/system_health/memory_metric.html:808: if (detected) On 2017/02/16 10:58:01, hjd wrote: > I know of a lot of code in tracing still looks like this but the new style guide > is to do either: > > if (myCondition) doMyThing(); > > or > > if (myCondition) { > doMyThing(); > } > > and to avoid > > if (myCondition) > doMyThing(); Done. https://codereview.chromium.org/2693313005/diff/1/tracing/tracing/metrics/sys... tracing/tracing/metrics/system_health/memory_metric.html:810: if (processDump.heapDumps && processDump.heapDumps.malloc) { On 2017/02/16 10:58:01, hjd wrote: > hmm, is there a reason it needs to be malloc? If not perhaps it would be better > just to check it any of heapDumps keys has a non-zero length > so it doesn't look like malloc is special. Actually, heap profiler always should create a 'malloc' heap dump and it's expected from their trace format. https://codereview.chromium.org/2693313005/diff/1/tracing/tracing/metrics/sys... tracing/tracing/metrics/system_health/memory_metric.html:813: detected = true; On 2017/02/16 10:58:01, hjd wrote: > Dito here. Done. https://codereview.chromium.org/2693313005/diff/1/tracing/tracing/metrics/sys... tracing/tracing/metrics/system_health/memory_metric.html:844: ].join(' ').replace('_', ' '); On 2017/02/16 10:58:01, hjd wrote: > nit: I would put this on levelOfDetailName just so it's clearer what we are > trying to accomplish. Done.
lgtm!
primiano@chromium.org changed reviewers: + perezju@chromium.org
+perezju can you take a look here, as that will affect the metrics
lgtm! sorry for the slow review.
LGTM although I was expecting something to report the total for the various heap, i.e. something like memory:chrome:browser_process:reported_by_chrome:malloc:heap_profiler:allocations_size memory:chrome:browser_process:reported_by_chrome:malloc:heap_profiler:allocations_count memory:chrome:browser_process:reported_by_chrome:partition_alloc:heap_profiler:allocations_size memory:chrome:browser_process:reported_by_chrome:partition_alloc:heap_profiler:allocations_count ... ditto for blinkgc. I mean, if somebody breaks the heap profiler in a way that generates no dumps, but still creates empty snapshot, the current CL won't help. https://codereview.chromium.org/2693313005/diff/40001/tracing/tracing/metrics... File tracing/tracing/metrics/system_health/memory_metric.html (right): https://codereview.chromium.org/2693313005/diff/40001/tracing/tracing/metrics... tracing/tracing/metrics/system_health/memory_metric.html:843: (levelOfDetailName || 'all').replace('_', ' '); the "|| 'all'" part is not immediately obvious to me, can you expand a bit and maybe improve the comment?
The CQ bit was checked by kraynov@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...
On 2017/02/20 10:45:53, Primiano Tucci wrote: > LGTM although I was expecting something to report the total for the various > heap, i.e. something like > memory:chrome:browser_process:reported_by_chrome:malloc:heap_profiler:allocations_size > memory:chrome:browser_process:reported_by_chrome:malloc:heap_profiler:allocations_count > memory:chrome:browser_process:reported_by_chrome:partition_alloc:heap_profiler:allocations_size > memory:chrome:browser_process:reported_by_chrome:partition_alloc:heap_profiler:allocations_count > ... ditto for blinkgc. Do ping me again (on this or a follow up CL) if you make changes to add those ^^^ (The proposed names look OK, just would like to double check.)
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: Catapult Windows Tryserver on master.tryserver.client.catapult (JOB_FAILED, https://build.chromium.org/p/tryserver.client.catapult/builders/Catapult%20Wi...)
The CQ bit was checked by kraynov@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from primiano@chromium.org, perezju@chromium.org, hjd@chromium.org Link to the patchset: https://codereview.chromium.org/2693313005/#ps60001 (title: "nit")
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
Try jobs failed on following builders: Catapult Windows Tryserver on master.tryserver.client.catapult (JOB_FAILED, https://build.chromium.org/p/tryserver.client.catapult/builders/Catapult%20Wi...)
The CQ bit was checked by kraynov@chromium.org
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": 1487597727956150,
"parent_rev": "df581f5fc8deac24b0027c260b6bace47906161b", "commit_rev":
"71c4c9aba898fe6d112646d549cb94de1b41a54f"}
Message was sent while issue was closed.
Description was changed from ========== Add dump_count:heap_profiler in memory metric. Some detailed memory dumps can contain heap profiler information. This dump count is a subset of detailed memory dumps. BUG=chromium:670828 ========== to ========== Add dump_count:heap_profiler in memory metric. Some detailed memory dumps can contain heap profiler information. This dump count is a subset of detailed memory dumps. BUG=chromium:670828 Review-Url: https://codereview.chromium.org/2693313005 Committed: https://chromium.googlesource.com/external/github.com/catapult-project/catapu... ==========
Message was sent while issue was closed.
Committed patchset #4 (id:60001) as https://chromium.googlesource.com/external/github.com/catapult-project/catapu... |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
