Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2153823002:
[system-health] Track amount of malloced memory in V8. (Closed)
|
Created:
4 years, 5 months ago by ulan Modified:
4 years, 5 months ago Reviewers:
petrcermak CC:
catapult-reviews_chromium.org, tracing-review_chromium.org Base URL:
https://chromium.googlesource.com/external/github.com/catapult-project/catapult.git@master Target Ref:
refs/heads/master Project:
catapult Visibility:
Public. |
Description[system-health] Track amount of malloced memory in V8.
This patch extends the memory metric with v8:allocated_by_malloc counter.
BUG=chromium:628239
Committed: https://chromium.googlesource.com/external/github.com/catapult-project/catapult/+/3c2ec02098fa8c1f425648a0b13110565a7c8fc5
Patch Set 1 #
Total comments: 13
Patch Set 2 : Address comments #
Total comments: 2
Patch Set 3 : 11 => 1 + 10 #Patch Set 4 : rebase #
Messages
Total messages: 19 (10 generated)
ulan@chromium.org changed reviewers: + petrcermak@chromium.org
ptal
LGTM % a few comments. Please also: * Prefix the patch title and first description line with "[system-health] ". * s/memory metric/the memory metric/ in the patch description. Thanks, Petr https://codereview.chromium.org/2153823002/diff/1/tracing/tracing/metrics/sys... File tracing/tracing/metrics/system_health/memory_metric.html (right): https://codereview.chromium.org/2153823002/diff/1/tracing/tracing/metrics/sys... tracing/tracing/metrics/system_health/memory_metric.html:273: var v8Dump = processDump.getMemoryAllocatorDumpByFullName('v8'); Please add a blank line and a comment above this line similar to lines 257-258: // Add memory:<browser-name>:<process-name>:reported_by_chrome:v8: // allocated_by_malloc:effective_size when available. https://codereview.chromium.org/2153823002/diff/1/tracing/tracing/metrics/sys... tracing/tracing/metrics/system_health/memory_metric.html:287: nit: I suggest you remove this blank line so that the V8-specific code is clearly grouped together. https://codereview.chromium.org/2153823002/diff/1/tracing/tracing/metrics/sys... tracing/tracing/metrics/system_health/memory_metric.html:376: if ('allocated_by_malloc' === componentPath[componentPath.length - 1]) { Please flip the condition of the if statement and reuse the code on line 381: if (componentPath[componentPath.length - 1] === 'allocated_by_malloc') { nameParts.push('objects allocated by malloc in'); componentPath.pop(); } nameParts.push(componentPath.join(':')); I also wonder whether it should be "... allocated by malloc FOR v8 in ...". I'm not sure though, so I leave it up to you. https://codereview.chromium.org/2153823002/diff/1/tracing/tracing/metrics/sys... File tracing/tracing/metrics/system_health/memory_metric_test.html (right): https://codereview.chromium.org/2153823002/diff/1/tracing/tracing/metrics/sys... tracing/tracing/metrics/system_health/memory_metric_test.html:366: var isolateDump = addChildDump(v8Dump, 'isolate', {}); no need for the empty options dict ;-) https://codereview.chromium.org/2153823002/diff/1/tracing/tracing/metrics/sys... tracing/tracing/metrics/system_health/memory_metric_test.html:367: addChildDump(isolateDump, 'malloc', { 'size': 1 }); Could you please modify one of the two tests you changed to have more than one isolate? https://codereview.chromium.org/2153823002/diff/1/tracing/tracing/metrics/sys... tracing/tracing/metrics/system_health/memory_metric_test.html:1455: var isolateDump = addChildDump(v8Dump, 'isolate', {}); ditto (no need for empty options dict)
Description was changed from ========== Track amount of malloced memory in V8. This patch extends memory metric with v8:allocated_by_malloc counter. BUG=chromium:628239 ========== to ========== Track amount of malloced memory in V8. This patch extends the memory metric with v8:allocated_by_malloc counter. BUG=chromium:628239 ==========
Thank you! https://codereview.chromium.org/2153823002/diff/1/tracing/tracing/metrics/sys... File tracing/tracing/metrics/system_health/memory_metric.html (right): https://codereview.chromium.org/2153823002/diff/1/tracing/tracing/metrics/sys... tracing/tracing/metrics/system_health/memory_metric.html:273: var v8Dump = processDump.getMemoryAllocatorDumpByFullName('v8'); On 2016/07/18 11:41:13, petrcermak wrote: > Please add a blank line and a comment above this line similar to lines 257-258: > > // Add memory:<browser-name>:<process-name>:reported_by_chrome:v8: > // allocated_by_malloc:effective_size when available. Done. https://codereview.chromium.org/2153823002/diff/1/tracing/tracing/metrics/sys... tracing/tracing/metrics/system_health/memory_metric.html:287: On 2016/07/18 11:41:13, petrcermak wrote: > nit: I suggest you remove this blank line so that the V8-specific code is > clearly grouped together. Done. https://codereview.chromium.org/2153823002/diff/1/tracing/tracing/metrics/sys... tracing/tracing/metrics/system_health/memory_metric.html:376: if ('allocated_by_malloc' === componentPath[componentPath.length - 1]) { On 2016/07/18 11:41:13, petrcermak wrote: > Please flip the condition of the if statement and reuse the code on line 381: > > > if (componentPath[componentPath.length - 1] === 'allocated_by_malloc') { > nameParts.push('objects allocated by malloc in'); > componentPath.pop(); > } > nameParts.push(componentPath.join(':')); > Flipped the condition, but I would prefer to keep componentPath immutable. Otherwise, we might be setting up a bug for someone who extends this function in future. > I also wonder whether it should be "... allocated by malloc FOR v8 in ...". I'm > not sure though, so I leave it up to you. Done. https://codereview.chromium.org/2153823002/diff/1/tracing/tracing/metrics/sys... File tracing/tracing/metrics/system_health/memory_metric_test.html (right): https://codereview.chromium.org/2153823002/diff/1/tracing/tracing/metrics/sys... tracing/tracing/metrics/system_health/memory_metric_test.html:366: var isolateDump = addChildDump(v8Dump, 'isolate', {}); On 2016/07/18 11:41:13, petrcermak wrote: > no need for the empty options dict ;-) Done. https://codereview.chromium.org/2153823002/diff/1/tracing/tracing/metrics/sys... tracing/tracing/metrics/system_health/memory_metric_test.html:367: addChildDump(isolateDump, 'malloc', { 'size': 1 }); On 2016/07/18 11:41:13, petrcermak wrote: > Could you please modify one of the two tests you changed to have more than one > isolate? Done. https://codereview.chromium.org/2153823002/diff/1/tracing/tracing/metrics/sys... tracing/tracing/metrics/system_health/memory_metric_test.html:1455: var isolateDump = addChildDump(v8Dump, 'isolate', {}); On 2016/07/18 11:41:13, petrcermak wrote: > ditto (no need for empty options dict) Done.
LGTM % one final comment. Thanks, Petr https://codereview.chromium.org/2153823002/diff/1/tracing/tracing/metrics/sys... File tracing/tracing/metrics/system_health/memory_metric.html (right): https://codereview.chromium.org/2153823002/diff/1/tracing/tracing/metrics/sys... tracing/tracing/metrics/system_health/memory_metric.html:376: if ('allocated_by_malloc' === componentPath[componentPath.length - 1]) { On 2016/07/18 12:13:16, ulan wrote: > On 2016/07/18 11:41:13, petrcermak wrote: > > Please flip the condition of the if statement and reuse the code on line 381: > > > > > > if (componentPath[componentPath.length - 1] === 'allocated_by_malloc') { > > nameParts.push('objects allocated by malloc in'); > > componentPath.pop(); > > } > > nameParts.push(componentPath.join(':')); > > > > Flipped the condition, but I would prefer to keep componentPath immutable. > Otherwise, we might be setting up a bug for someone who extends this function in > future. Ack. Makes sense. > > > I also wonder whether it should be "... allocated by malloc FOR v8 in ...". > I'm > > not sure though, so I leave it up to you. > Done. https://codereview.chromium.org/2153823002/diff/20001/tracing/tracing/metrics... File tracing/tracing/metrics/system_health/memory_metric_test.html (right): https://codereview.chromium.org/2153823002/diff/20001/tracing/tracing/metrics... tracing/tracing/metrics/system_health/memory_metric_test.html:496: value: [0, 11, 0, 0], Please change this to "10 + 1" (so that it's easier to search through the file). Also please do this for the other expectations you added.
https://codereview.chromium.org/2153823002/diff/20001/tracing/tracing/metrics... File tracing/tracing/metrics/system_health/memory_metric_test.html (right): https://codereview.chromium.org/2153823002/diff/20001/tracing/tracing/metrics... tracing/tracing/metrics/system_health/memory_metric_test.html:496: value: [0, 11, 0, 0], On 2016/07/18 12:19:02, petrcermak wrote: > Please change this to "10 + 1" (so that it's easier to search through the file). > Also please do this for the other expectations you added. Done.
The CQ bit was checked by ulan@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from petrcermak@chromium.org Link to the patchset: https://codereview.chromium.org/2153823002/#ps40001 (title: "11 => 1 + 10")
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 Android Tryserver on master.tryserver.client.catapult (JOB_FAILED, https://build.chromium.org/p/tryserver.client.catapult/builders/Catapult%20An...) Catapult Presubmit on master.tryserver.client.catapult (JOB_FAILED, https://build.chromium.org/p/tryserver.client.catapult/builders/Catapult%20Pr...)
The CQ bit was unchecked by ulan@chromium.org
Description was changed from ========== Track amount of malloced memory in V8. This patch extends the memory metric with v8:allocated_by_malloc counter. BUG=chromium:628239 ========== to ========== [system-health] Track amount of malloced memory in V8. This patch extends the memory metric with v8:allocated_by_malloc counter. BUG=chromium:628239 ==========
The CQ bit was checked by ulan@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from petrcermak@chromium.org Link to the patchset: https://codereview.chromium.org/2153823002/#ps60001 (title: "rebase")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Message was sent while issue was closed.
Description was changed from ========== [system-health] Track amount of malloced memory in V8. This patch extends the memory metric with v8:allocated_by_malloc counter. BUG=chromium:628239 ========== to ========== [system-health] Track amount of malloced memory in V8. This patch extends the memory metric with v8:allocated_by_malloc counter. BUG=chromium:628239 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... |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
