Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2204213002:
[system-health] Report V8 heap space sizes in the memory metric. (Closed)
|
Created:
4 years, 4 months ago by ulan Modified:
4 years, 4 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] Report V8 heap space sizes in the memory metric.
This patch adds v8:heap:[space_name:]{effective_size, allocated_objects_size}
to the memory metric.
BUG=chromium:633160
Committed: https://chromium.googlesource.com/external/github.com/catapult-project/catapult/+/ae72aad918e0b2dfdc123c413e29ceffbd663d40
Patch Set 1 #
Total comments: 12
Patch Set 2 : Address comments #
Total comments: 14
Patch Set 3 : Address comments #Patch Set 4 : fix tests #Patch Set 5 : fix comment #Patch Set 6 : Address more comments #
Total comments: 13
Patch Set 7 : Address comments #
Total comments: 2
Patch Set 8 : Fix line #Patch Set 9 : Add custom handler #Patch Set 10 : whitespace #
Total comments: 12
Patch Set 11 : Address comments #Patch Set 12 : revert to PS 8 #
Messages
Total messages: 21 (5 generated)
Description was changed from
==========
[system-health] Report V8 heap space sizes in the memory metric.
This patch adds v8:heap:[space:]{effective_size, allocated_objects_size}
to the memory metric.
BUG=chromium:633160
==========
to
==========
[system-health] Report V8 heap space sizes in the memory metric.
This patch adds v8:heap:[space_name:]{effective_size, allocated_objects_size}
to the memory metric.
BUG=chromium:633160
==========
ulan@chromium.org changed reviewers: + petrcermak@chromium.org
Hi Petr, wdyt about this patch? If it looks good I will fix the tests.
Looks good overall. Thanks for taking care of this. Petr https://codereview.chromium.org/2204213002/diff/1/tracing/tracing/metrics/sys... File tracing/tracing/metrics/system_health/memory_metric.html (right): https://codereview.chromium.org/2204213002/diff/1/tracing/tracing/metrics/sys... tracing/tracing/metrics/system_health/memory_metric.html:275: if (v8Dump !== undefined) { for code readability purposes, I think it would be better to extract this whole block into a separate function: var v8Dump = processDump.getMemoryAllocatorDumpByFullName('v8'); if (v8Dump !== undefined) addV8MemoryDumpValues(v8Dump, addProcessScalar); https://codereview.chromium.org/2204213002/diff/1/tracing/tracing/metrics/sys... tracing/tracing/metrics/system_health/memory_metric.html:278: addV8MemoryDumpValues(v8Dump, addProcessScalar, Is there any value in hardcoding all the heap spaces? Why not just iterate over the children of the 'heap_spaces' MemoryAllocatorDump? https://codereview.chromium.org/2204213002/diff/1/tracing/tracing/metrics/sys... tracing/tracing/metrics/system_health/memory_metric.html:322: var foundAtLeastOneDump = false; You're essentially duplicating the work that addProcessScalar does. I suggest you just call addProcessScalar multiple times: v8Dump.children.forEach(function(isolateDump) { var componentDump = isolateDump.getDescendantDumpByFullName(componentName); propertyNames.forEach(function(propertyName) { source: ..., component: ..., property: propertyName, value: isolateDump.numerics[propertyName], descriptionPrefixBuilder: CHROME_VALUE_PROPERTIES[propertyName] }); }); https://codereview.chromium.org/2204213002/diff/1/tracing/tracing/metrics/sys... tracing/tracing/metrics/system_health/memory_metric.html:335: componentName.replace('malloc', 'allocated_by_malloc') This seems unnecessarily brute-force. These could be also passed as arguments on lines 276-294. https://codereview.chromium.org/2204213002/diff/1/tracing/tracing/metrics/sys... tracing/tracing/metrics/system_health/memory_metric.html:337: if (foundAtLeastOneDump) { Firstly, you can do this before line 334. Secondly, please invert the if-statement: if (!foundAtLeastOneDump) return;
Thanks a lot! Working on tests. https://codereview.chromium.org/2204213002/diff/1/tracing/tracing/metrics/sys... File tracing/tracing/metrics/system_health/memory_metric.html (right): https://codereview.chromium.org/2204213002/diff/1/tracing/tracing/metrics/sys... tracing/tracing/metrics/system_health/memory_metric.html:275: if (v8Dump !== undefined) { On 2016/08/03 14:24:01, petrcermak wrote: > for code readability purposes, I think it would be better to extract this whole > block into a separate function: > > > var v8Dump = processDump.getMemoryAllocatorDumpByFullName('v8'); > if (v8Dump !== undefined) > addV8MemoryDumpValues(v8Dump, addProcessScalar); Done. https://codereview.chromium.org/2204213002/diff/1/tracing/tracing/metrics/sys... tracing/tracing/metrics/system_health/memory_metric.html:278: addV8MemoryDumpValues(v8Dump, addProcessScalar, On 2016/08/03 14:24:01, petrcermak wrote: > Is there any value in hardcoding all the heap spaces? Why not just iterate over > the children of the 'heap_spaces' MemoryAllocatorDump? The advantage is that we can use function for all v8 components. For the iterating approach I would need a special function for heap spaces. Besides, I would like to filter out 'other_space', which would increase complexity. https://codereview.chromium.org/2204213002/diff/1/tracing/tracing/metrics/sys... tracing/tracing/metrics/system_health/memory_metric.html:322: var foundAtLeastOneDump = false; On 2016/08/03 14:24:01, petrcermak wrote: > You're essentially duplicating the work that addProcessScalar does. I suggest > you just call addProcessScalar multiple times: > > v8Dump.children.forEach(function(isolateDump) { > var componentDump = isolateDump.getDescendantDumpByFullName(componentName); > propertyNames.forEach(function(propertyName) { > source: ..., > component: ..., > property: propertyName, > value: isolateDump.numerics[propertyName], > descriptionPrefixBuilder: CHROME_VALUE_PROPERTIES[propertyName] > }); > }); Done. https://codereview.chromium.org/2204213002/diff/1/tracing/tracing/metrics/sys... tracing/tracing/metrics/system_health/memory_metric.html:335: componentName.replace('malloc', 'allocated_by_malloc') On 2016/08/03 14:24:01, petrcermak wrote: > This seems unnecessarily brute-force. These could be also passed as arguments on > lines 276-294. Done. https://codereview.chromium.org/2204213002/diff/1/tracing/tracing/metrics/sys... tracing/tracing/metrics/system_health/memory_metric.html:337: if (foundAtLeastOneDump) { On 2016/08/03 14:24:01, petrcermak wrote: > Firstly, you can do this before line 334. Secondly, please invert the > if-statement: > > if (!foundAtLeastOneDump) > return; Acknowledged. This code is gone.
https://codereview.chromium.org/2204213002/diff/1/tracing/tracing/metrics/sys... File tracing/tracing/metrics/system_health/memory_metric.html (right): https://codereview.chromium.org/2204213002/diff/1/tracing/tracing/metrics/sys... tracing/tracing/metrics/system_health/memory_metric.html:278: addV8MemoryDumpValues(v8Dump, addProcessScalar, On 2016/08/03 16:02:40, ulan wrote: > On 2016/08/03 14:24:01, petrcermak wrote: > > Is there any value in hardcoding all the heap spaces? Why not just iterate > over > > the children of the 'heap_spaces' MemoryAllocatorDump? > > The advantage is that we can use function for all v8 components. For the > iterating approach I would need a special function for heap spaces. Besides, I > would like to filter out 'other_space', which would increase complexity. I'm not sure I agree about the complexity: v8Dump.children.forEach(function(isolateDump) { var mallocDump = isolateDump.getDescendantDumpByFullName('malloc'); if (mallocDump !== undefined) addV8ComponentValues(mallocDump, addProcessScalar, ['allocated_by_malloc'], ['effective_size', 'peak_size']); var heapSpacesDump = isolateDump.getDescendantDumpByFullName('heap_spaces'); if (heapSpaces !== undefined) { addV8ComponentValues(heapSpacesDump, addProcessScalar, ['heap'], ['effective_size', 'allocated_objects_size']); heapSpacesDump.children.forEach(function(heapSpaceDump) { if (heapSpaceDump.name === 'other_space') return; addV8ComponentValues(heapSpaceDump, addProcessScalar, ...) }) } }); However, if you really don't want to do that, you could at least do something like the following: var REPORTED_HEAP_SPACE_PREFIXES = ['code', 'large', 'map', ...]; REPORTED_HEAP_SPACES.forEach(function(spacePrefix) { var spaceName = spacePrefix + '_space'; addV8ComponentValues(v8Dump, addProcessScalar, 'heap_spaces/' + spaceName, ['v8', 'heap', spaceName], ['effective_size, 'allocated_objects_size']); }); If you find that too complex, then it's fine as well ;-) Also, could you please add a comment that all heap spaces except for 'other' are reported? https://codereview.chromium.org/2204213002/diff/20001/tracing/tracing/metrics... File tracing/tracing/metrics/system_health/memory_metric.html (right): https://codereview.chromium.org/2204213002/diff/20001/tracing/tracing/metrics... tracing/tracing/metrics/system_health/memory_metric.html:274: // Add memory:<browser-name>:<process-name>:reported_by_chrome:v8:... This is a little misleading because it suggests that v8:effective_size and v8:allocated_objects_size is also reported through here, which is not the case. Can you add something like "(except for v8:effective_size and v8:allocated_objects_size)"? https://codereview.chromium.org/2204213002/diff/20001/tracing/tracing/metrics... tracing/tracing/metrics/system_health/memory_metric.html:291: * Add memory dump values of V8 components. nit: s/of/calculated from/ (also applies to addV8ComponentValues) https://codereview.chromium.org/2204213002/diff/20001/tracing/tracing/metrics... tracing/tracing/metrics/system_health/memory_metric.html:291: * Add memory dump values of V8 components. nit: please add a blank line above the parameters (also applies to the other function) https://codereview.chromium.org/2204213002/diff/20001/tracing/tracing/metrics... tracing/tracing/metrics/system_health/memory_metric.html:297: if (v8Dump !== undefined) { Please invert: if (v8Dump === undefined) return; https://codereview.chromium.org/2204213002/diff/20001/tracing/tracing/metrics... tracing/tracing/metrics/system_health/memory_metric.html:332: componentPath, propertyNames) { To simplify your code, how about you always try to report all three properties (effective, allocated_objects, peak)? If they're not present, they'll just be ignored. https://codereview.chromium.org/2204213002/diff/20001/tracing/tracing/metrics... tracing/tracing/metrics/system_health/memory_metric.html:336: if (componentDump !== undefined) { Please invert: if (componentDump === undefined) return; https://codereview.chromium.org/2204213002/diff/20001/tracing/tracing/metrics... tracing/tracing/metrics/system_health/memory_metric.html:338: if (componentDump.numerics[propertyName] !== undefined) { No need for this check. Just pass componentDump.numerics[propertyName] as the `value` to addProcessStalar. It will automatically handle "undefinedness" and pick the unit (if the numeric is defined).
PTAL https://codereview.chromium.org/2204213002/diff/1/tracing/tracing/metrics/sys... File tracing/tracing/metrics/system_health/memory_metric.html (right): https://codereview.chromium.org/2204213002/diff/1/tracing/tracing/metrics/sys... tracing/tracing/metrics/system_health/memory_metric.html:278: addV8MemoryDumpValues(v8Dump, addProcessScalar, On 2016/08/03 16:43:18, petrcermak wrote: > On 2016/08/03 16:02:40, ulan wrote: > > On 2016/08/03 14:24:01, petrcermak wrote: > > > Is there any value in hardcoding all the heap spaces? Why not just iterate > > over > > > the children of the 'heap_spaces' MemoryAllocatorDump? > > > > The advantage is that we can use function for all v8 components. For the > > iterating approach I would need a special function for heap spaces. Besides, I > > would like to filter out 'other_space', which would increase complexity. > > I'm not sure I agree about the complexity: > > v8Dump.children.forEach(function(isolateDump) { > var mallocDump = isolateDump.getDescendantDumpByFullName('malloc'); > if (mallocDump !== undefined) > addV8ComponentValues(mallocDump, addProcessScalar, ['allocated_by_malloc'], > ['effective_size', 'peak_size']); > > var heapSpacesDump = isolateDump.getDescendantDumpByFullName('heap_spaces'); > if (heapSpaces !== undefined) { > addV8ComponentValues(heapSpacesDump, addProcessScalar, ['heap'], > ['effective_size', 'allocated_objects_size']); > heapSpacesDump.children.forEach(function(heapSpaceDump) { > if (heapSpaceDump.name === 'other_space') > return; > addV8ComponentValues(heapSpaceDump, addProcessScalar, ...) > }) > } > }); > Thanks a lot. I like it, simpler than I expected :) I added a comment about other spaces. https://codereview.chromium.org/2204213002/diff/20001/tracing/tracing/metrics... File tracing/tracing/metrics/system_health/memory_metric.html (right): https://codereview.chromium.org/2204213002/diff/20001/tracing/tracing/metrics... tracing/tracing/metrics/system_health/memory_metric.html:274: // Add memory:<browser-name>:<process-name>:reported_by_chrome:v8:... On 2016/08/03 16:43:19, petrcermak wrote: > This is a little misleading because it suggests that v8:effective_size and > v8:allocated_objects_size is also reported through here, which is not the case. > Can you add something like "(except for v8:effective_size and > v8:allocated_objects_size)"? Done. https://codereview.chromium.org/2204213002/diff/20001/tracing/tracing/metrics... tracing/tracing/metrics/system_health/memory_metric.html:291: * Add memory dump values of V8 components. On 2016/08/03 16:43:19, petrcermak wrote: > nit: please add a blank line above the parameters (also applies to the other > function) Done. https://codereview.chromium.org/2204213002/diff/20001/tracing/tracing/metrics... tracing/tracing/metrics/system_health/memory_metric.html:291: * Add memory dump values of V8 components. On 2016/08/03 16:43:19, petrcermak wrote: > nit: s/of/calculated from/ (also applies to addV8ComponentValues) Done. https://codereview.chromium.org/2204213002/diff/20001/tracing/tracing/metrics... tracing/tracing/metrics/system_health/memory_metric.html:297: if (v8Dump !== undefined) { On 2016/08/03 16:43:19, petrcermak wrote: > Please invert: > > if (v8Dump === undefined) > return; Done. https://codereview.chromium.org/2204213002/diff/20001/tracing/tracing/metrics... tracing/tracing/metrics/system_health/memory_metric.html:332: componentPath, propertyNames) { On 2016/08/03 16:43:18, petrcermak wrote: > To simplify your code, how about you always try to report all three properties > (effective, allocated_objects, peak)? If they're not present, they'll just be > ignored. Done. https://codereview.chromium.org/2204213002/diff/20001/tracing/tracing/metrics... tracing/tracing/metrics/system_health/memory_metric.html:336: if (componentDump !== undefined) { On 2016/08/03 16:43:19, petrcermak wrote: > Please invert: > > if (componentDump === undefined) > return; Done. https://codereview.chromium.org/2204213002/diff/20001/tracing/tracing/metrics... tracing/tracing/metrics/system_health/memory_metric.html:338: if (componentDump.numerics[propertyName] !== undefined) { On 2016/08/03 16:43:19, petrcermak wrote: > No need for this check. Just pass componentDump.numerics[propertyName] as the > `value` to addProcessStalar. It will automatically handle "undefinedness" and > pick the unit (if the numeric is defined). Done.
I think this is almost done :-) https://codereview.chromium.org/2204213002/diff/100001/tracing/tracing/metric... File tracing/tracing/metrics/system_health/memory_metric.html (right): https://codereview.chromium.org/2204213002/diff/100001/tracing/tracing/metric... tracing/tracing/metrics/system_health/memory_metric.html:292: * the 'other_spaces'. nit: drop "the" and change 'other_spaces' to 'heap_spaces/other_spaces' https://codereview.chromium.org/2204213002/diff/100001/tracing/tracing/metric... tracing/tracing/metrics/system_health/memory_metric.html:302: var mallocDump = isolateDump.getDescendantDumpByFullName('malloc'); Could you add a comment above this line: // v8:allocated_by_malloc:... https://codereview.chromium.org/2204213002/diff/100001/tracing/tracing/metric... tracing/tracing/metrics/system_health/memory_metric.html:305: var heapDump = isolateDump.getDescendantDumpByFullName('heap_spaces'); Similarly: // v8:heap:... https://codereview.chromium.org/2204213002/diff/100001/tracing/tracing/metric... tracing/tracing/metrics/system_health/memory_metric.html:325: function addV8ComponentValues(componentDump, addProcessScalar, nit: it feels like componentDump and componentPath should be next to each other since they're coupled. Could you please swap addProcessScalar and componentPath to achieve this? https://codereview.chromium.org/2204213002/diff/100001/tracing/tracing/metric... tracing/tracing/metrics/system_health/memory_metric.html:327: if (componentDump === undefined) With the exception of the malloc dump, you're doing an unnecessary check here. I suggest you just add the if-check after line 302 and remove it from here. Otherwise, you should change the parameter's type to "(!tr.model.MemoryAllocatorDump|undefined)" https://codereview.chromium.org/2204213002/diff/100001/tracing/tracing/metric... File tracing/tracing/metrics/system_health/memory_metric_test.html (right): https://codereview.chromium.org/2204213002/diff/100001/tracing/tracing/metric... tracing/tracing/metrics/system_health/memory_metric_test.html:601: description: 'size of all objects allocated by v8:heap in all ' + should we have a custom handler for 'heap' as well (given that we have one for malloc)? Ideas: size of all objects allocated on heap in v8 ... size of all objects allocated on heap:large_object_space in v8 ...
Thanks! https://codereview.chromium.org/2204213002/diff/100001/tracing/tracing/metric... File tracing/tracing/metrics/system_health/memory_metric.html (right): https://codereview.chromium.org/2204213002/diff/100001/tracing/tracing/metric... tracing/tracing/metrics/system_health/memory_metric.html:292: * the 'other_spaces'. On 2016/08/04 09:11:28, petrcermak wrote: > nit: drop "the" and change 'other_spaces' to 'heap_spaces/other_spaces' Done. https://codereview.chromium.org/2204213002/diff/100001/tracing/tracing/metric... tracing/tracing/metrics/system_health/memory_metric.html:302: var mallocDump = isolateDump.getDescendantDumpByFullName('malloc'); On 2016/08/04 09:11:27, petrcermak wrote: > Could you add a comment above this line: > > // v8:allocated_by_malloc:... Done. https://codereview.chromium.org/2204213002/diff/100001/tracing/tracing/metric... tracing/tracing/metrics/system_health/memory_metric.html:305: var heapDump = isolateDump.getDescendantDumpByFullName('heap_spaces'); On 2016/08/04 09:11:27, petrcermak wrote: > Similarly: > > // v8:heap:... Done. https://codereview.chromium.org/2204213002/diff/100001/tracing/tracing/metric... tracing/tracing/metrics/system_health/memory_metric.html:325: function addV8ComponentValues(componentDump, addProcessScalar, On 2016/08/04 09:11:28, petrcermak wrote: > nit: it feels like componentDump and componentPath should be next to each other > since they're coupled. Could you please swap addProcessScalar and componentPath > to achieve this? Done. https://codereview.chromium.org/2204213002/diff/100001/tracing/tracing/metric... tracing/tracing/metrics/system_health/memory_metric.html:327: if (componentDump === undefined) On 2016/08/04 09:11:28, petrcermak wrote: > With the exception of the malloc dump, you're doing an unnecessary check here. I > suggest you just add the if-check after line 302 and remove it from here. > > Otherwise, you should change the parameter's type to > "(!tr.model.MemoryAllocatorDump|undefined)" Done.
Wow, that's a spectacular turnaround time! I think you skipped my comment in the test file. Apart from that, once we agree on https://bugs.chromium.org/p/chromium/issues/detail?id=633160#c6, this is ready to land :-) Thanks, Petr https://codereview.chromium.org/2204213002/diff/120001/tracing/tracing/metric... File tracing/tracing/metrics/system_health/memory_metric.html (right): https://codereview.chromium.org/2204213002/diff/120001/tracing/tracing/metric... tracing/tracing/metrics/system_health/memory_metric.html:312: addProcessScalar); I think this might fit on the previous line?
> I think you skipped my comment in the test file. Oops, missed it. https://codereview.chromium.org/2204213002/diff/100001/tracing/tracing/metric... File tracing/tracing/metrics/system_health/memory_metric_test.html (right): https://codereview.chromium.org/2204213002/diff/100001/tracing/tracing/metric... tracing/tracing/metrics/system_health/memory_metric_test.html:601: description: 'size of all objects allocated by v8:heap in all ' + On 2016/08/04 09:11:28, petrcermak wrote: > should we have a custom handler for 'heap' as well (given that we have one for > malloc)? Ideas: > > size of all objects allocated on heap in v8 ... > size of all objects allocated on heap:large_object_space in v8 ... Yes, we can have custom handler. Implementing it. https://codereview.chromium.org/2204213002/diff/120001/tracing/tracing/metric... File tracing/tracing/metrics/system_health/memory_metric.html (right): https://codereview.chromium.org/2204213002/diff/120001/tracing/tracing/metric... tracing/tracing/metrics/system_health/memory_metric.html:312: addProcessScalar); On 2016/08/04 09:26:47, petrcermak wrote: > I think this might fit on the previous line? Done.
PTAL https://codereview.chromium.org/2204213002/diff/100001/tracing/tracing/metric... File tracing/tracing/metrics/system_health/memory_metric_test.html (right): https://codereview.chromium.org/2204213002/diff/100001/tracing/tracing/metric... tracing/tracing/metrics/system_health/memory_metric_test.html:601: description: 'size of all objects allocated by v8:heap in all ' + On 2016/08/04 09:38:26, ulan wrote: > On 2016/08/04 09:11:28, petrcermak wrote: > > should we have a custom handler for 'heap' as well (given that we have one for > > malloc)? Ideas: > > > > size of all objects allocated on heap in v8 ... > > size of all objects allocated on heap:large_object_space in v8 ... > > Yes, we can have custom handler. Implementing it. Done.
https://codereview.chromium.org/2204213002/diff/180001/tracing/tracing/metric... File tracing/tracing/metrics/system_health/memory_metric.html (right): https://codereview.chromium.org/2204213002/diff/180001/tracing/tracing/metric... tracing/tracing/metrics/system_health/memory_metric.html:380: // Returns true iff we need custom component description after the I think that this is way too complex. Why not just do the following on line 443: var length = componentPath.length; // You can actually declare this earlier and use on line 419 already. var hasCustomComponentDescription = false; if (componentPath[0] === 'v8') { if (componentPath[length - 1] === 'allocated_by_malloc') { hasCustomComponentDescription = true; nameParts.push(...); ...; } else if (componentPath[length - 1] === 'heap') { hasCustomComponentDescription = true; ... } else if ... ... } Or you could keep a addCustomComponentDescriptionIfApplicable function that returns a Boolean (whether a custom description was added or not). https://codereview.chromium.org/2204213002/diff/180001/tracing/tracing/metric... tracing/tracing/metrics/system_health/memory_metric.html:390: return (length >= 2 && componentPath[length - 1] == 'heap' && === (4 times) https://codereview.chromium.org/2204213002/diff/180001/tracing/tracing/metric... tracing/tracing/metrics/system_health/memory_metric.html:390: return (length >= 2 && componentPath[length - 1] == 'heap' && note that since this is JS, you don't actually need the length checks. Because A[i] is |undefined| for i < 0 and i >= A.length. https://codereview.chromium.org/2204213002/diff/180001/tracing/tracing/metric... tracing/tracing/metrics/system_health/memory_metric.html:409: nameParts.push('on'); on a second thought, I think you should reuse componentPreposition so that this can be used with effective_size as well. https://codereview.chromium.org/2204213002/diff/180001/tracing/tracing/metric... File tracing/tracing/metrics/system_health/memory_metric_test.html (right): https://codereview.chromium.org/2204213002/diff/180001/tracing/tracing/metric... tracing/tracing/metrics/system_health/memory_metric_test.html:873: description: 'effective size of v8:heap:map_space in renderer ' + Isn't this supposed to change as well?
https://codereview.chromium.org/2204213002/diff/180001/tracing/tracing/metric... File tracing/tracing/metrics/system_health/memory_metric.html (right): https://codereview.chromium.org/2204213002/diff/180001/tracing/tracing/metric... tracing/tracing/metrics/system_health/memory_metric.html:380: // Returns true iff we need custom component description after the On 2016/08/04 11:21:43, petrcermak wrote: > I think that this is way too complex. Why not just do the following on line 443: > > var length = componentPath.length; // You can actually declare this earlier and > use on line 419 already. > > var hasCustomComponentDescription = false; > if (componentPath[0] === 'v8') { > if (componentPath[length - 1] === 'allocated_by_malloc') { > hasCustomComponentDescription = true; > nameParts.push(...); > ...; > } else if (componentPath[length - 1] === 'heap') { > hasCustomComponentDescription = true; > ... > } else if ... > ... > } > > Or you could keep a addCustomComponentDescriptionIfApplicable function that > returns a Boolean (whether a custom description was added or not). Thanks, I went with the first suggestion. Note that (componentPath[0] === 'v8') instead of componentPath[heapPosition - 1] === 'v8' makes the code more fragile. But I guess it is OK since it is unlikely that v8 will move inside another component. https://codereview.chromium.org/2204213002/diff/180001/tracing/tracing/metric... tracing/tracing/metrics/system_health/memory_metric.html:390: return (length >= 2 && componentPath[length - 1] == 'heap' && On 2016/08/04 11:21:43, petrcermak wrote: > === (4 times) Acknowledged. https://codereview.chromium.org/2204213002/diff/180001/tracing/tracing/metric... tracing/tracing/metrics/system_health/memory_metric.html:390: return (length >= 2 && componentPath[length - 1] == 'heap' && On 2016/08/04 11:21:43, petrcermak wrote: > note that since this is JS, you don't actually need the length checks. Because > A[i] is |undefined| for i < 0 and i >= A.length. Acknowledged. https://codereview.chromium.org/2204213002/diff/180001/tracing/tracing/metric... tracing/tracing/metrics/system_health/memory_metric.html:409: nameParts.push('on'); On 2016/08/04 11:21:43, petrcermak wrote: > on a second thought, I think you should reuse componentPreposition so that this > can be used with effective_size as well. Oh, I thought the main point was to get the preposition correct since the objects are allocated not by heap, but on heap. Now I am not sure if the change from "v8:heap:space" to "heap:space in v8" is worth the complexity. https://codereview.chromium.org/2204213002/diff/180001/tracing/tracing/metric... File tracing/tracing/metrics/system_health/memory_metric_test.html (right): https://codereview.chromium.org/2204213002/diff/180001/tracing/tracing/metric... tracing/tracing/metrics/system_health/memory_metric_test.html:873: description: 'effective size of v8:heap:map_space in renderer ' + On 2016/08/04 11:21:44, petrcermak wrote: > Isn't this supposed to change as well? Done, but I thought this one was OK from grammar point of view :)
LGTM with one more comment (let's not special case the heap descriptions). Thanks, Petr https://codereview.chromium.org/2204213002/diff/180001/tracing/tracing/metric... File tracing/tracing/metrics/system_health/memory_metric.html (right): https://codereview.chromium.org/2204213002/diff/180001/tracing/tracing/metric... tracing/tracing/metrics/system_health/memory_metric.html:409: nameParts.push('on'); I think you're right. It's probably not worth the complexity at all. Maybe we should just keep the special case for 'allocated_by_malloc' only. It's probably a more future-proof approach. Sorry for making you do extra work.
Reverted custom handler changes, landing. https://codereview.chromium.org/2204213002/diff/180001/tracing/tracing/metric... File tracing/tracing/metrics/system_health/memory_metric.html (right): https://codereview.chromium.org/2204213002/diff/180001/tracing/tracing/metric... tracing/tracing/metrics/system_health/memory_metric.html:409: nameParts.push('on'); On 2016/08/04 12:45:12, petrcermak wrote: > I think you're right. It's probably not worth the complexity at all. Maybe we > should just keep the special case for 'allocated_by_malloc' only. It's probably > a more future-proof approach. Sorry for making you do extra work. No problem :) I agree that not having special handler is better tradeoff.
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/2204213002/#ps220001 (title: "revert to PS 8")
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] Report V8 heap space sizes in the memory metric.
This patch adds v8:heap:[space_name:]{effective_size, allocated_objects_size}
to the memory metric.
BUG=chromium:633160
==========
to
==========
[system-health] Report V8 heap space sizes in the memory metric.
This patch adds v8:heap:[space_name:]{effective_size, allocated_objects_size}
to the memory metric.
BUG=chromium:633160
Committed:
https://chromium.googlesource.com/external/github.com/catapult-project/catapu...
==========
Message was sent while issue was closed.
Committed patchset #12 (id:220001) as https://chromium.googlesource.com/external/github.com/catapult-project/catapu... |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
