Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2043203003:
Add MemoryInfra overheads to tracingMetrics (Closed)
|
Created:
4 years, 6 months ago by ssid Modified:
4 years, 6 months ago CC:
catapult-reviews_chromium.org, tracing-review_chromium.org, telemetry-reviews_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. |
DescriptionAdd MemoryInfra overheads to tracingMetrics
This CL adds metrics to track the overhead of MemoryInfra dumps to
tracingMetrics. This is useful to track the memory dump timings and
trace size.
Tracks the duration of memory-infra trace events, duration of each
dump provider and total json size of memory-infra trace events.
BUG=catapult:#2400
Committed: https://chromium.googlesource.com/external/github.com/catapult-project/catapult/+/b0c2eccc8ff2adef60dc53764d38b97b2a3d1a52
Patch Set 1 : #Patch Set 2 : #
Total comments: 66
Patch Set 3 : address Petr's comments. #Patch Set 4 : nit. #
Total comments: 6
Patch Set 5 : Renames. #Patch Set 6 : nit. #
Total comments: 4
Patch Set 7 : Renames to memory-infra. #
Messages
Total messages: 24 (8 generated)
Patchset #1 (id:1) has been deleted
Description was changed from ========== add memory tracing metric BUG=catapult:# ========== to ========== Add MemoryInfra overheads to tracingMetrics This CL adds metrics to track the overhead of MemoryInfra dumps to tracingMetrics. This is useful to track the memory dump timings and trace size. Tracks the duration of memory-infra trace events, duration of each dump provider and total json size of memory-infra trace events. BUG=catapult:#2400 ==========
ssid@chromium.org changed reviewers: + petrcermak@chromium.org, zhenw@chromium.org
ptal, thanks
Looks good overall. I've left a bunch of inline comments (sorry for being so pedantic). Apart from the names of the reported values, they're mostly just code style nits. Thanks, Petr https://codereview.chromium.org/2043203003/diff/40001/tracing/tracing/metrics... File tracing/tracing/metrics/tracing_metric.html (right): https://codereview.chromium.org/2043203003/diff/40001/tracing/tracing/metrics... tracing/tracing/metrics/tracing_metric.html:17: function _addTimeDurationMetric(metricName, duration, allValues) { no need for preceding underscore. Since you don't export this function, it won't be visible outside anyway ;-) Also, it should be addTimeDuration*Value* and *value*Name. https://codereview.chromium.org/2043203003/diff/40001/tracing/tracing/metrics... tracing/tracing/metrics/tracing_metric.html:123: // The metrics below are specific to memory infra dumps. supernit: s/memory infra/memory-infra/ https://codereview.chromium.org/2043203003/diff/40001/tracing/tracing/metrics... tracing/tracing/metrics/tracing_metric.html:123: // The metrics below are specific to memory infra dumps. please factor your code into a separate function if possible (e.g. addMemoryInfraValues or something like that) https://codereview.chromium.org/2043203003/diff/40001/tracing/tracing/metrics... tracing/tracing/metrics/tracing_metric.html:123: // The metrics below are specific to memory infra dumps. s/metrics/values/ https://codereview.chromium.org/2043203003/diff/40001/tracing/tracing/metrics... tracing/tracing/metrics/tracing_metric.html:124: var memoryInfraCategory = 'disabled-by-default-memory-infra'; This should be a top-level constant defined outside this function. Please call it MEMORY_INFRA_TRACING_CATEGORY. https://codereview.chromium.org/2043203003/diff/40001/tracing/tracing/metrics... tracing/tracing/metrics/tracing_metric.html:125: var memoryDumpsCount = model.globalMemoryDumps.length; nit: should be "memoryDumpCount" (not plural) to be consistent with everywhere else. https://codereview.chromium.org/2043203003/diff/40001/tracing/tracing/metrics... tracing/tracing/metrics/tracing_metric.html:127: memoryDumpsCount == 0) { Always use === instead of ==. https://codereview.chromium.org/2043203003/diff/40001/tracing/tracing/metrics... tracing/tracing/metrics/tracing_metric.html:132: var mainThreadsOverhead = 0; Shouldn't this be nonMemoryInfraThreadOverhead? https://codereview.chromium.org/2043203003/diff/40001/tracing/tracing/metrics... tracing/tracing/metrics/tracing_metric.html:133: var overheadByProviders = {}; nit: overheadByProvider (not plural) https://codereview.chromium.org/2043203003/diff/40001/tracing/tracing/metrics... tracing/tracing/metrics/tracing_metric.html:134: for (var pid in model.processes) { To simplify your code, I suggest using tracing/base/iteration_helpers.html (you should add it as an import at the top of the file): tr.b.iterItems(model.processes, function(pid, process) { tr.b.iterItems(process.threads, function(tid, thread) { ... }); }); https://codereview.chromium.org/2043203003/diff/40001/tracing/tracing/metrics... tracing/tracing/metrics/tracing_metric.html:140: if (slice.category != memoryInfraCategory) !== https://codereview.chromium.org/2043203003/diff/40001/tracing/tracing/metrics... tracing/tracing/metrics/tracing_metric.html:143: if (thread.name != 'MemoryInfra') !== https://codereview.chromium.org/2043203003/diff/40001/tracing/tracing/metrics... tracing/tracing/metrics/tracing_metric.html:146: var mdp_name = slice.args['dump_provider.name']; please use camel case and more descriptive name: var providerName = ... https://codereview.chromium.org/2043203003/diff/40001/tracing/tracing/metrics... tracing/tracing/metrics/tracing_metric.html:154: _addTimeDurationMetric('MemoryInfra total CPU overhead ', please remove trailing whitespace in the name https://codereview.chromium.org/2043203003/diff/40001/tracing/tracing/metrics... tracing/tracing/metrics/tracing_metric.html:154: _addTimeDurationMetric('MemoryInfra total CPU overhead ', Please say "Memory-infra dump" or "Memory-Infra dump" in all names. https://codereview.chromium.org/2043203003/diff/40001/tracing/tracing/metrics... tracing/tracing/metrics/tracing_metric.html:156: _addTimeDurationMetric('MemoryInfra CPU overhead MainThreads', Please change this to a human-readable name: "Memory-infra dump total CPU overhead on main threads". Furthermore, as I suggested earlier, I think it should be "on non-memory-infra threads" instead. https://codereview.chromium.org/2043203003/diff/40001/tracing/tracing/metrics... tracing/tracing/metrics/tracing_metric.html:158: for (var provider in overheadByProviders) { I'd use tr.b.iterItems here as well https://codereview.chromium.org/2043203003/diff/40001/tracing/tracing/metrics... tracing/tracing/metrics/tracing_metric.html:159: _addTimeDurationMetric('memory_dump_cpu_overhead_' + provider, Please change this to a human readable string: 'Memory-infra dump CPU overhead of <PROVIDER>' https://codereview.chromium.org/2043203003/diff/40001/tracing/tracing/metrics... tracing/tracing/metrics/tracing_metric.html:170: 'Trace size of MemoryInfra dumps in bytes', "Total trace size ..."? https://codereview.chromium.org/2043203003/diff/40001/tracing/tracing/metrics... tracing/tracing/metrics/tracing_metric.html:175: Math.floor(memoryInfraEventsSize / memoryDumpsCount)); Is there any reason to round this? The dashboard should be able to handle decimal numbers perfectly well. https://codereview.chromium.org/2043203003/diff/40001/tracing/tracing/metrics... tracing/tracing/metrics/tracing_metric.html:177: 'Trace size per MemoryInfra dump in bytes', traceBytesPerDumpValue)); "Average trace size ..." https://codereview.chromium.org/2043203003/diff/40001/tracing/tracing/metrics... File tracing/tracing/metrics/tracing_metric_test.html (right): https://codereview.chromium.org/2043203003/diff/40001/tracing/tracing/metrics... tracing/tracing/metrics/tracing_metric_test.html:33: assert.strictEqual(values.length, 1); assert.lengthOf(values, 1); https://codereview.chromium.org/2043203003/diff/40001/tracing/tracing/metrics... tracing/tracing/metrics/tracing_metric_test.html:34: var durationInMs = Math.round(1000 * values[0].numeric.value); Why are you doing the rounding? If you're worried about imprecise calculations, I suggest you use assert.closeTo(actual, expected, 0.001) instead. https://codereview.chromium.org/2043203003/diff/40001/tracing/tracing/metrics... tracing/tracing/metrics/tracing_metric_test.html:35: assert.strictEqual(expected, durationInMs); The order should be ACTUAL, EXPECTED https://codereview.chromium.org/2043203003/diff/40001/tracing/tracing/metrics... tracing/tracing/metrics/tracing_metric_test.html:121: var memoryCategory = 'disabled-by-default-memory-infra'; MEMORY_INFRA_TRACING_CATEGORY Since you already defined it in the tested file, I suggest you export it from there for testing purposes so that you wouldn't have to define it twice (you need to add this to the return statement in that file): MEMORY_INFRA_TRACING_CATEGORY: MEMORY_INFRA_TRACING_CATEGORY // For testing only. https://codereview.chromium.org/2043203003/diff/40001/tracing/tracing/metrics... tracing/tracing/metrics/tracing_metric_test.html:123: {name: 'OnMemoryDump', pid: 1, ts: 510, tid: 1, ph: 'X', cat: memoryCategory, args: {'dump_provider.name': 'mdp1'}, dur: 4}, // @suppress longLineCheck Could you please insert line breaks in to the objects instead of suppressing every single line (we try to use @suppress only if necessary): {name: ..., pid: ..., ... cat: ..., ...}, https://codereview.chromium.org/2043203003/diff/40001/tracing/tracing/metrics... tracing/tracing/metrics/tracing_metric_test.html:140: tr.model.MemoryDumpTestUtils.addGlobalMemoryDump(model, 550); Why don't you just add a process memory dump event to the raw |events|? https://codereview.chromium.org/2043203003/diff/40001/tracing/tracing/metrics... tracing/tracing/metrics/tracing_metric_test.html:143: var sizeByCategory = events.reduce((map, slice) => ( this is sort of difficult to read. Please do the following instead: var sizeByCategory = new Map(); events.forEach(function(slice) { var key = slice['cat']; map.set(key, (map.get(key) || 0) + JSON.stringify(slice).length)); }); In fact, now that I'm looking at this, why do you build the map in the first place when you only use the size of the memory category afterwards? Just use an integer. At that point, reduce starts to make sense: var memoryCategorySize = events.reduce((acc, slice) => acc + (slice['cat'] === MEMORY_INFRA_TRACING_CATEGORY ? JSON.stringigy(slice).length : 0), 0); The following might be even easier to understand (it's a test, so performance is not super-critical): var memoryCategorySize = events.filter(slice => slice['cat'] === MEMORY_INFRA_TRACING_CATEGORY).reduce((acc, slice) => acc + JSON.stringify(slice).length, 0); https://codereview.chromium.org/2043203003/diff/40001/tracing/tracing/metrics... tracing/tracing/metrics/tracing_metric_test.html:149: assert.strictEqual(totalSizeValue.length, 1); assert.lengthOf https://codereview.chromium.org/2043203003/diff/40001/tracing/tracing/metrics... tracing/tracing/metrics/tracing_metric_test.html:154: assert.strictEqual(sizePerDumpValue.length, 1); assert.lengthOf
https://codereview.chromium.org/2043203003/diff/40001/telemetry/telemetry/tim... File telemetry/telemetry/timeline/tracing_config.py (right): https://codereview.chromium.org/2043203003/diff/40001/telemetry/telemetry/tim... telemetry/telemetry/timeline/tracing_config.py:67: assert mode in ['background', 'light', 'detailed'] I see that memory dump triggers are removed from devtools protocol. How is this configuration passed to Chrome now? So devtools can treat memory dump config object as a black box and we do not need to specify the detailed properties inside? See https://codereview.chromium.org/1911643002/diff/260001/third_party/WebKit/Sou...
Thanks for your patient review. js is vast, i am trying to learn. PTAL. https://codereview.chromium.org/2043203003/diff/40001/telemetry/telemetry/tim... File telemetry/telemetry/timeline/tracing_config.py (right): https://codereview.chromium.org/2043203003/diff/40001/telemetry/telemetry/tim... telemetry/telemetry/timeline/tracing_config.py:67: assert mode in ['background', 'light', 'detailed'] On 2016/06/09 17:08:17, Zhen Wang wrote: > I see that memory dump triggers are removed from devtools protocol. How is this > configuration passed to Chrome now? So devtools can treat memory dump config > object as a black box and we do not need to specify the detailed properties > inside? > > See > https://codereview.chromium.org/1911643002/diff/260001/third_party/WebKit/Sou... Yes, the devtool protocol definition does not contain memory dump config details because the dump config is frequently changing and not stable. So, the plan is to add the detailed description there once the config is more stable. For now, this is treated as black box and the strings are still passed. https://codereview.chromium.org/2043203003/diff/40001/tracing/tracing/metrics... File tracing/tracing/metrics/tracing_metric.html (right): https://codereview.chromium.org/2043203003/diff/40001/tracing/tracing/metrics... tracing/tracing/metrics/tracing_metric.html:17: function _addTimeDurationMetric(metricName, duration, allValues) { On 2016/06/09 13:38:19, petrcermak wrote: > no need for preceding underscore. Since you don't export this function, it won't > be visible outside anyway ;-) > > Also, it should be addTimeDuration*Value* and *value*Name. I thought private functions should have _. fixed both. https://codereview.chromium.org/2043203003/diff/40001/tracing/tracing/metrics... tracing/tracing/metrics/tracing_metric.html:123: // The metrics below are specific to memory infra dumps. On 2016/06/09 13:38:19, petrcermak wrote: > s/metrics/values/ Done. https://codereview.chromium.org/2043203003/diff/40001/tracing/tracing/metrics... tracing/tracing/metrics/tracing_metric.html:123: // The metrics below are specific to memory infra dumps. On 2016/06/09 13:38:19, petrcermak wrote: > supernit: s/memory infra/memory-infra/ Done. https://codereview.chromium.org/2043203003/diff/40001/tracing/tracing/metrics... tracing/tracing/metrics/tracing_metric.html:123: // The metrics below are specific to memory infra dumps. On 2016/06/09 13:38:20, petrcermak wrote: > please factor your code into a separate function if possible (e.g. > addMemoryInfraValues or something like that) Done. https://codereview.chromium.org/2043203003/diff/40001/tracing/tracing/metrics... tracing/tracing/metrics/tracing_metric.html:124: var memoryInfraCategory = 'disabled-by-default-memory-infra'; On 2016/06/09 13:38:20, petrcermak wrote: > This should be a top-level constant defined outside this function. Please call > it MEMORY_INFRA_TRACING_CATEGORY. Done. https://codereview.chromium.org/2043203003/diff/40001/tracing/tracing/metrics... tracing/tracing/metrics/tracing_metric.html:125: var memoryDumpsCount = model.globalMemoryDumps.length; On 2016/06/09 13:38:20, petrcermak wrote: > nit: should be "memoryDumpCount" (not plural) to be consistent with everywhere > else. Done. https://codereview.chromium.org/2043203003/diff/40001/tracing/tracing/metrics... tracing/tracing/metrics/tracing_metric.html:127: memoryDumpsCount == 0) { On 2016/06/09 13:38:19, petrcermak wrote: > Always use === instead of ==. Done. https://codereview.chromium.org/2043203003/diff/40001/tracing/tracing/metrics... tracing/tracing/metrics/tracing_metric.html:132: var mainThreadsOverhead = 0; On 2016/06/09 13:38:20, petrcermak wrote: > Shouldn't this be nonMemoryInfraThreadOverhead? Done. https://codereview.chromium.org/2043203003/diff/40001/tracing/tracing/metrics... tracing/tracing/metrics/tracing_metric.html:133: var overheadByProviders = {}; On 2016/06/09 13:38:19, petrcermak wrote: > nit: overheadByProvider (not plural) Done. https://codereview.chromium.org/2043203003/diff/40001/tracing/tracing/metrics... tracing/tracing/metrics/tracing_metric.html:134: for (var pid in model.processes) { On 2016/06/09 13:38:19, petrcermak wrote: > To simplify your code, I suggest using tracing/base/iteration_helpers.html (you > should add it as an import at the top of the file): > > tr.b.iterItems(model.processes, function(pid, process) { > tr.b.iterItems(process.threads, function(tid, thread) { > ... > }); > }); Done. https://codereview.chromium.org/2043203003/diff/40001/tracing/tracing/metrics... tracing/tracing/metrics/tracing_metric.html:140: if (slice.category != memoryInfraCategory) On 2016/06/09 13:38:19, petrcermak wrote: > !== Done. https://codereview.chromium.org/2043203003/diff/40001/tracing/tracing/metrics... tracing/tracing/metrics/tracing_metric.html:143: if (thread.name != 'MemoryInfra') On 2016/06/09 13:38:19, petrcermak wrote: > !== Done. https://codereview.chromium.org/2043203003/diff/40001/tracing/tracing/metrics... tracing/tracing/metrics/tracing_metric.html:146: var mdp_name = slice.args['dump_provider.name']; On 2016/06/09 13:38:20, petrcermak wrote: > please use camel case and more descriptive name: > > var providerName = ... Done. https://codereview.chromium.org/2043203003/diff/40001/tracing/tracing/metrics... tracing/tracing/metrics/tracing_metric.html:154: _addTimeDurationMetric('MemoryInfra total CPU overhead ', On 2016/06/09 13:38:19, petrcermak wrote: > please remove trailing whitespace in the name Done. https://codereview.chromium.org/2043203003/diff/40001/tracing/tracing/metrics... tracing/tracing/metrics/tracing_metric.html:154: _addTimeDurationMetric('MemoryInfra total CPU overhead ', On 2016/06/09 13:38:20, petrcermak wrote: > Please say "Memory-infra dump" or "Memory-Infra dump" in all names. Done. https://codereview.chromium.org/2043203003/diff/40001/tracing/tracing/metrics... tracing/tracing/metrics/tracing_metric.html:156: _addTimeDurationMetric('MemoryInfra CPU overhead MainThreads', On 2016/06/09 13:38:19, petrcermak wrote: > Please change this to a human-readable name: "Memory-infra dump total CPU > overhead on main threads". > > Furthermore, as I suggested earlier, I think it should be "on non-memory-infra > threads" instead. Done. https://codereview.chromium.org/2043203003/diff/40001/tracing/tracing/metrics... tracing/tracing/metrics/tracing_metric.html:158: for (var provider in overheadByProviders) { On 2016/06/09 13:38:19, petrcermak wrote: > I'd use tr.b.iterItems here as well Done. https://codereview.chromium.org/2043203003/diff/40001/tracing/tracing/metrics... tracing/tracing/metrics/tracing_metric.html:159: _addTimeDurationMetric('memory_dump_cpu_overhead_' + provider, On 2016/06/09 13:38:20, petrcermak wrote: > Please change this to a human readable string: > > 'Memory-infra dump CPU overhead of <PROVIDER>' Done. https://codereview.chromium.org/2043203003/diff/40001/tracing/tracing/metrics... tracing/tracing/metrics/tracing_metric.html:170: 'Trace size of MemoryInfra dumps in bytes', On 2016/06/09 13:38:20, petrcermak wrote: > "Total trace size ..."? Done. https://codereview.chromium.org/2043203003/diff/40001/tracing/tracing/metrics... tracing/tracing/metrics/tracing_metric.html:175: Math.floor(memoryInfraEventsSize / memoryDumpsCount)); On 2016/06/09 13:38:19, petrcermak wrote: > Is there any reason to round this? The dashboard should be able to handle > decimal numbers perfectly well. Done. https://codereview.chromium.org/2043203003/diff/40001/tracing/tracing/metrics... tracing/tracing/metrics/tracing_metric.html:177: 'Trace size per MemoryInfra dump in bytes', traceBytesPerDumpValue)); On 2016/06/09 13:38:19, petrcermak wrote: > "Average trace size ..." Done. https://codereview.chromium.org/2043203003/diff/40001/tracing/tracing/metrics... File tracing/tracing/metrics/tracing_metric_test.html (right): https://codereview.chromium.org/2043203003/diff/40001/tracing/tracing/metrics... tracing/tracing/metrics/tracing_metric_test.html:33: assert.strictEqual(values.length, 1); On 2016/06/09 13:38:21, petrcermak wrote: > assert.lengthOf(values, 1); Done. https://codereview.chromium.org/2043203003/diff/40001/tracing/tracing/metrics... tracing/tracing/metrics/tracing_metric_test.html:34: var durationInMs = Math.round(1000 * values[0].numeric.value); On 2016/06/09 13:38:21, petrcermak wrote: > Why are you doing the rounding? If you're worried about imprecise calculations, > I suggest you use assert.closeTo(actual, expected, 0.001) instead. Done. https://codereview.chromium.org/2043203003/diff/40001/tracing/tracing/metrics... tracing/tracing/metrics/tracing_metric_test.html:35: assert.strictEqual(expected, durationInMs); On 2016/06/09 13:38:20, petrcermak wrote: > The order should be ACTUAL, EXPECTED Done. https://codereview.chromium.org/2043203003/diff/40001/tracing/tracing/metrics... tracing/tracing/metrics/tracing_metric_test.html:121: var memoryCategory = 'disabled-by-default-memory-infra'; On 2016/06/09 13:38:20, petrcermak wrote: > MEMORY_INFRA_TRACING_CATEGORY > > Since you already defined it in the tested file, I suggest you export it from > there for testing purposes so that you wouldn't have to define it twice (you > need to add this to the return statement in that file): > > MEMORY_INFRA_TRACING_CATEGORY: MEMORY_INFRA_TRACING_CATEGORY // For testing > only. Done thanks https://codereview.chromium.org/2043203003/diff/40001/tracing/tracing/metrics... tracing/tracing/metrics/tracing_metric_test.html:123: {name: 'OnMemoryDump', pid: 1, ts: 510, tid: 1, ph: 'X', cat: memoryCategory, args: {'dump_provider.name': 'mdp1'}, dur: 4}, // @suppress longLineCheck On 2016/06/09 13:38:21, petrcermak wrote: > Could you please insert line breaks in to the objects instead of suppressing > every single line (we try to use @suppress only if necessary): > > {name: ..., pid: ..., ... > cat: ..., ...}, Isaw examples in other test files like trace_event_importer_test.html, net_async_slice_test.html and chrome_model_helper_test.html which uses suppress for the trace events similar to these. I think it looks better with suppress. https://codereview.chromium.org/2043203003/diff/40001/tracing/tracing/metrics... tracing/tracing/metrics/tracing_metric_test.html:140: tr.model.MemoryDumpTestUtils.addGlobalMemoryDump(model, 550); On 2016/06/09 13:38:21, petrcermak wrote: > Why don't you just add a process memory dump event to the raw |events|? Is it necessary? I just want this because i need the dump count to be positive. It's not requried for this test to have a process dump. https://codereview.chromium.org/2043203003/diff/40001/tracing/tracing/metrics... tracing/tracing/metrics/tracing_metric_test.html:143: var sizeByCategory = events.reduce((map, slice) => ( On 2016/06/09 13:38:20, petrcermak wrote: > this is sort of difficult to read. Please do the following instead: > > var sizeByCategory = new Map(); > events.forEach(function(slice) { > var key = slice['cat']; > map.set(key, (map.get(key) || 0) + JSON.stringify(slice).length)); > }); > > In fact, now that I'm looking at this, why do you build the map in the first > place when you only use the size of the memory category afterwards? Just use an > integer. At that point, reduce starts to make sense: > > var memoryCategorySize = events.reduce((acc, slice) => acc + (slice['cat'] === > MEMORY_INFRA_TRACING_CATEGORY ? JSON.stringigy(slice).length : 0), 0); > > The following might be even easier to understand (it's a test, so performance is > not super-critical): > > var memoryCategorySize = events.filter(slice => slice['cat'] === > MEMORY_INFRA_TRACING_CATEGORY).reduce((acc, slice) => acc + > JSON.stringify(slice).length, 0); Thanks for your help. it's very hard for to find the correct methods to use currently. https://codereview.chromium.org/2043203003/diff/40001/tracing/tracing/metrics... tracing/tracing/metrics/tracing_metric_test.html:149: assert.strictEqual(totalSizeValue.length, 1); On 2016/06/09 13:38:20, petrcermak wrote: > assert.lengthOf Done. https://codereview.chromium.org/2043203003/diff/40001/tracing/tracing/metrics... tracing/tracing/metrics/tracing_metric_test.html:154: assert.strictEqual(sizePerDumpValue.length, 1); On 2016/06/09 13:38:20, petrcermak wrote: > assert.lengthOf Done.
I think we're almost there :-) I've left only a few more inline comments. Thanks, Petr https://codereview.chromium.org/2043203003/diff/40001/tracing/tracing/metrics... File tracing/tracing/metrics/tracing_metric.html (right): https://codereview.chromium.org/2043203003/diff/40001/tracing/tracing/metrics... tracing/tracing/metrics/tracing_metric.html:17: function _addTimeDurationMetric(metricName, duration, allValues) { On 2016/06/09 20:21:16, ssid wrote: > On 2016/06/09 13:38:19, petrcermak wrote: > > no need for preceding underscore. Since you don't export this function, it > won't > > be visible outside anyway ;-) > > > > Also, it should be addTimeDuration*Value* and *value*Name. > > I thought private functions should have _. fixed both. We do this only for private fields and methods of objects. https://codereview.chromium.org/2043203003/diff/40001/tracing/tracing/metrics... File tracing/tracing/metrics/tracing_metric_test.html (right): https://codereview.chromium.org/2043203003/diff/40001/tracing/tracing/metrics... tracing/tracing/metrics/tracing_metric_test.html:123: {name: 'OnMemoryDump', pid: 1, ts: 510, tid: 1, ph: 'X', cat: memoryCategory, args: {'dump_provider.name': 'mdp1'}, dur: 4}, // @suppress longLineCheck On 2016/06/09 20:21:17, ssid wrote: > On 2016/06/09 13:38:21, petrcermak wrote: > > Could you please insert line breaks in to the objects instead of suppressing > > every single line (we try to use @suppress only if necessary): > > > > {name: ..., pid: ..., ... > > cat: ..., ...}, > > Isaw examples in other test files like trace_event_importer_test.html, > net_async_slice_test.html and chrome_model_helper_test.html which uses suppress > for the trace events similar to these. I think it looks better with suppress. Acknowledged. https://codereview.chromium.org/2043203003/diff/40001/tracing/tracing/metrics... tracing/tracing/metrics/tracing_metric_test.html:140: tr.model.MemoryDumpTestUtils.addGlobalMemoryDump(model, 550); On 2016/06/09 20:21:17, ssid wrote: > On 2016/06/09 13:38:21, petrcermak wrote: > > Why don't you just add a process memory dump event to the raw |events|? > > Is it necessary? I just want this because i need the dump count to be positive. > It's not requried for this test to have a process dump. By that logic you could also drop the __metadata events, which you could inject manually as well. Anyway, if you really want it this way (even though my proposal would require roughly the same amount of code and, in my opinion, be more consistent), it's fine. https://codereview.chromium.org/2043203003/diff/80001/tracing/tracing/metrics... File tracing/tracing/metrics/tracing_metric.html (right): https://codereview.chromium.org/2043203003/diff/80001/tracing/tracing/metrics... tracing/tracing/metrics/tracing_metric.html:52: addTimeDurationValue('Memory-Infra dump total CPU overhead', Now that I look at the expression, I think that the names should be: Average CPU overhead on all threads per Memory-Infra dump Average CPU overhead on non-memory-infra threads per Memory-Infra dump Average CPU overhead of <PROVIDER> per Memory-Infra dump https://codereview.chromium.org/2043203003/diff/80001/tracing/tracing/metrics... tracing/tracing/metrics/tracing_metric.html:174: if (model.categories.indexOf(MEMORY_INFRA_TRACING_CATEGORY) >= 0) { Two things: 1) Why is one check here and one check in the helper function? Please do all checks in one place if possible. 2) Why do you need both checks? Wouldn't one of them be enough? If the model contains memory dumps, I'd argue that we should report the values irrespective of whether the memory-infra tracing category was enabled or not. https://codereview.chromium.org/2043203003/diff/80001/tracing/tracing/metrics... tracing/tracing/metrics/tracing_metric.html:177: addMemoryInfraValues(values, model, memoryInfraEventsSize); I think you should hide all the memory-infra specific stuff in the helper function. Hence, I would pass |categoryNamesToTotalEventSizes| to the helper function and let it extract what it needs. This of course requires pushing the if check on line 174 into the helper function (point 1 in my previous comment) unless you drop it completely (point 2 in my previous comment).
Thanks, made suggested changes. https://codereview.chromium.org/2043203003/diff/80001/tracing/tracing/metrics... File tracing/tracing/metrics/tracing_metric.html (right): https://codereview.chromium.org/2043203003/diff/80001/tracing/tracing/metrics... tracing/tracing/metrics/tracing_metric.html:52: addTimeDurationValue('Memory-Infra dump total CPU overhead', On 2016/06/10 11:07:06, petrcermak wrote: > Now that I look at the expression, I think that the names should be: > > Average CPU overhead on all threads per Memory-Infra dump > Average CPU overhead on non-memory-infra threads per Memory-Infra dump > Average CPU overhead of <PROVIDER> per Memory-Infra dump Done. https://codereview.chromium.org/2043203003/diff/80001/tracing/tracing/metrics... tracing/tracing/metrics/tracing_metric.html:174: if (model.categories.indexOf(MEMORY_INFRA_TRACING_CATEGORY) >= 0) { On 2016/06/10 11:07:06, petrcermak wrote: > Two things: > > 1) Why is one check here and one check in the helper function? Please do all > checks in one place if possible. > 2) Why do you need both checks? Wouldn't one of them be enough? If the model > contains memory dumps, I'd argue that we should report the values irrespective > of whether the memory-infra tracing category was enabled or not. Yeah makes sense. removed. https://codereview.chromium.org/2043203003/diff/80001/tracing/tracing/metrics... tracing/tracing/metrics/tracing_metric.html:177: addMemoryInfraValues(values, model, memoryInfraEventsSize); On 2016/06/10 11:07:07, petrcermak wrote: > I think you should hide all the memory-infra specific stuff in the helper > function. Hence, I would pass |categoryNamesToTotalEventSizes| to the helper > function and let it extract what it needs. This of course requires pushing the > if check on line 174 into the helper function (point 1 in my previous comment) > unless you drop it completely (point 2 in my previous comment). Done. Just the function call here. Removed the check
https://codereview.chromium.org/2043203003/diff/40001/tracing/tracing/metrics... File tracing/tracing/metrics/tracing_metric_test.html (right): https://codereview.chromium.org/2043203003/diff/40001/tracing/tracing/metrics... tracing/tracing/metrics/tracing_metric_test.html:140: tr.model.MemoryDumpTestUtils.addGlobalMemoryDump(model, 550); On 2016/06/10 11:07:06, petrcermak wrote: > On 2016/06/09 20:21:17, ssid wrote: > > On 2016/06/09 13:38:21, petrcermak wrote: > > > Why don't you just add a process memory dump event to the raw |events|? > > > > Is it necessary? I just want this because i need the dump count to be > positive. > > It's not requried for this test to have a process dump. > > By that logic you could also drop the __metadata events, which you could inject > manually as well. Anyway, if you really want it this way (even though my > proposal would require roughly the same amount of code and, in my opinion, be > more consistent), it's fine. I would have to add helper functions to utils for adding metadata events into model. I tried to add global memory dump events in the events list and I had to add processmemory dumps and other events for the valid global memory dump to get added to the model. So, calling the addDump was easier. Here the events list has a list of trace events and thread names that are required for the test and global dump is not really necessary. If this should be consistent then I think all the events should be added with these helper methods.
LGTM with one final nit. Thanks, Petr https://codereview.chromium.org/2043203003/diff/120001/tracing/tracing/metric... File tracing/tracing/metrics/tracing_metric.html (right): https://codereview.chromium.org/2043203003/diff/120001/tracing/tracing/metric... tracing/tracing/metrics/tracing_metric.html:53: 'Average CPU overhead on all threads per Memory-Infra dump', I've just realized that we "Memory-Infra" but "non-memory-infra" (inconsistent case). I'd probably just go for "memory-infra" (lowercase) everywhere.
https://codereview.chromium.org/2043203003/diff/120001/tracing/tracing/metric... File tracing/tracing/metrics/tracing_metric.html (right): https://codereview.chromium.org/2043203003/diff/120001/tracing/tracing/metric... tracing/tracing/metrics/tracing_metric.html:178: addMemoryInfraValues(values, model, categoryNamesToTotalEventSizes); Should we add you (or anyone from memory infra team) as a test owner for memory related regression?
https://codereview.chromium.org/2043203003/diff/120001/tracing/tracing/metric... File tracing/tracing/metrics/tracing_metric.html (right): https://codereview.chromium.org/2043203003/diff/120001/tracing/tracing/metric... tracing/tracing/metrics/tracing_metric.html:178: addMemoryInfraValues(values, model, categoryNamesToTotalEventSizes); On 2016/06/13 17:55:37, Zhen Wang wrote: > Should we add you (or anyone from memory infra team) as a test owner for memory > related regression? Yes, that sounds good. But, maybe this should be done along with this CL that adds the actual benchamrk : https://codereview.chromium.org/2052753002/ It would be great if you can tell me how to do it. Thanks
lgtm
Thanks. https://codereview.chromium.org/2043203003/diff/120001/tracing/tracing/metric... File tracing/tracing/metrics/tracing_metric.html (right): https://codereview.chromium.org/2043203003/diff/120001/tracing/tracing/metric... tracing/tracing/metrics/tracing_metric.html:53: 'Average CPU overhead on all threads per Memory-Infra dump', On 2016/06/13 08:26:30, petrcermak wrote: > I've just realized that we "Memory-Infra" but "non-memory-infra" (inconsistent > case). I'd probably just go for "memory-infra" (lowercase) everywhere. 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/patch-status/2043203003/140001
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 zhenw@chromium.org, petrcermak@chromium.org Link to the patchset: https://codereview.chromium.org/2043203003/#ps140001 (title: "Renames to memory-infra.")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/2043203003/140001
Message was sent while issue was closed.
Description was changed from ========== Add MemoryInfra overheads to tracingMetrics This CL adds metrics to track the overhead of MemoryInfra dumps to tracingMetrics. This is useful to track the memory dump timings and trace size. Tracks the duration of memory-infra trace events, duration of each dump provider and total json size of memory-infra trace events. BUG=catapult:#2400 ========== to ========== Add MemoryInfra overheads to tracingMetrics This CL adds metrics to track the overhead of MemoryInfra dumps to tracingMetrics. This is useful to track the memory dump timings and trace size. Tracks the duration of memory-infra trace events, duration of each dump provider and total json size of memory-infra trace events. BUG=catapult:#2400 Committed: https://chromium.googlesource.com/external/github.com/catapult-project/catapu... ==========
Message was sent while issue was closed.
Committed patchset #7 (id:140001) as https://chromium.googlesource.com/external/github.com/catapult-project/catapu... |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
