telemetry: Add thread time to telmetry timeline.

telemetry: Add thread time to telmetry timeline.

R=nduca@chromium.org,tonyg@chromium.org,fmeawad@chromium.org
BUG=

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

[ernstm, 2013-12-03 01:15:21]

PTAL.

[tonyg, 2013-12-03 04:07:24]

I'm excited to see this happen, but am not a great reviewer for this.

Tim is a good reviewer for stuff in this directory.

[nduca, 2013-12-03 07:18:55]

lgtm with minor fiddly comments

https://codereview.chromium.org/100073003/diff/1/tools/perf/metrics/rendering...
File tools/perf/metrics/rendering_stats_unittest.py (right):

https://codereview.chromium.org/100073003/diff/1/tools/perf/metrics/rendering...
tools/perf/metrics/rendering_stats_unittest.py:63: timestamp, 0.0, None, None,
{'data': data})
here and elsewhere, after the timestamp parameter, can you please use
explicitly-named args here so its clear what each argument is?

timestamp, blah=0.0, blah2=None, blah3=None, args={'data'....

https://codereview.chromium.org/100073003/diff/1/tools/telemetry/telemetry/co...
File tools/telemetry/telemetry/core/timeline/event.py (right):

https://codereview.chromium.org/100073003/diff/1/tools/telemetry/telemetry/co...
tools/telemetry/telemetry/core/timeline/event.py:6: """Represents a timeline
event."""
can you put a docstring here to indicate that thread-start, duration and end are
none sometimes?

https://codereview.chromium.org/100073003/diff/1/tools/telemetry/telemetry/co...
tools/telemetry/telemetry/core/timeline/event.py:22: def thread_end(self):
docstring ehre too

https://codereview.chromium.org/100073003/diff/1/tools/telemetry/telemetry/co...
File tools/telemetry/telemetry/core/timeline/model.py (right):

https://codereview.chromium.org/100073003/diff/1/tools/telemetry/telemetry/co...
tools/telemetry/telemetry/core/timeline/model.py:43: self._thread_bounds =
bounds.Bounds()
i think the name here is very ... funky. can you think of another name?
bounds_in_thread_time? thread_time_bounds?

otherwise, might be read as bounds on a thread object.

https://codereview.chromium.org/100073003/diff/1/tools/telemetry/telemetry/co...
File tools/telemetry/telemetry/core/timeline/slice.py (right):

https://codereview.chromium.org/100073003/diff/1/tools/telemetry/telemetry/co...
tools/telemetry/telemetry/core/timeline/slice.py:42: 
unit test for this new function plz

[fmeawad, 2013-12-03 17:34:17]

https://codereview.chromium.org/100073003/diff/1/tools/telemetry/telemetry/co...
File tools/telemetry/telemetry/core/timeline/model.py (right):

https://codereview.chromium.org/100073003/diff/1/tools/telemetry/telemetry/co...
tools/telemetry/telemetry/core/timeline/model.py:106: thread_shift_amount =
self._thread_bounds.min
Thread timestamps are reported from the time the thread started, shifting them
may create a loss of information.
On the other hand, thread timestamps are rarely used, thread duration is the one
usually used (shifting has no effect then).

[ernstm, 2013-12-03 20:55:53]

https://codereview.chromium.org/100073003/diff/1/tools/perf/metrics/rendering...
File tools/perf/metrics/rendering_stats_unittest.py (right):

https://codereview.chromium.org/100073003/diff/1/tools/perf/metrics/rendering...
tools/perf/metrics/rendering_stats_unittest.py:63: timestamp, 0.0, None, None,
{'data': data})
On 2013/12/03 07:18:55, nduca wrote:
> here and elsewhere, after the timestamp parameter, can you please use
> explicitly-named args here so its clear what each argument is?
> 
> timestamp, blah=0.0, blah2=None, blah3=None, args={'data'....
> 

Done.

https://codereview.chromium.org/100073003/diff/1/tools/telemetry/telemetry/co...
File tools/telemetry/telemetry/core/timeline/event.py (right):

https://codereview.chromium.org/100073003/diff/1/tools/telemetry/telemetry/co...
tools/telemetry/telemetry/core/timeline/event.py:6: """Represents a timeline
event."""
On 2013/12/03 07:18:55, nduca wrote:
> can you put a docstring here to indicate that thread-start, duration and end
are
> none sometimes?

Done.

https://codereview.chromium.org/100073003/diff/1/tools/telemetry/telemetry/co...
tools/telemetry/telemetry/core/timeline/event.py:22: def thread_end(self):
On 2013/12/03 07:18:55, nduca wrote:
> docstring ehre too

Done.

https://codereview.chromium.org/100073003/diff/1/tools/telemetry/telemetry/co...
File tools/telemetry/telemetry/core/timeline/model.py (right):

https://codereview.chromium.org/100073003/diff/1/tools/telemetry/telemetry/co...
tools/telemetry/telemetry/core/timeline/model.py:43: self._thread_bounds =
bounds.Bounds()
On 2013/12/03 07:18:55, nduca wrote:
> i think the name here is very ... funky. can you think of another name?
> bounds_in_thread_time? thread_time_bounds?
> 
> otherwise, might be read as bounds on a thread object.

Done: thread_time_bounds

https://codereview.chromium.org/100073003/diff/1/tools/telemetry/telemetry/co...
tools/telemetry/telemetry/core/timeline/model.py:106: thread_shift_amount =
self._thread_bounds.min
On 2013/12/03 17:34:17, fmeawad-cr wrote:
> Thread timestamps are reported from the time the thread started, shifting them
> may create a loss of information.
> On the other hand, thread timestamps are rarely used, thread duration is the
one
> usually used (shifting has no effect then).

Maybe we keep it as is (which is consistent with the other timestamps), until we
find a use case that requires non-shifted thread timestamps?

https://codereview.chromium.org/100073003/diff/1/tools/telemetry/telemetry/co...
File tools/telemetry/telemetry/core/timeline/slice.py (right):

https://codereview.chromium.org/100073003/diff/1/tools/telemetry/telemetry/co...
tools/telemetry/telemetry/core/timeline/slice.py:42: 
On 2013/12/03 07:18:55, nduca wrote:
> unit test for this new function plz

Done.

[fmeawad, 2013-12-03 21:04:41]

https://codereview.chromium.org/100073003/diff/1/tools/telemetry/telemetry/co...
File tools/telemetry/telemetry/core/timeline/model.py (right):

https://codereview.chromium.org/100073003/diff/1/tools/telemetry/telemetry/co...
tools/telemetry/telemetry/core/timeline/model.py:106: thread_shift_amount =
self._thread_bounds.min
On 2013/12/03 20:55:54, Manfred Ernst wrote:
> On 2013/12/03 17:34:17, fmeawad-cr wrote:
> > Thread timestamps are reported from the time the thread started, shifting
them
> > may create a loss of information.
> > On the other hand, thread timestamps are rarely used, thread duration is the
> one
> > usually used (shifting has no effect then).
> 
> Maybe we keep it as is (which is consistent with the other timestamps), until
we
> find a use case that requires non-shifted thread timestamps?
Sounds good.

[commit-bot: I haz the power, 2013-12-03 21:13:56]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/ernstm@chromium.org/100073003/20001

[commit-bot: I haz the power, 2013-12-03 23:52:20]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/ernstm@chromium.org/100073003/20001

[commit-bot: I haz the power, 2013-12-04 06:37:27]

Change committed as 238607