Create a mapper to compute browser and renderer startup durations.

Create a mapper to compute browser and renderer startup durations.

Committed: https://chromium.googlesource.com/external/github.com/catapult-project/catapult/+/1aa2364c5416f712b12827629c170887845127a5

[beaudoin, 2015-08-14 03:26:54]

beaudoin@chromium.org changed reviewers:
+ nduca@chromium.org

[beaudoin, 2015-08-14 03:26:54]

Nat: So I hacked this together really quickly. It uses the "name" argument I
added to IRs in the previous CL to identify startup Load IRs, matches them with
their associatedEvents, figure out if these events are in the browser or
renderer processes, and computes startup duration from these processes. The
overall CL is really simple and I verified that it works on the startup trace in
measuremt.

Let me know if I should that code to the examples. If yes I'll clean it up and
add some tests.

[dsinclair, 2015-08-14 13:18:33]

dsinclair@chromium.org changed reviewers:
+ dsinclair@chromium.org

[dsinclair, 2015-08-14 13:18:34]

https://codereview.chromium.org/1290323003/diff/1/perf_insights/perf_insights...
File perf_insights/perf_insights_examples/map_startup_info.html (right):

https://codereview.chromium.org/1290323003/diff/1/perf_insights/perf_insights...
perf_insights/perf_insights_examples/map_startup_info.html:15: }
Can you add an explict return undefined; to the end of this please.

https://codereview.chromium.org/1290323003/diff/1/perf_insights/perf_insights...
perf_insights/perf_insights_examples/map_startup_info.html:23: var
browser_begin;
We usually use start instead of begin I believe.

https://codereview.chromium.org/1290323003/diff/1/perf_insights/perf_insights...
perf_insights/perf_insights_examples/map_startup_info.html:26: var renderer_end;
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.

https://codereview.chromium.org/1290323003/diff/1/perf_insights/perf_insights...
perf_insights/perf_insights_examples/map_startup_info.html:30: if (processName
=== 'Browser') {
There is a tr.e.audits.ChromeBrowserHelper.isBrowserProcess(process) and
tr.e.audits.ChromeRendererHelper.isRenderProcess(process) which would possibly
make this clearer.

https://codereview.chromium.org/1290323003/diff/1/perf_insights/perf_insights...
perf_insights/perf_insights_examples/map_startup_info.html:32: var end =
event.start + event.duration;
This is the same for both if statements so can be pulled out.

https://codereview.chromium.org/1290323003/diff/1/perf_insights/perf_insights...
perf_insights/perf_insights_examples/map_startup_info.html:57: mapProcessCount:
mapProcessCount
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)

https://codereview.chromium.org/1290323003/diff/1/tracing/tracing/extras/rail...
File tracing/tracing/extras/rail/rail_ir_finder.html (right):

