Switch LatencyInfo struct to use a map.

Switch LatencyInfo struct to use a map.

This allows us to have multiple instances of a specific component, for example each renderer will have a separate main thread and compositor.

BUG=155367
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=194687

[nduca, 2013-04-09 23:15:08]

https://codereview.chromium.org/13874002/diff/8001/cc/debug/latency_info.cc
File cc/debug/latency_info.cc (right):

https://codereview.chromium.org/13874002/diff/8001/cc/debug/latency_info.cc#n...
cc/debug/latency_info.cc:19: int64 id, int64 number) {
lgtm

numer-> component_sequence_number?


lets evolve this api as we add consumers... we really want random-new-person to
be able to grok this.

[jbauman, 2013-04-09 23:55:35]

cevans@, could you do an OWNERS review for the (hopefully trivial) IPC change?

[jbauman, 2013-04-15 16:41:52]

Actually, I decided to modify this to add a timestamp that a consumer of this
structure should be able to use to calculate latency without having to keep
track of the start time and sequence number itself. Does this make sense?

[nduca, 2013-04-15 16:49:23]

Yeah that makes sense. Some nits.

https://codereview.chromium.org/13874002/diff/20001/cc/debug/latency_info.h
File cc/debug/latency_info.h (right):

https://codereview.chromium.org/13874002/diff/20001/cc/debug/latency_info.h#n...
cc/debug/latency_info.h:25: struct ComponentInfo {
This should match the typename you've got up higher. E.g. thats enum be a
LatencyComponentType, and this be LatencyComponent.

Something like that.

https://codereview.chromium.org/13874002/diff/20001/cc/debug/latency_info.h#n...
cc/debug/latency_info.h:41: LatencyMap latency_numbers;
this name should match whatever the type name. latency_components?

[jbauman, 2013-04-15 16:58:19]

Ok, changed the names.

On 2013/04/15 16:49:23, nduca wrote:
> Yeah that makes sense. Some nits.
> 
> https://codereview.chromium.org/13874002/diff/20001/cc/debug/latency_info.h
> File cc/debug/latency_info.h (right):
> 
>
https://codereview.chromium.org/13874002/diff/20001/cc/debug/latency_info.h#n...
> cc/debug/latency_info.h:25: struct ComponentInfo {
> This should match the typename you've got up higher. E.g. thats enum be a
> LatencyComponentType, and this be LatencyComponent.
> 
> Something like that.
> 
>
https://codereview.chromium.org/13874002/diff/20001/cc/debug/latency_info.h#n...
> cc/debug/latency_info.h:41: LatencyMap latency_numbers;
> this name should match whatever the type name. latency_components?

[nduca, 2013-04-15 17:03:28]

old lgtm from #2 still holds, needless to say. This is good stuff.

[jbauman, 2013-04-15 17:06:43]

cevans@, would you mind reviewing the IPC change to see if it's safe? Hopefully
should be trivial.

[Chris Evans, 2013-04-15 19:33:35]

On 2013/04/15 17:06:43, jbauman wrote:
> cevans@, would you mind reviewing the IPC change to see if it's safe?
Hopefully
> should be trivial.

Sure! Quick question: who sends this message and who receives it (in terms of
process types). It wasn't immediately obvious to me.

[jbauman, 2013-04-15 19:39:33]

On 2013/04/15 19:33:35, Chris Evans wrote:
> On 2013/04/15 17:06:43, jbauman wrote:
> > cevans@, would you mind reviewing the IPC change to see if it's safe?
> Hopefully
> > should be trivial.
> 
> Sure! Quick question: who sends this message and who receives it (in terms of
> process types). It wasn't immediately obvious to me.

Everyone sends and receives it, pretty much. Browser sends to renderer, renderer
sends to GPU process, GPU process sends to browser (and we're maybe adding other
paths later).

[Chris Evans, 2013-04-15 21:06:25]

https://codereview.chromium.org/13874002/diff/23002/cc/debug/latency_info.cc
File cc/debug/latency_info.cc (right):

https://codereview.chromium.org/13874002/diff/23002/cc/debug/latency_info.cc#...
cc/debug/latency_info.cc:43: (event_count + f->second.event_count);
Seems like there's opportunity to be more careful here, given that LatencyInfo's
are passing from untrusted to more trusted processes?

e.g. nothing super serious but a negative "event count" could presumable lead to
a div-by-zero condition here?

https://codereview.chromium.org/13874002/diff/23002/cc/debug/latency_info.h
File cc/debug/latency_info.h (right):

https://codereview.chromium.org/13874002/diff/23002/cc/debug/latency_info.h#n...
cc/debug/latency_info.h:33: int event_count;
A negative event_count would probably be non-sensical? Would uint32_t be a
better type?

[jbauman, 2013-04-16 17:49:50]

Good idea; I switched it to uint32_t and added a check (to protect against
overflow).

On 2013/04/15 21:06:25, Chris Evans wrote:
> https://codereview.chromium.org/13874002/diff/23002/cc/debug/latency_info.cc
> File cc/debug/latency_info.cc (right):
> 
>
https://codereview.chromium.org/13874002/diff/23002/cc/debug/latency_info.cc#...
> cc/debug/latency_info.cc:43: (event_count + f->second.event_count);
> Seems like there's opportunity to be more careful here, given that
LatencyInfo's
> are passing from untrusted to more trusted processes?
> 
> e.g. nothing super serious but a negative "event count" could presumable lead
to
> a div-by-zero condition here?
> 
> https://codereview.chromium.org/13874002/diff/23002/cc/debug/latency_info.h
> File cc/debug/latency_info.h (right):
> 
>
https://codereview.chromium.org/13874002/diff/23002/cc/debug/latency_info.h#n...
> cc/debug/latency_info.h:33: int event_count;
> A negative event_count would probably be non-sensical? Would uint32_t be a
> better type?

[danakj, 2013-04-16 18:41:44]

On Tue, Apr 16, 2013 at 1:49 PM, <jbauman@chromium.org> wrote:

> Good idea; I switched it to uint32_t and added a check (to protect against
> overflow).


nit: base/basictypes.h defines uint32 (as opposed to uint32_t).

[Chris Evans, 2013-04-17 00:54:59]

LGTM with nit

https://codereview.chromium.org/13874002/diff/30001/cc/debug/latency_info.cc
File cc/debug/latency_info.cc (right):

https://codereview.chromium.org/13874002/diff/30001/cc/debug/latency_info.cc#...
cc/debug/latency_info.cc:37: f->second.sequence_number);
Nit: can avoid some minor duplication by declaring here,
uint32_t new_count = event_count + f->second.event_count;

and then using it in three places below.

[jbauman, 2013-04-17 21:39:06]

Committed patchset #10 manually as r194687 (presubmit successful).