Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1336373002:
Port rendering_stats' implementation to javascript (Closed)
DescriptionPort rendering_stats' implementation to javascript
The original implementation can be found here:
https://code.google.com/p/chromium/codesearch#chromium/src/tools/telemetry/telemetry/web_perf/metrics/rendering_stats.py
Steps:
1) Add rendering_frame.html (DONE)
2) Add rendering_stats.html (which uses rendering_frame.html - this patch)
3) Add smoothness.html (which uses rendering_stats.html)
BUG=#1228
Patch Set 1 #
Total comments: 99
Patch Set 2 : Address some of the review comments #Patch Set 3 : Address Dan's reviews #
Total comments: 8
Patch Set 4 : Address some of Nat's comments #
Total comments: 27
Messages
Total messages: 11 (3 generated)
nednguyen@google.com changed reviewers: + beaudoin@chromium.org, dsinclair@chromium.org
dsinclair@chromium.org changed reviewers: + nduca@chromium.org
https://codereview.chromium.org/1336373002/diff/1/perf_insights/perf_insights... File perf_insights/perf_insights/timeline_based_measurement/rendering_stats.html (right): https://codereview.chromium.org/1336373002/diff/1/perf_insights/perf_insights... perf_insights/perf_insights/timeline_based_measurement/rendering_stats.html:12: href="/perf_insights/timeline_based_measurement/rendering_frame.html"> Move this before the tracing importers. https://codereview.chromium.org/1336373002/diff/1/perf_insights/perf_insights... perf_insights/perf_insights/timeline_based_measurement/rendering_stats.html:28: return Math.round(number * k) / k; We should round at the latest point possible, is this needed here? https://codereview.chromium.org/1336373002/diff/1/perf_insights/perf_insights... perf_insights/perf_insights/timeline_based_measurement/rendering_stats.html:35: * Input events dump their LatencyInfo into trace buffer as async trace There is also InputLatencyAsyncSlice events. This should probably share code with either extras/chrome/cc/input_latency_async_slice.html or the IR code to find the events. https://codereview.chromium.org/1336373002/diff/1/perf_insights/perf_insights... perf_insights/perf_insights/timeline_based_measurement/rendering_stats.html:37: * starting with "Latency". The trace event has a memeber 'data' containing nit: s/memeber/member https://codereview.chromium.org/1336373002/diff/1/perf_insights/perf_insights... perf_insights/perf_insights/timeline_based_measurement/rendering_stats.html:50: latencyEvents.push.apply( Why is this push.apply and not just push? https://codereview.chromium.org/1336373002/diff/1/perf_insights/perf_insights... perf_insights/perf_insights/timeline_based_measurement/rendering_stats.html:53: return e.args['data'] !== undefined; Can this happen? What does it mean if the 'data' is undefined? https://codereview.chromium.org/1336373002/diff/1/perf_insights/perf_insights... perf_insights/perf_insights/timeline_based_measurement/rendering_stats.html:86: var endTime, startTime; These should be initialized to undefined. https://codereview.chromium.org/1336373002/diff/1/perf_insights/perf_insights... perf_insights/perf_insights/timeline_based_measurement/rendering_stats.html:87: if (constants.END_COMP_NAME in data) { if (data[constants.END_COMP_NAME]) the 'in' is difficult to read in the blob of text. https://codereview.chromium.org/1336373002/diff/1/perf_insights/perf_insights... perf_insights/perf_insights/timeline_based_measurement/rendering_stats.html:89: if (constants.ORIGINAL_COMP_NAME in data) { [contants.ORIGINAL_COMP_NAME, contants.UI_COMP_NAME, ...].forEach(function(name) { if (data[name]) starTime = data[name]['time']; }); if (startTime === undefined) throw .... https://codereview.chromium.org/1336373002/diff/1/perf_insights/perf_insights... perf_insights/perf_insights/timeline_based_measurement/rendering_stats.html:100: } if END_COMP_NAME is not in data then endTime and startTime will be undefined. I don't think you can subtract undefined from undefined. https://codereview.chromium.org/1336373002/diff/1/perf_insights/perf_insights... perf_insights/perf_insights/timeline_based_measurement/rendering_stats.html:101: var latency = (endTime - startTime) / 1000.0; Why / 1000.0? https://codereview.chromium.org/1336373002/diff/1/perf_insights/perf_insights... perf_insights/perf_insights/timeline_based_measurement/rendering_stats.html:104: title: event.title, Why not return the event instead of just the title? https://codereview.chromium.org/1336373002/diff/1/perf_insights/perf_insights... perf_insights/perf_insights/timeline_based_measurement/rendering_stats.html:113: return { Why not just return inputEventLatencies? https://codereview.chromium.org/1336373002/diff/1/perf_insights/perf_insights... perf_insights/perf_insights/timeline_based_measurement/rendering_stats.html:119: nit: remove blank line https://codereview.chromium.org/1336373002/diff/1/perf_insights/perf_insights... perf_insights/perf_insights/timeline_based_measurement/rendering_stats.html:122: * Returns true if the process contains at least one Isn't this exactly one? https://codereview.chromium.org/1336373002/diff/1/perf_insights/perf_insights... perf_insights/perf_insights/timeline_based_measurement/rendering_stats.html:141: function getTimestampEventName(process) { Should this just be an inner function of RenderingStats below? https://codereview.chromium.org/1336373002/diff/1/perf_insights/perf_insights... perf_insights/perf_insights/timeline_based_measurement/rendering_stats.html:143: return 'vsync_before'; These should be constants if they're coming from the trace https://codereview.chromium.org/1336373002/diff/1/perf_insights/perf_insights... perf_insights/perf_insights/timeline_based_measurement/rendering_stats.html:157: nit: remove blank line. https://codereview.chromium.org/1336373002/diff/1/perf_insights/perf_insights... perf_insights/perf_insights/timeline_based_measurement/rendering_stats.html:164: * timeline range. I don't understand this, the parent list is timestamps and the inner list is statistics? https://codereview.chromium.org/1336373002/diff/1/perf_insights/perf_insights... perf_insights/perf_insights/timeline_based_measurement/rendering_stats.html:169: surfaceFlingerProcess, timeRanges) { nit: indenting https://codereview.chromium.org/1336373002/diff/1/perf_insights/perf_insights... perf_insights/perf_insights/timeline_based_measurement/rendering_stats.html:170: if (timeRanges.length <= 0) { I don't think this can be < 0 so just === 0 should work. https://codereview.chromium.org/1336373002/diff/1/perf_insights/perf_insights... perf_insights/perf_insights/timeline_based_measurement/rendering_stats.html:172: } nit: no {}'s on single line bodies. https://codereview.chromium.org/1336373002/diff/1/perf_insights/perf_insights... perf_insights/perf_insights/timeline_based_measurement/rendering_stats.html:175: throw new Error('timelineRanges must contain only Range object'); nit: s/object/objects/ https://codereview.chromium.org/1336373002/diff/1/perf_insights/perf_insights... perf_insights/perf_insights/timeline_based_measurement/rendering_stats.html:175: throw new Error('timelineRanges must contain only Range object'); nit: s/timelineRanges/timeRanges https://codereview.chromium.org/1336373002/diff/1/perf_insights/perf_insights... perf_insights/perf_insights/timeline_based_measurement/rendering_stats.html:177: this.refresh_period = null; We don't use null, use undefined. https://codereview.chromium.org/1336373002/diff/1/perf_insights/perf_insights... perf_insights/perf_insights/timeline_based_measurement/rendering_stats.html:177: this.refresh_period = null; This should be camel case. https://codereview.chromium.org/1336373002/diff/1/perf_insights/perf_insights... perf_insights/perf_insights/timeline_based_measurement/rendering_stats.html:195: this.errors = {}; What is this for? Why not throw errors instead of recording into an array? Are they actually errors or just warnings. https://codereview.chromium.org/1336373002/diff/1/perf_insights/perf_insights... perf_insights/perf_insights/timeline_based_measurement/rendering_stats.html:197: this.frameTimestamps = []; Instance variables should be named with a trailing _. So, this.frameTimestamps_ = []; https://codereview.chromium.org/1336373002/diff/1/perf_insights/perf_insights... perf_insights/perf_insights/timeline_based_measurement/rendering_stats.html:201: // End-to-end latency for input event - from when input event is nit: add blank line above. (and other blocks below as well) https://codereview.chromium.org/1336373002/diff/1/perf_insights/perf_insights... perf_insights/perf_insights/timeline_based_measurement/rendering_stats.html:209: this.gestureScrollUpdateLatency = []; Why not store a this.statistics_ object which then has each of these as keys? Then above: this.stats_ = { frameTimestamps: [], frameTimes: [], ... gestureScrollUpdateLatency: [] } https://codereview.chromium.org/1336373002/diff/1/perf_insights/perf_insights... perf_insights/perf_insights/timeline_based_measurement/rendering_stats.html:222: timestampProcess, timestampEventTitle, timeRange); nit: indent 4 on wrap, and below. https://codereview.chromium.org/1336373002/diff/1/perf_insights/perf_insights... perf_insights/perf_insights/timeline_based_measurement/rendering_stats.html:235: surfaceFlingerProcess) { nit: indenting. https://codereview.chromium.org/1336373002/diff/1/perf_insights/perf_insights... perf_insights/perf_insights/timeline_based_measurement/rendering_stats.html:240: } nit: no {}'s https://codereview.chromium.org/1336373002/diff/1/perf_insights/perf_insights... perf_insights/perf_insights/timeline_based_measurement/rendering_stats.html:257: this.inputEventLatency[this.inputEventLatency.length - 1] = Isn't this just .push? https://codereview.chromium.org/1336373002/diff/1/perf_insights/perf_insights... perf_insights/perf_insights/timeline_based_measurement/rendering_stats.html:264: return e.latency; This would be easier to read as: eventLatencies.forEach(function(evtLatency) { if (e.title !== constnats ...) return; this.inputEventLatency.push(e.latency); }.bind(this)); https://codereview.chromium.org/1336373002/diff/1/perf_insights/perf_insights... perf_insights/perf_insights/timeline_based_measurement/rendering_stats.html:282: return e.latency; These three blocks are the same just with a different e.title. Can you move it into a function. https://codereview.chromium.org/1336373002/diff/1/perf_insights/perf_insights... perf_insights/perf_insights/timeline_based_measurement/rendering_stats.html:290: event.start >= timeRange.min && nit: indenting. https://codereview.chromium.org/1336373002/diff/1/perf_insights/perf_insights... perf_insights/perf_insights/timeline_based_measurement/rendering_stats.html:302: var frame_count = event.args['data']['frame_count']; nit: s/frame_count/frameCount/ https://codereview.chromium.org/1336373002/diff/1/perf_insights/perf_insights... perf_insights/perf_insights/timeline_based_measurement/rendering_stats.html:305: if (frame_count === 1) { if (frame_count === 0) return; https://codereview.chromium.org/1336373002/diff/1/perf_insights/perf_insights... perf_insights/perf_insights/timeline_based_measurement/rendering_stats.html:318: process, timestampEventTitle, timeRange) { nit: indenting https://codereview.chromium.org/1336373002/diff/1/perf_insights/perf_insights... perf_insights/perf_insights/timeline_based_measurement/rendering_stats.html:333: this.errors[APPROXIMATED_PIXEL_ERROR] = ( no need for ()'s around the strings. https://codereview.chromium.org/1336373002/diff/1/perf_insights/perf_insights... perf_insights/perf_insights/timeline_based_measurement/rendering_stats.html:383: if (e.missingBeginFrameId === true) { This is the only error we're throwing, do we need special case logic for it? Lets just catch e and add the error to this.errors. Then we can get rid of the missingBeingFrameId stuff. https://codereview.chromium.org/1336373002/diff/1/perf_insights/perf_insights... perf_insights/perf_insights/timeline_based_measurement/rendering_stats.html:396: round: round, This should not be exported. https://codereview.chromium.org/1336373002/diff/1/perf_insights/perf_insights... File perf_insights/perf_insights/timeline_based_measurement/rendering_stats_test.html (right): https://codereview.chromium.org/1336373002/diff/1/perf_insights/perf_insights... perf_insights/perf_insights/timeline_based_measurement/rendering_stats_test.html:1: <!DOCTYPE HTML> nit s/HTML/html (and in other files) https://codereview.chromium.org/1336373002/diff/1/perf_insights/perf_insights... perf_insights/perf_insights/timeline_based_measurement/rendering_stats_test.html:33: function random(low, high) { Randomness in tests make them hard to debug. Why do we need random values instead of just pre-determined ones? https://codereview.chromium.org/1336373002/diff/1/tracing/tracing/base/utils.... File tracing/tracing/base/utils.html (right): https://codereview.chromium.org/1336373002/diff/1/tracing/tracing/base/utils.... tracing/tracing/base/utils.html:56: function flattenArray(array_of_arrays) { If this is just for tests move it into the test file.
Thanks for your thorough review, Dan! I will work on are code with either extras/chrome/cc/input_latency_async_slice.html after other issues are addressed. https://codereview.chromium.org/1336373002/diff/1/perf_insights/perf_insights... File perf_insights/perf_insights/timeline_based_measurement/rendering_stats.html (right): https://codereview.chromium.org/1336373002/diff/1/perf_insights/perf_insights... perf_insights/perf_insights/timeline_based_measurement/rendering_stats.html:12: href="/perf_insights/timeline_based_measurement/rendering_frame.html"> On 2015/09/15 20:48:32, dsinclair wrote: > Move this before the tracing importers. Done. https://codereview.chromium.org/1336373002/diff/1/perf_insights/perf_insights... perf_insights/perf_insights/timeline_based_measurement/rendering_stats.html:28: return Math.round(number * k) / k; On 2015/09/15 20:48:31, dsinclair wrote: > We should round at the latest point possible, is this needed here? This is from the original implementation: https://code.google.com/p/chromium/codesearch#chromium/src/tools/telemetry/te... I think we should keep the js implementation as close to the existing one as much as possible. Then do subsequent cleanup that may cause regression on the metrics. Wdyt? https://codereview.chromium.org/1336373002/diff/1/perf_insights/perf_insights... perf_insights/perf_insights/timeline_based_measurement/rendering_stats.html:37: * starting with "Latency". The trace event has a memeber 'data' containing On 2015/09/15 20:48:32, dsinclair wrote: > nit: s/memeber/member Done. https://codereview.chromium.org/1336373002/diff/1/perf_insights/perf_insights... perf_insights/perf_insights/timeline_based_measurement/rendering_stats.html:50: latencyEvents.push.apply( On 2015/09/15 20:48:32, dsinclair wrote: > Why is this push.apply and not just push? Done. https://codereview.chromium.org/1336373002/diff/1/perf_insights/perf_insights... perf_insights/perf_insights/timeline_based_measurement/rendering_stats.html:53: return e.args['data'] !== undefined; On 2015/09/15 20:48:32, dsinclair wrote: > Can this happen? What does it mean if the 'data' is undefined? This is from the original implementation: https://code.google.com/p/chromium/codesearch#chromium/src/tools/telemetry/te... My guess is this is needed for backward compat with the old browser. The 'data' field probably only added after a certain point? https://codereview.chromium.org/1336373002/diff/1/perf_insights/perf_insights... perf_insights/perf_insights/timeline_based_measurement/rendering_stats.html:86: var endTime, startTime; On 2015/09/15 20:48:32, dsinclair wrote: > These should be initialized to undefined. Done. https://codereview.chromium.org/1336373002/diff/1/perf_insights/perf_insights... perf_insights/perf_insights/timeline_based_measurement/rendering_stats.html:87: if (constants.END_COMP_NAME in data) { On 2015/09/15 20:48:32, dsinclair wrote: > if (data[constants.END_COMP_NAME]) > > the 'in' is difficult to read in the blob of text. Done. https://codereview.chromium.org/1336373002/diff/1/perf_insights/perf_insights... perf_insights/perf_insights/timeline_based_measurement/rendering_stats.html:89: if (constants.ORIGINAL_COMP_NAME in data) { On 2015/09/15 20:48:31, dsinclair wrote: > [contants.ORIGINAL_COMP_NAME, contants.UI_COMP_NAME, ...].forEach(function(name) > { > if (data[name]) > starTime = data[name]['time']; > }); > if (startTime === undefined) > throw .... Done. Very nice. Thanks Dan. https://codereview.chromium.org/1336373002/diff/1/perf_insights/perf_insights... perf_insights/perf_insights/timeline_based_measurement/rendering_stats.html:100: } On 2015/09/15 20:48:32, dsinclair wrote: > if END_COMP_NAME is not in data then endTime and startTime will be undefined. I > don't think you can subtract undefined from undefined. Done. Sorry, the latency & push should be move into the " if (data[constants.ORIGNAL_COMP_NAME]) {...}" block (https://code.google.com/p/chromium/codesearch#chromium/src/tools/telemetry/te...) https://codereview.chromium.org/1336373002/diff/1/perf_insights/perf_insights... perf_insights/perf_insights/timeline_based_measurement/rendering_stats.html:101: var latency = (endTime - startTime) / 1000.0; On 2015/09/15 20:48:31, dsinclair wrote: > Why / 1000.0? Because the latency is measured in second, I think (https://code.google.com/p/chromium/codesearch#chromium/src/tools/telemetry/te...). I would want to preserve existing logic when porting this. https://codereview.chromium.org/1336373002/diff/1/perf_insights/perf_insights... perf_insights/perf_insights/timeline_based_measurement/rendering_stats.html:104: title: event.title, On 2015/09/15 20:48:33, dsinclair wrote: > Why not return the event instead of just the title? Because the call sites that use this only need the event title. https://codereview.chromium.org/1336373002/diff/1/perf_insights/perf_insights... perf_insights/perf_insights/timeline_based_measurement/rendering_stats.html:113: return { On 2015/09/15 20:48:32, dsinclair wrote: > Why not just return inputEventLatencies? Done. Good idea. I just blindly followed the legacy code. https://codereview.chromium.org/1336373002/diff/1/perf_insights/perf_insights... perf_insights/perf_insights/timeline_based_measurement/rendering_stats.html:119: On 2015/09/15 20:48:31, dsinclair wrote: > nit: remove blank line Done. https://codereview.chromium.org/1336373002/diff/1/perf_insights/perf_insights... perf_insights/perf_insights/timeline_based_measurement/rendering_stats.html:122: * Returns true if the process contains at least one On 2015/09/15 20:48:31, dsinclair wrote: > Isn't this exactly one? This is "at least one" since we set processHasRenderingStats to true for any event in process.iterateAllEvents that satisfies the check as below. https://codereview.chromium.org/1336373002/diff/1/perf_insights/perf_insights... perf_insights/perf_insights/timeline_based_measurement/rendering_stats.html:141: function getTimestampEventName(process) { On 2015/09/15 20:48:31, dsinclair wrote: > Should this just be an inner function of RenderingStats below? This doesn't access "this", so there is no need. https://codereview.chromium.org/1336373002/diff/1/perf_insights/perf_insights... perf_insights/perf_insights/timeline_based_measurement/rendering_stats.html:143: return 'vsync_before'; On 2015/09/15 20:48:31, dsinclair wrote: > These should be constants if they're coming from the trace Done. https://codereview.chromium.org/1336373002/diff/1/perf_insights/perf_insights... perf_insights/perf_insights/timeline_based_measurement/rendering_stats.html:157: On 2015/09/15 20:48:32, dsinclair wrote: > nit: remove blank line. Done. https://codereview.chromium.org/1336373002/diff/1/perf_insights/perf_insights... perf_insights/perf_insights/timeline_based_measurement/rendering_stats.html:164: * timeline range. On 2015/09/15 20:48:31, dsinclair wrote: > I don't understand this, the parent list is timestamps and the inner list is > statistics? The parent list contains the inner lists, in which each inner list contains statistics. https://codereview.chromium.org/1336373002/diff/1/perf_insights/perf_insights... perf_insights/perf_insights/timeline_based_measurement/rendering_stats.html:169: surfaceFlingerProcess, timeRanges) { On 2015/09/15 20:48:32, dsinclair wrote: > nit: indenting Done. https://codereview.chromium.org/1336373002/diff/1/perf_insights/perf_insights... perf_insights/perf_insights/timeline_based_measurement/rendering_stats.html:170: if (timeRanges.length <= 0) { On 2015/09/15 20:48:31, dsinclair wrote: > I don't think this can be < 0 so just === 0 should work. Done. https://codereview.chromium.org/1336373002/diff/1/perf_insights/perf_insights... perf_insights/perf_insights/timeline_based_measurement/rendering_stats.html:172: } On 2015/09/15 20:48:32, dsinclair wrote: > nit: no {}'s on single line bodies. Done. https://codereview.chromium.org/1336373002/diff/1/perf_insights/perf_insights... perf_insights/perf_insights/timeline_based_measurement/rendering_stats.html:175: throw new Error('timelineRanges must contain only Range object'); On 2015/09/15 20:48:31, dsinclair wrote: > nit: s/timelineRanges/timeRanges Done. https://codereview.chromium.org/1336373002/diff/1/perf_insights/perf_insights... perf_insights/perf_insights/timeline_based_measurement/rendering_stats.html:177: this.refresh_period = null; On 2015/09/15 20:48:32, dsinclair wrote: > This should be camel case. Done. https://codereview.chromium.org/1336373002/diff/1/perf_insights/perf_insights... perf_insights/perf_insights/timeline_based_measurement/rendering_stats.html:195: this.errors = {}; On 2015/09/15 20:48:32, dsinclair wrote: > What is this for? Why not throw errors instead of recording into an array? Are > they actually errors or just warnings. We don't want to stop the execution of the other metrics if few of them fail, so we record errors that happen instead. https://codereview.chromium.org/1336373002/diff/1/perf_insights/perf_insights... perf_insights/perf_insights/timeline_based_measurement/rendering_stats.html:197: this.frameTimestamps = []; On 2015/09/15 20:48:31, dsinclair wrote: > Instance variables should be named with a trailing _. So, this.frameTimestamps_ > = []; Done. I like this because it also enforce "immutability". Also change other variables. https://codereview.chromium.org/1336373002/diff/1/perf_insights/perf_insights... perf_insights/perf_insights/timeline_based_measurement/rendering_stats.html:201: // End-to-end latency for input event - from when input event is On 2015/09/15 20:48:31, dsinclair wrote: > nit: add blank line above. (and other blocks below as well) Done. https://codereview.chromium.org/1336373002/diff/1/perf_insights/perf_insights... perf_insights/perf_insights/timeline_based_measurement/rendering_stats.html:209: this.gestureScrollUpdateLatency = []; On 2015/09/15 20:48:32, dsinclair wrote: > Why not store a this.statistics_ object which then has each of these as keys? > > Then above: > > this.stats_ = { > frameTimestamps: [], > frameTimes: [], > ... > gestureScrollUpdateLatency: [] > } What does adding another layer of reference help us here? https://codereview.chromium.org/1336373002/diff/1/perf_insights/perf_insights... perf_insights/perf_insights/timeline_based_measurement/rendering_stats.html:222: timestampProcess, timestampEventTitle, timeRange); On 2015/09/15 20:48:32, dsinclair wrote: > nit: indent 4 on wrap, and below. Done. https://codereview.chromium.org/1336373002/diff/1/perf_insights/perf_insights... perf_insights/perf_insights/timeline_based_measurement/rendering_stats.html:235: surfaceFlingerProcess) { On 2015/09/15 20:48:32, dsinclair wrote: > nit: indenting. Done. https://codereview.chromium.org/1336373002/diff/1/perf_insights/perf_insights... perf_insights/perf_insights/timeline_based_measurement/rendering_stats.html:240: } On 2015/09/15 20:48:32, dsinclair wrote: > nit: no {}'s Done. https://codereview.chromium.org/1336373002/diff/1/perf_insights/perf_insights... perf_insights/perf_insights/timeline_based_measurement/rendering_stats.html:257: this.inputEventLatency[this.inputEventLatency.length - 1] = On 2015/09/15 20:48:32, dsinclair wrote: > Isn't this just .push? No, because this modifies the last element of this.inputEventLatency https://codereview.chromium.org/1336373002/diff/1/perf_insights/perf_insights... perf_insights/perf_insights/timeline_based_measurement/rendering_stats.html:264: return e.latency; On 2015/09/15 20:48:31, dsinclair wrote: > This would be easier to read as: > > eventLatencies.forEach(function(evtLatency) { > if (e.title !== constnats ...) > return; > this.inputEventLatency.push(e.latency); > }.bind(this)); Done. https://codereview.chromium.org/1336373002/diff/1/perf_insights/perf_insights... perf_insights/perf_insights/timeline_based_measurement/rendering_stats.html:282: return e.latency; On 2015/09/15 20:48:31, dsinclair wrote: > These three blocks are the same just with a different e.title. Can you move it > into a function. I update the code to make it less repetitive. Did not move them to function, can you ptal? https://codereview.chromium.org/1336373002/diff/1/perf_insights/perf_insights... perf_insights/perf_insights/timeline_based_measurement/rendering_stats.html:290: event.start >= timeRange.min && On 2015/09/15 20:48:32, dsinclair wrote: > nit: indenting. Done. https://codereview.chromium.org/1336373002/diff/1/perf_insights/perf_insights... perf_insights/perf_insights/timeline_based_measurement/rendering_stats.html:302: var frame_count = event.args['data']['frame_count']; On 2015/09/15 20:48:31, dsinclair wrote: > nit: s/frame_count/frameCount/ Done. https://codereview.chromium.org/1336373002/diff/1/perf_insights/perf_insights... perf_insights/perf_insights/timeline_based_measurement/rendering_stats.html:305: if (frame_count === 1) { On 2015/09/15 20:48:33, dsinclair wrote: > if (frame_count === 0) > return; Done. https://codereview.chromium.org/1336373002/diff/1/perf_insights/perf_insights... perf_insights/perf_insights/timeline_based_measurement/rendering_stats.html:318: process, timestampEventTitle, timeRange) { On 2015/09/15 20:48:31, dsinclair wrote: > nit: indenting Done. https://codereview.chromium.org/1336373002/diff/1/perf_insights/perf_insights... perf_insights/perf_insights/timeline_based_measurement/rendering_stats.html:333: this.errors[APPROXIMATED_PIXEL_ERROR] = ( On 2015/09/15 20:48:32, dsinclair wrote: > no need for ()'s around the strings. Done. https://codereview.chromium.org/1336373002/diff/1/perf_insights/perf_insights... perf_insights/perf_insights/timeline_based_measurement/rendering_stats.html:383: if (e.missingBeginFrameId === true) { On 2015/09/15 20:48:32, dsinclair wrote: > This is the only error we're throwing, do we need special case logic for it? > Lets just catch e and add the error to this.errors. > > Then we can get rid of the missingBeingFrameId stuff. Done. https://codereview.chromium.org/1336373002/diff/1/perf_insights/perf_insights... perf_insights/perf_insights/timeline_based_measurement/rendering_stats.html:396: round: round, On 2015/09/15 20:48:31, dsinclair wrote: > This should not be exported. Done. Moved round's implementation to tracing/base/math.html https://codereview.chromium.org/1336373002/diff/1/perf_insights/perf_insights... File perf_insights/perf_insights/timeline_based_measurement/rendering_stats_test.html (right): https://codereview.chromium.org/1336373002/diff/1/perf_insights/perf_insights... perf_insights/perf_insights/timeline_based_measurement/rendering_stats_test.html:1: <!DOCTYPE HTML> On 2015/09/15 20:48:33, dsinclair wrote: > nit s/HTML/html (and in other files) Done. https://codereview.chromium.org/1336373002/diff/1/perf_insights/perf_insights... perf_insights/perf_insights/timeline_based_measurement/rendering_stats_test.html:33: function random(low, high) { On 2015/09/15 20:48:33, dsinclair wrote: > Randomness in tests make them hard to debug. Why do we need random values > instead of just pre-determined ones? This is from the original implementation. Fixed https://codereview.chromium.org/1336373002/diff/1/tracing/tracing/base/utils.... File tracing/tracing/base/utils.html (right): https://codereview.chromium.org/1336373002/diff/1/tracing/tracing/base/utils.... tracing/tracing/base/utils.html:56: function flattenArray(array_of_arrays) { On 2015/09/15 20:48:33, dsinclair wrote: > If this is just for tests move it into the test file. This is also used in smoothness's implementation: https://code.google.com/p/chromium/codesearch#chromium/src/tools/telemetry/te...
https://codereview.chromium.org/1336373002/diff/40001/perf_insights/perf_insi... File perf_insights/perf_insights/timeline_based_measurement/rendering_frame.html (right): https://codereview.chromium.org/1336373002/diff/40001/perf_insights/perf_insi... perf_insights/perf_insights/timeline_based_measurement/rendering_frame.html:36: var allSendBeginFrameEvents = events.filter(function(e) { lets file a bug to reconcile rendering stats & rendering frame with chrome helper and self assign it to you relatively urgently after this lands https://codereview.chromium.org/1336373002/diff/40001/perf_insights/perf_insi... File perf_insights/perf_insights/timeline_based_measurement/rendering_stats.html (right): https://codereview.chromium.org/1336373002/diff/40001/perf_insights/perf_insi... perf_insights/perf_insights/timeline_based_measurement/rendering_stats.html:2: <!-- i think we should probably put tbm stuff in metrics/ folder, instead of having tbm folder separate from metrics https://codereview.chromium.org/1336373002/diff/40001/tracing/tracing/base/ut... File tracing/tracing/base/utils.html (right): https://codereview.chromium.org/1336373002/diff/40001/tracing/tracing/base/ut... tracing/tracing/base/utils.html:57: return [].concat.apply([], array_of_arrays); Array.prototype.concat
https://codereview.chromium.org/1336373002/diff/40001/perf_insights/perf_insi... File perf_insights/perf_insights/timeline_based_measurement/rendering_frame.html (right): https://codereview.chromium.org/1336373002/diff/40001/perf_insights/perf_insi... perf_insights/perf_insights/timeline_based_measurement/rendering_frame.html:36: var allSendBeginFrameEvents = events.filter(function(e) { On 2015/09/21 19:55:15, nduca wrote: > lets file a bug to reconcile rendering stats & rendering frame with chrome > helper and self assign it to you relatively urgently after this lands Done. https://github.com/catapult-project/catapult/issues/1491 https://codereview.chromium.org/1336373002/diff/40001/perf_insights/perf_insi... File perf_insights/perf_insights/timeline_based_measurement/rendering_stats.html (right): https://codereview.chromium.org/1336373002/diff/40001/perf_insights/perf_insi... perf_insights/perf_insights/timeline_based_measurement/rendering_stats.html:2: <!-- On 2015/09/21 19:55:15, nduca wrote: > i think we should probably put tbm stuff in metrics/ folder, instead of having > tbm folder separate from metrics Do you mean mappers/ folder? I don't see any metrics/ folder. https://codereview.chromium.org/1336373002/diff/40001/tracing/tracing/base/ut... File tracing/tracing/base/utils.html (right): https://codereview.chromium.org/1336373002/diff/40001/tracing/tracing/base/ut... tracing/tracing/base/utils.html:57: return [].concat.apply([], array_of_arrays); On 2015/09/21 19:55:16, nduca wrote: > Array.prototype.concat Done.
dsinclair@chromium.org changed reviewers: + benjhayden@chromium.org
This seems like it has some overlap with the IR finding code, things like finding InputLatency events. benjhayden@ can we lean on IR finder stuff in here to simplify this CL? https://codereview.chromium.org/1336373002/diff/1/perf_insights/perf_insights... File perf_insights/perf_insights/timeline_based_measurement/rendering_stats.html (right): https://codereview.chromium.org/1336373002/diff/1/perf_insights/perf_insights... perf_insights/perf_insights/timeline_based_measurement/rendering_stats.html:28: return Math.round(number * k) / k; On 2015/09/21 17:20:52, nednguyen wrote: > On 2015/09/15 20:48:31, dsinclair wrote: > > We should round at the latest point possible, is this needed here? > > This is from the original implementation: > https://code.google.com/p/chromium/codesearch#chromium/src/tools/telemetry/te... > > I think we should keep the js implementation as close to the existing one as > much as possible. Then do subsequent cleanup that may cause regression on the > metrics. Wdyt? I'd prefer if we do it right the first time. Do we have tests for the python version we can port over to make sure the results are the same? https://codereview.chromium.org/1336373002/diff/1/perf_insights/perf_insights... perf_insights/perf_insights/timeline_based_measurement/rendering_stats.html:35: * Input events dump their LatencyInfo into trace buffer as async trace On 2015/09/15 20:48:32, dsinclair wrote: > There is also InputLatencyAsyncSlice events. This should probably share code > with either extras/chrome/cc/input_latency_async_slice.html or the IR code to > find the events. ping. https://codereview.chromium.org/1336373002/diff/1/perf_insights/perf_insights... perf_insights/perf_insights/timeline_based_measurement/rendering_stats.html:53: return e.args['data'] !== undefined; On 2015/09/21 17:20:52, nednguyen wrote: > On 2015/09/15 20:48:32, dsinclair wrote: > > Can this happen? What does it mean if the 'data' is undefined? > > This is from the original implementation: > https://code.google.com/p/chromium/codesearch#chromium/src/tools/telemetry/te... > > My guess is this is needed for backward compat with the old browser. The 'data' > field probably only added after a certain point? Let's remove it if we don't know what it's for. If we find traces with issues we can add it back in. https://codereview.chromium.org/1336373002/diff/1/perf_insights/perf_insights... perf_insights/perf_insights/timeline_based_measurement/rendering_stats.html:101: var latency = (endTime - startTime) / 1000.0; On 2015/09/21 17:20:52, nednguyen wrote: > On 2015/09/15 20:48:31, dsinclair wrote: > > Why / 1000.0? > > Because the latency is measured in second, I think > (https://code.google.com/p/chromium/codesearch#chromium/src/tools/telemetry/te...). > I would want to preserve existing logic when porting this. We should keep the existing accuracy and convert to the display format as late as possible. https://codereview.chromium.org/1336373002/diff/1/perf_insights/perf_insights... perf_insights/perf_insights/timeline_based_measurement/rendering_stats.html:104: title: event.title, On 2015/09/21 17:20:51, nednguyen wrote: > On 2015/09/15 20:48:33, dsinclair wrote: > > Why not return the event instead of just the title? > > Because the call sites that use this only need the event title. My question is, will they need more in the future? What's the harm in passing the whole event back so callsites have access too it? https://codereview.chromium.org/1336373002/diff/1/perf_insights/perf_insights... perf_insights/perf_insights/timeline_based_measurement/rendering_stats.html:141: function getTimestampEventName(process) { On 2015/09/21 17:20:52, nednguyen wrote: > On 2015/09/15 20:48:31, dsinclair wrote: > > Should this just be an inner function of RenderingStats below? > > This doesn't access "this", so there is no need. Sorry, I meant move this to be a function defined in function RenderingStats() below. https://codereview.chromium.org/1336373002/diff/1/perf_insights/perf_insights... perf_insights/perf_insights/timeline_based_measurement/rendering_stats.html:164: * timeline range. On 2015/09/21 17:20:52, nednguyen wrote: > On 2015/09/15 20:48:31, dsinclair wrote: > > I don't understand this, the parent list is timestamps and the inner list is > > statistics? > > The parent list contains the inner lists, in which each inner list contains > statistics. So, we have a hash of lists of lists? Or how do the outer lists map back to time ranges? Is it based on index? https://codereview.chromium.org/1336373002/diff/1/perf_insights/perf_insights... perf_insights/perf_insights/timeline_based_measurement/rendering_stats.html:195: this.errors = {}; On 2015/09/21 17:20:52, nednguyen wrote: > On 2015/09/15 20:48:32, dsinclair wrote: > > What is this for? Why not throw errors instead of recording into an array? Are > > they actually errors or just warnings. > > We don't want to stop the execution of the other metrics if few of them fail, so > we record errors that happen instead. Do the failures matter? Do we display this somewhere? https://codereview.chromium.org/1336373002/diff/1/perf_insights/perf_insights... perf_insights/perf_insights/timeline_based_measurement/rendering_stats.html:209: this.gestureScrollUpdateLatency = []; On 2015/09/21 17:20:51, nednguyen wrote: > On 2015/09/15 20:48:32, dsinclair wrote: > > Why not store a this.statistics_ object which then has each of these as keys? > > > > Then above: > > > > this.stats_ = { > > frameTimestamps: [], > > frameTimes: [], > > ... > > gestureScrollUpdateLatency: [] > > } > > What does adding another layer of reference help us here? Clarity? Seems like it would be clearer to group the related things together, no? https://codereview.chromium.org/1336373002/diff/1/tracing/tracing/base/utils.... File tracing/tracing/base/utils.html (right): https://codereview.chromium.org/1336373002/diff/1/tracing/tracing/base/utils.... tracing/tracing/base/utils.html:56: function flattenArray(array_of_arrays) { On 2015/09/21 17:20:53, nednguyen wrote: > On 2015/09/15 20:48:33, dsinclair wrote: > > If this is just for tests move it into the test file. > > This is also used in smoothness's implementation: > https://code.google.com/p/chromium/codesearch#chromium/src/tools/telemetry/te... So it's used outside the tests? https://codereview.chromium.org/1336373002/diff/40001/perf_insights/perf_insi... File perf_insights/perf_insights/timeline_based_measurement/rendering_frame.html (right): https://codereview.chromium.org/1336373002/diff/40001/perf_insights/perf_insi... perf_insights/perf_insights/timeline_based_measurement/rendering_frame.html:88: throw err; No need to store err into a variable, just throw new Error. https://codereview.chromium.org/1336373002/diff/40001/perf_insights/perf_insi... File perf_insights/perf_insights/timeline_based_measurement/rendering_stats.html (right): https://codereview.chromium.org/1336373002/diff/40001/perf_insights/perf_insi... perf_insights/perf_insights/timeline_based_measurement/rendering_stats.html:1: <!DOCTYPE HTML> nit: s/HTML/html https://codereview.chromium.org/1336373002/diff/60001/perf_insights/perf_insi... File perf_insights/perf_insights/timeline_based_measurement/rendering_stats.html (right): https://codereview.chromium.org/1336373002/diff/60001/perf_insights/perf_insi... perf_insights/perf_insights/timeline_based_measurement/rendering_stats.html:89: if (data[name] && startTime === undefined) { not: no {}'s https://codereview.chromium.org/1336373002/diff/60001/perf_insights/perf_insi... perf_insights/perf_insights/timeline_based_measurement/rendering_stats.html:122: event.args['data'] && event.args['data']['frame_count'] === 1) { event.args.data && event.args.data.frame_count === 1 (and below) https://codereview.chromium.org/1336373002/diff/60001/perf_insights/perf_insi... perf_insights/perf_insights/timeline_based_measurement/rendering_stats.html:131: if (process.title === 'SurfaceFlinger') SurfaceFlinger should be a const. https://codereview.chromium.org/1336373002/diff/60001/perf_insights/perf_insi... perf_insights/perf_insights/timeline_based_measurement/rendering_stats.html:148: * other loggin facilities), and providing them in a common format to nit: s/loggin/logging https://codereview.chromium.org/1336373002/diff/60001/perf_insights/perf_insi... perf_insights/perf_insights/timeline_based_measurement/rendering_stats.html:154: * All *_time values are measured in milliseconds. It seems inconsistent that these times are milliseconds and latency times are seconds. https://codereview.chromium.org/1336373002/diff/60001/perf_insights/perf_insi... perf_insights/perf_insights/timeline_based_measurement/rendering_stats.html:171: surfaceFlingerProcess); Does this need to be wrapped? https://codereview.chromium.org/1336373002/diff/60001/perf_insights/perf_insi... perf_insights/perf_insights/timeline_based_measurement/rendering_stats.html:196: this.scrollUpdateLatency_ = []; How does this data get used? The structure of lists of lists seems weird. Do we iterate over the ranges and match up the indexes to these lists for each range and output? Or, do we iterate over all scrollUpdateLatency sub-lists and combine the results? Would something like a hash of data make more sense: [ { frameTimestamps: [], frameTimes: [], ... }, { ... } ] For each index into the time range array you get a hash of all the data for a that range instead of having to get n arrays back. https://codereview.chromium.org/1336373002/diff/60001/perf_insights/perf_insi... perf_insights/perf_insights/timeline_based_measurement/rendering_stats.html:201: for (var i = 0; i < timeRanges.length; i++) { What about: timeRanges.forEach(function(timeRange) { ... }); https://codereview.chromium.org/1336373002/diff/60001/perf_insights/perf_insi... perf_insights/perf_insights/timeline_based_measurement/rendering_stats.html:257: getRefreshPeriodFromSurfaceFlingerProcess_: function( nit: s/getRefresh/refresh https://codereview.chromium.org/1336373002/diff/60001/perf_insights/perf_insi... perf_insights/perf_insights/timeline_based_measurement/rendering_stats.html:263: } nit: remove {}'s nit: event.args.data.refreshPeriod https://codereview.chromium.org/1336373002/diff/60001/perf_insights/perf_insi... perf_insights/perf_insights/timeline_based_measurement/rendering_stats.html:272: getLatencyEvents(rendererProcess, timeRange)); nit: indent 4 on wrap https://codereview.chromium.org/1336373002/diff/60001/perf_insights/perf_insi... perf_insights/perf_insights/timeline_based_measurement/rendering_stats.html:273: var eventLatencies = computeEventLatencies(latencyEvents); nit: add blank line before https://codereview.chromium.org/1336373002/diff/60001/perf_insights/perf_insi... perf_insights/perf_insights/timeline_based_measurement/rendering_stats.html:317: this.frameTimestamps_[this.frameTimestamps_.length - 1]; nit: indent 4 https://codereview.chromium.org/1336373002/diff/60001/perf_insights/perf_insi... perf_insights/perf_insights/timeline_based_measurement/rendering_stats.html:321: lastFrameTimestamps[lastFrameTimestamps.length - 1] - nit: indenting https://codereview.chromium.org/1336373002/diff/60001/perf_insights/perf_insi... perf_insights/perf_insights/timeline_based_measurement/rendering_stats.html:324: }, nit: outdent 4 https://codereview.chromium.org/1336373002/diff/60001/perf_insights/perf_insi... perf_insights/perf_insights/timeline_based_measurement/rendering_stats.html:336: process, timeRange) { nit: indenting (does this need to wrap?) https://codereview.chromium.org/1336373002/diff/60001/perf_insights/perf_insi... perf_insights/perf_insights/timeline_based_measurement/rendering_stats.html:343: 'Calculating approximatedPixelPercentages not possible ' + This isn't going to be a complete set of errors, it will just be the error of the type that's seen last as it overwrites, yea? Why not just console.error and skip the logging? https://codereview.chromium.org/1336373002/diff/60001/perf_insights/perf_insi... perf_insights/perf_insights/timeline_based_measurement/rendering_stats.html:391: this.errors['frameQueueingDurations'] = Why isn't this a const like the others? https://codereview.chromium.org/1336373002/diff/60001/perf_insights/perf_insi... File perf_insights/perf_insights/timeline_based_measurement/rendering_stats_test.html (right): https://codereview.chromium.org/1336373002/diff/60001/perf_insights/perf_insi... perf_insights/perf_insights/timeline_based_measurement/rendering_stats_test.html:35: nit: remove blank line https://codereview.chromium.org/1336373002/diff/60001/perf_insights/perf_insi... perf_insights/perf_insights/timeline_based_measurement/rendering_stats_test.html:55: opt_low = 0.1; opt_low = opt_low || 0.1; (although this doesn't allow the values to be 0, not sure if that's an issue) opt_high = opt_high || 1.0; opt_delta = opt_delta || 0.4; https://codereview.chromium.org/1336373002/diff/60001/perf_insights/perf_insi... perf_insights/perf_insights/timeline_based_measurement/rendering_stats_test.html:99: // Create randonm data and timestap for impl thread rendering stats. nit: s/randonm/random nit: s/timestap/timestamp https://codereview.chromium.org/1336373002/diff/60001/perf_insights/perf_insi... perf_insights/perf_insights/timeline_based_measurement/rendering_stats_test.html:105: nit: remove blank line https://codereview.chromium.org/1336373002/diff/60001/perf_insights/perf_insi... perf_insights/perf_insights/timeline_based_measurement/rendering_stats_test.html:124: if (data['frame_count'] === 1) { if (data.frame_count !== 1) return; https://codereview.chromium.org/1336373002/diff/60001/perf_insights/perf_insi... perf_insights/perf_insights/timeline_based_measurement/rendering_stats_test.html:145: * first_frame: Is this the first frame within the bounds of an action? Do we know the answer to this? https://codereview.chromium.org/1336373002/diff/60001/tracing/tracing/base/ut... File tracing/tracing/base/utils.html (right): https://codereview.chromium.org/1336373002/diff/60001/tracing/tracing/base/ut... tracing/tracing/base/utils.html:58: } nit: indenting
https://codereview.chromium.org/1336373002/diff/60001/perf_insights/perf_insi... File perf_insights/perf_insights/timeline_based_measurement/rendering_frame.html (right): https://codereview.chromium.org/1336373002/diff/60001/perf_insights/perf_insi... perf_insights/perf_insights/timeline_based_measurement/rendering_frame.html:86: var err = new Error( What is the purpose of this change? https://codereview.chromium.org/1336373002/diff/60001/tracing/tracing/base/ma... File tracing/tracing/base/math.html (right): https://codereview.chromium.org/1336373002/diff/60001/tracing/tracing/base/ma... tracing/tracing/base/math.html:15: opt_ndigits = opt_ndigits || 1; This prevents setting opt_ndigits = 0. I don't know if that's an expected use case, or if you'd expect callers to just use Math.round().
If this implementation is intended to provide a seamless transition from the existing implementation, then it should not switch to using the RAILIRFinder, because it is radically different. I'd have to sit down with somebody for a while to see which implementation is smarter, and in which cases. Even if this implementation is not intended to provide a seamless transition, I'd still want to fix Load IRs at a bare minimum before recommending switching any analysis that is relied upon over to the RAILIRFinder. But yes, eventually, I'd love to have the RAILIRFinder be smart enough that the perf dashboard relies on it. |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
