perf_insights/perf_insights_examples/map_startup_info.html - Issue 1290323003: Create a mapper to compute browser and renderer startup durations.

Side by Side Diff: perf_insights/perf_insights_examples/map_startup_info.html

Issue 1290323003: Create a mapper to compute browser and renderer startup durations. (Closed) Base URL: https://github.com/catapult-project/catapult.git@master

Patch Set: Created 5 years, 4 months ago

Use n/p to move between diff chunks; N/P to move between comments. Draft comments are only viewable by you.

Jump to:

View unified diff | Download patch

OLD	NEW
(Empty)
	1 <!DOCTYPE HTML>

	2 <!--

	3 Copyright (c) 2015 The Chromium Authors. All rights reserved.

	4 Use of this source code is governed by a BSD-style license that can be

	5 found in the LICENSE file.

	6 -->

	7 <link rel="import" href="/perf_insights/value/value.html">

	8 <link rel="import" href="/perf_insights/map_function.html">

	9

	10 <script>

	11 tr.exportTo('pie', function() {

	12 function getProcessName(event) {

	13 if (event && event.parentContainer && event.parentContainer.parent)

	14 return event.parentContainer.parent.name;

	15 }
	dsinclair 2015/08/14 13:18:33 Can you add an explict return undefined; to the en Can you add an explict return undefined; to the end of this please. beaudoin 2015/08/14 20:28:54 Done. Show quoted text On 2015/08/14 13:18:33, dsinclair wrote: > Can you add an explict return undefined; to the end of this please. Done.
	16

	17 function mapProcessCount(results, run_info, model) {

	18 var startupIRs = model.interaction_records.filter(function(ir) {

	19 return ir instanceof tr.e.rail.LoadInteractionRecord &&

	20 ir.args['name'] && ir.args['name'] === 'Startup';

	21 });

	22 // Find the Startup IR in the browser process, there should only be 1.

	23 var browser_begin;
	dsinclair 2015/08/14 13:18:33 We usually use start instead of begin I believe. We usually use start instead of begin I believe. beaudoin 2015/08/14 20:28:54 Done. Show quoted text On 2015/08/14 13:18:33, dsinclair wrote: > We usually use start instead of begin I believe. Done.
	24 var browser_end;

	25 var renderer_begin;

	26 var renderer_end;
	dsinclair 2015/08/14 13:18:33 Should these default to 0, then you don't need the Should these default to 0, then you don't need the undefined checks below. If we have no values, then the startup time was 0. beaudoin 2015/08/14 20:28:54 Well, I can't really initialize them to 0 because Show quoted text On 2015/08/14 13:18:33, dsinclair wrote: > Should these default to 0, then you don't need the undefined checks below. If > we have no values, then the startup time was 0. Well, I can't really initialize them to 0 because then my max/min will not work below. (When it's undefined, I want to take the start of the first event I encounter. I've added a comment and made the undefined explicit so that nobody springs that trap in the future. LMK if it's OK.
	27 startupIRs.forEach(function(ir) {

	28 ir.associatedEvents.toArray().forEach(function(event) {

	29 var processName = getProcessName(event);

	30 if (processName === 'Browser') {
	dsinclair 2015/08/14 13:18:33 There is a tr.e.audits.ChromeBrowserHelper.isBrows There is a tr.e.audits.ChromeBrowserHelper.isBrowserProcess(process) and tr.e.audits.ChromeRendererHelper.isRenderProcess(process) which would possibly make this clearer. beaudoin 2015/08/14 20:28:54 Done. Do you know of another helper method to fin Show quoted text On 2015/08/14 13:18:33, dsinclair wrote: > There is a tr.e.audits.ChromeBrowserHelper.isBrowserProcess(process) and > tr.e.audits.ChromeRendererHelper.isRenderProcess(process) which would possibly > make this clearer. Done. Do you know of another helper method to find the process of an event? Sounds like this should be in some helper class too, although not necessarily Chrome-specific.
	31 browser_begin = Math.min(event.start, browser_begin \|\| event.start);

	32 var end = event.start + event.duration;
	dsinclair 2015/08/14 13:18:33 This is the same for both if statements so can be This is the same for both if statements so can be pulled out. beaudoin 2015/08/14 20:28:54 Done. Show quoted text On 2015/08/14 13:18:33, dsinclair wrote: > This is the same for both if statements so can be pulled out. Done.
	33 browser_end = Math.max(end, browser_end \|\| end);

	34 }

	35 if (processName === 'Renderer') {

	36 renderer_begin = Math.min(event.start, renderer_begin \|\| event.start);

	37 var end = event.start + event.duration;

	38 renderer_end = Math.max(end, renderer_end \|\| end);

	39 }

	40 });

	41 });

	42

	43 var startupInfo = {};

	44 if (browser_begin !== undefined && browser_end !== undefined)

	45 startupInfo['browserStartupDuration'] = browser_end - browser_begin;

	46 if (renderer_begin !== undefined && renderer_end !== undefined)

	47 startupInfo['rendererStartupDuration'] = renderer_end - renderer_begin;

	48

	49 results.addValue(new pi.v.DictValue(

	50 run_info,

	51 'startup_info', startupInfo));

	52 }

	53

	54 pi.MapFunction.register(mapProcessCount);

	55

	56 return {

	57 mapProcessCount: mapProcessCount
	dsinclair 2015/08/14 13:18:33 This is going to export mapProcessCount into the p This is going to export mapProcessCount into the pie namespace which seems pretty general and high level. Does it need to be exported? Do we call it or is it just for the register above (which doesn't need the exporting) beaudoin 2015/08/14 20:28:54 Removed it and it still works. Nat: I notice you Show quoted text On 2015/08/14 13:18:33, dsinclair wrote: > This is going to export mapProcessCount into the pie namespace which seems > pretty general and high level. > > Does it need to be exported? Do we call it or is it just for the register above > (which doesn't need the exporting) Removed it and it still works. Nat: I notice you use this pattern in the other mappers. Should we correct them?
	58 };

	59 });

	60 </script>

OLD	NEW

« no previous file with comments | « no previous file | tracing/tracing/extras/rail/rail_ir_finder.html » ('j') | tracing/tracing/extras/rail/rail_ir_finder.html » ('J')