https://codereview.chromium.org/1290323003/diff/1/tracing/tracing/extras/rail...
tracing/tracing/extras/rail/rail_ir_finder.html:139: irs.forEach(function(ir) {
This is from a different CL, should it be removed from this one to reduce
confusion?

[beaudoin, 2015-08-14 20:28:54]

https://codereview.chromium.org/1290323003/diff/1/perf_insights/perf_insights...
File perf_insights/perf_insights_examples/map_startup_info.html (right):

https://codereview.chromium.org/1290323003/diff/1/perf_insights/perf_insights...
perf_insights/perf_insights_examples/map_startup_info.html:15: }
On 2015/08/14 13:18:33, dsinclair wrote:
> Can you add an explict return undefined; to the end of this please.

Done.

https://codereview.chromium.org/1290323003/diff/1/perf_insights/perf_insights...
perf_insights/perf_insights_examples/map_startup_info.html:23: var
browser_begin;
On 2015/08/14 13:18:33, dsinclair wrote:
> We usually use start instead of begin I believe.

Done.

https://codereview.chromium.org/1290323003/diff/1/perf_insights/perf_insights...
perf_insights/perf_insights_examples/map_startup_info.html:26: var renderer_end;
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.

https://codereview.chromium.org/1290323003/diff/1/perf_insights/perf_insights...
perf_insights/perf_insights_examples/map_startup_info.html:30: if (processName
=== 'Browser') {
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.

https://codereview.chromium.org/1290323003/diff/1/perf_insights/perf_insights...
perf_insights/perf_insights_examples/map_startup_info.html:32: var end =
event.start + event.duration;
On 2015/08/14 13:18:33, dsinclair wrote:
> This is the same for both if statements so can be pulled out.

Done.

https://codereview.chromium.org/1290323003/diff/1/perf_insights/perf_insights...
perf_insights/perf_insights_examples/map_startup_info.html:57: mapProcessCount:
mapProcessCount
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?

https://codereview.chromium.org/1290323003/diff/1/tracing/tracing/extras/rail...
File tracing/tracing/extras/rail/rail_ir_finder.html (right):

https://codereview.chromium.org/1290323003/diff/1/tracing/tracing/extras/rail...
tracing/tracing/extras/rail/rail_ir_finder.html:139: irs.forEach(function(ir) {
On 2015/08/14 13:18:34, dsinclair wrote:
> This is from a different CL, should it be removed from this one to reduce
> confusion?

Yup, it's gone.

This was a git branch on a git branch and there's a way to set the base CL so as
not to confuse the codereview tool, but I always forget how to do it. :)

[nduca, 2015-08-14 20:29:32]

some nits

https://codereview.chromium.org/1290323003/diff/20001/perf_insights/perf_insi...
File perf_insights/perf_insights_examples/map_startup_info.html (right):

https://codereview.chromium.org/1290323003/diff/20001/perf_insights/perf_insi...
perf_insights/perf_insights_examples/map_startup_info.html:12: function
getProcessName(event) {
driveby thought, nonblocking: should we be making this easier? i notice this is
suprisingly hard to do in the model. partly becuase not all events have
processes, but still... everyone wants to do this.

Maybe ThreadSlice should have a process getter and everyone else would have
undefined implicitly?

https://codereview.chromium.org/1290323003/diff/20001/perf_insights/perf_insi...
perf_insights/perf_insights_examples/map_startup_info.html:19: return ir
instanceof tr.e.rail.LoadInteractionRecord &&
can you have the title of the LoadInteractionRecord be startup? we typically
avoid looking inside args conditionally --- args the idea is you use for generic
processing ("group the args by name"), but if you need a specific part out of
args, you typically want  to promote the logic to a field.

so now that i see this code, maybe we should modify your ir finder so that it
creates subclasses LoadInteractionRecord called BrowserStartupInteractionRecord
that have a title of "startup" (as opposed to Loading) and then you could
instanceof here

https://codereview.chromium.org/1290323003/diff/20001/perf_insights/perf_insi...
perf_insights/perf_insights_examples/map_startup_info.html:29: var processName =
getProcessName(event);
can you somehow use chrome model helper here?

e.g. make a helper before your loop, then have helper.isBrowser(process)?

https://codereview.chromium.org/1290323003/diff/20001/perf_insights/perf_insi...
perf_insights/perf_insights_examples/map_startup_info.html:31: browser_begin =
Math.min(event.start, browser_begin || event.start);
can you use tr.b.Range for this?

https://codereview.chromium.org/1290323003/diff/20001/perf_insights/perf_insi...
perf_insights/perf_insights_examples/map_startup_info.html:44: if (browser_begin
!== undefined && browser_end !== undefined)
if you used ranges, you'd say if (range.isEmpty) range.range

https://codereview.chromium.org/1290323003/diff/20001/perf_insights/perf_insi...
perf_insights/perf_insights_examples/map_startup_info.html:57: mapProcessCount:
mapProcessCount
name the function differently plox?

[nduca, 2015-08-14 20:36:59]

cna you add a smoketest?

the reason to have a smoketest is because i keep changing the mapper definition
api  so i wanna make sure this doesn't rot.

doesn't have to be an amazing test, you could use chrome_test_utils.html to make
a newChromeModel, then hand craft an browser ir and whatever else you need so
that your paths trigger.

[beaudoin, 2015-08-14 22:10:42]

Answered nat, added a test (although I don't think it's a smoke test?) and now
emit a SkipValue if there's nothing to report.

https://codereview.chromium.org/1290323003/diff/20001/perf_insights/perf_insi...
File perf_insights/perf_insights_examples/map_startup_info.html (right):

https://codereview.chromium.org/1290323003/diff/20001/perf_insights/perf_insi...
perf_insights/perf_insights_examples/map_startup_info.html:12: function
getProcessName(event) {
On 2015/08/14 20:29:31, nduca wrote:
> driveby thought, nonblocking: should we be making this easier? i notice this
is
> suprisingly hard to do in the model. partly becuase not all events have
> processes, but still... everyone wants to do this.
> 
> Maybe ThreadSlice should have a process getter and everyone else would have
> undefined implicitly?

Did something. It's a bit far reaching but I think I like it. What do you think?

https://codereview.chromium.org/1290323003/diff/20001/perf_insights/perf_insi...
perf_insights/perf_insights_examples/map_startup_info.html:19: return ir
instanceof tr.e.rail.LoadInteractionRecord &&
On 2015/08/14 20:29:31, nduca wrote:
> can you have the title of the LoadInteractionRecord be startup? we typically
> avoid looking inside args conditionally --- args the idea is you use for
generic
> processing ("group the args by name"), but if you need a specific part out of
> args, you typically want  to promote the logic to a field.
> 
> so now that i see this code, maybe we should modify your ir finder so that it
> creates subclasses LoadInteractionRecord called
BrowserStartupInteractionRecord
> that have a title of "startup" (as opposed to Loading) and then you could
> instanceof here 

I now have a 'name' field on the all IRs. This was the other CL that just
landed. Is that better?

https://codereview.chromium.org/1290323003/diff/20001/perf_insights/perf_insi...
perf_insights/perf_insights_examples/map_startup_info.html:29: var processName =
getProcessName(event);
On 2015/08/14 20:29:31, nduca wrote:
> can you somehow use chrome model helper here?
> 
> e.g. make a helper before your loop, then have helper.isBrowser(process)?

Done.

https://codereview.chromium.org/1290323003/diff/20001/perf_insights/perf_insi...
perf_insights/perf_insights_examples/map_startup_info.html:31: browser_begin =
Math.min(event.start, browser_begin || event.start);
On 2015/08/14 20:29:32, nduca wrote:
> can you use tr.b.Range for this?

OMG! I SEE THE LIGHT!

Done.

https://codereview.chromium.org/1290323003/diff/20001/perf_insights/perf_insi...
perf_insights/perf_insights_examples/map_startup_info.html:44: if (browser_begin
!== undefined && browser_end !== undefined)
On 2015/08/14 20:29:31, nduca wrote:
> if you used ranges, you'd say if (range.isEmpty) range.range
> 

Problem went away...

https://codereview.chromium.org/1290323003/diff/20001/perf_insights/perf_insi...
perf_insights/perf_insights_examples/map_startup_info.html:57: mapProcessCount:
mapProcessCount
On 2015/08/14 20:29:32, nduca wrote:
> name the function differently plox?

Done.

[nduca, 2015-08-14 22:28:49]

lgtm with some nits

https://codereview.chromium.org/1290323003/diff/60001/perf_insights/perf_insi...
File perf_insights/perf_insights_examples/map_startup_info.html (right):

https://codereview.chromium.org/1290323003/diff/60001/perf_insights/perf_insi...
perf_insights/perf_insights_examples/map_startup_info.html:29: if
(helper.isBrowserProcess(process))
nit: this looks like you're referring to an on an isntance of the helper.


I think you wanna put up at your exportto line

var ChromeBrowserHelper = blahblah.ChromeBrowserHelpr
then on this line you'd say ChromeBrowserHelper.isBrowserProcess(...)

https://codereview.chromium.org/1290323003/diff/60001/perf_insights/perf_insi...
perf_insights/perf_insights_examples/map_startup_info.html:37: if
(!browser_startup.isEmpty)
i think it would be easier on the data processing side to form the dict all the
time.

the nice thing about the results object is it handles this properly...

https://codereview.chromium.org/1290323003/diff/60001/perf_insights/perf_insi...
perf_insights/perf_insights_examples/map_startup_info.html:43:
results.addValue(new pi.v.SkipValue(run_info));
you'll wanna say what result name was skipped, e.g  new
SkipValue('startup_info');

https://codereview.chromium.org/1290323003/diff/60001/tracing/tracing/extras/...
File tracing/tracing/extras/chrome/chrome_browser_helper.html (right):

https://codereview.chromium.org/1290323003/diff/60001/tracing/tracing/extras/...
tracing/tracing/extras/chrome/chrome_browser_helper.html:26:
ChromeBrowserHelper.isRendererProcess = function(process) {
you'll have to use ChromeRendererHelper for this. Its not actually as easy as
you think and we need the logic centralized [content shell etc have various non
trivial tricks that cause things to get named whackily]

[beaudoin, 2015-08-14 22:55:46]

The CQ bit was checked by beaudoin@chromium.org

[beaudoin, 2015-08-14 22:55:46]

The patchset sent to the CQ was uploaded after l-g-t-m from nduca@chromium.org
Link to the patchset: https://codereview.chromium.org/1290323003/#ps80001
(title: "Answered nits.")

[commit-bot: I haz the power, 2015-08-14 22:56:02]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1290323003/80001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1290323003/80001

[commit-bot: I haz the power, 2015-08-14 23:03:30]

Committed patchset #5 (id:80001) as
https://chromium.googlesource.com/external/github.com/catapult-project/catapu...

[beaudoin, 2015-08-18 16:16:54]

https://codereview.chromium.org/1290323003/diff/60001/perf_insights/perf_insi...
File perf_insights/perf_insights_examples/map_startup_info.html (right):

https://codereview.chromium.org/1290323003/diff/60001/perf_insights/perf_insi...
perf_insights/perf_insights_examples/map_startup_info.html:29: if
(helper.isBrowserProcess(process))
On 2015/08/14 22:28:49, nduca wrote:
> nit: this looks like you're referring to an on an isntance of the helper.
> 
> 
> I think you wanna put up at your exportto line
> 
> var ChromeBrowserHelper = blahblah.ChromeBrowserHelpr
> then on this line you'd say ChromeBrowserHelper.isBrowserProcess(...)

Done.

https://codereview.chromium.org/1290323003/diff/60001/perf_insights/perf_insi...
perf_insights/perf_insights_examples/map_startup_info.html:37: if
(!browser_startup.isEmpty)
On 2015/08/14 22:28:49, nduca wrote:
> i think it would be easier on the data processing side to form the dict all
the
> time.
> 
> the nice thing about the results object is it handles this properly...

Done.

https://codereview.chromium.org/1290323003/diff/60001/perf_insights/perf_insi...
perf_insights/perf_insights_examples/map_startup_info.html:43:
results.addValue(new pi.v.SkipValue(run_info));
On 2015/08/14 22:28:49, nduca wrote:
> you'll wanna say what result name was skipped, e.g  new
> SkipValue('startup_info');

Done.

https://codereview.chromium.org/1290323003/diff/60001/tracing/tracing/extras/...
File tracing/tracing/extras/chrome/chrome_browser_helper.html (right):

https://codereview.chromium.org/1290323003/diff/60001/tracing/tracing/extras/...
tracing/tracing/extras/chrome/chrome_browser_helper.html:26:
ChromeBrowserHelper.isRendererProcess = function(process) {
On 2015/08/14 22:28:49, nduca wrote:
> you'll have to use ChromeRendererHelper for this. Its not actually as easy as
> you think and we need the logic centralized [content shell etc have various
non
> trivial tricks that cause things to get named whackily]

Done.