telemetry: Add GetRendererProcessFromTab to timeline model.

telemetry: Add GetRendererProcessFromTab to timeline model.

R=nduca@chromium.org
BUG=264308
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=233417

[nduca, 2013-10-23 07:05:21]

https://codereview.chromium.org/29423005/diff/1/tools/telemetry/telemetry/cor...
File tools/telemetry/telemetry/core/tab.py (right):

https://codereview.chromium.org/29423005/diff/1/tools/telemetry/telemetry/cor...
tools/telemetry/telemetry/core/tab.py:34: @property
so we never want to expose this kind of stuff up to tab. we try to lock tab.py
and browser.py to stuff that is guaranteed to work in all browsers, even non
chrome ones.

how about adding a map to the tracing results object that goes
tab->string_to_search_for.

And add a generic feature to Timeline:
  self._core_object_to_timeline_container_map = {}
  AddMapping(core_object, container):
     assert core_object instanceof core.WebContents  or core.Browser
     self._core_boject_to_blah.Add(core_object, container).

Then, in your tracing_results object, the AsModel() can be:
1. create the timeline and import it, then
2. find all the mapping strings the browser_backend created before and then
3. AddMappings for each process and thread found

Make sense?

[ernstm, 2013-10-23 20:47:16]

I've re-factored the CL based on your feedback. Please take another look. One
question is how much we want to make the mapping functionality generic versus
specialized for tab-to-renderer-process?

https://codereview.chromium.org/29423005/diff/1/tools/telemetry/telemetry/cor...
File tools/telemetry/telemetry/core/tab.py (right):

https://codereview.chromium.org/29423005/diff/1/tools/telemetry/telemetry/cor...
tools/telemetry/telemetry/core/tab.py:34: @property
On 2013/10/23 07:05:21, nduca wrote:
> so we never want to expose this kind of stuff up to tab. we try to lock tab.py
> and browser.py to stuff that is guaranteed to work in all browsers, even non
> chrome ones.
> 
> how about adding a map to the tracing results object that goes
> tab->string_to_search_for.
> 
> And add a generic feature to Timeline:
>   self._core_object_to_timeline_container_map = {}
>   AddMapping(core_object, container):
>      assert core_object instanceof core.WebContents  or core.Browser
>      self._core_boject_to_blah.Add(core_object, container).
> 
> Then, in your tracing_results object, the AsModel() can be:
> 1. create the timeline and import it, then
> 2. find all the mapping strings the browser_backend created before and then
> 3. AddMappings for each process and thread found
> 
> Make sense?

Done.

[nduca, 2013-10-31 07:32:02]

Sorry, I have a ton more feedback :$

https://codereview.chromium.org/29423005/diff/340001/tools/perf/metrics/smoot...
File tools/perf/metrics/smoothness.py (left):

https://codereview.chromium.org/29423005/diff/340001/tools/perf/metrics/smoot...
tools/perf/metrics/smoothness.py:2: # Use of this source code is governed by a
BSD-style license that can be
i'm pretty sure there are other uses in perf/ that use this trick please make
sure you convert them all

https://codereview.chromium.org/29423005/diff/340001/tools/perf/metrics/smoot...
File tools/perf/metrics/smoothness.py (right):

https://codereview.chromium.org/29423005/diff/340001/tools/perf/metrics/smoot...
tools/perf/metrics/smoothness.py:55: timeline_marker_labels =
GetTimelineMarkerLabelsFromAction(
Labels is a bit funky. TimelineEvents have names, right, not labels. Maybe my
fault in suggeseting this name.

https://codereview.chromium.org/29423005/diff/340001/tools/perf/metrics/smoot...
tools/perf/metrics/smoothness.py:74: def SetStats(self, stats):
This isn't quite the right direction to be pursuing for unit tests and exposes
some architectural flaws that make testing hard. Often times when you find
yourself adding a method here you should step back and figure out whats going
wrong in your architecture that exposes the need for such a test-specific call.

I think the thing that goes wrong here is that that the
RenderingStats<->TimelineModel interface isn't very well defined. 

Stop should be stashing away timeline_marker_labels and the TimelineModel. You
can compute a RenderingStats results completely from labels. And you can dump
RenderingStats to a results object entirely without needing the tab, or any
fields on the Metric object.

In my mind, SmoothnessMetrics.AddResults() should be a simple function that says
literally two lines:
 stats = RenderingStats(self._timeline_model,
filter_by=self._timeline_marker_labels)
 stats.AddResults(results)

The bulk of Addresults, I think belongs in RenderingStats.

Then, the unit tests of this class should just test that the test is making the
right calls --- by creating a *real* browser and verifying simply that this code
does't crash.

The rest of the testing shoudl then go to RenderingStats where you give it an
already-created model, and verify that the numbers it computes are correct, AND
that the nubmers added to the results are correct.

https://codereview.chromium.org/29423005/diff/340001/tools/perf/metrics/smoot...
File tools/perf/metrics/smoothness_unittest.py (right):

https://codereview.chromium.org/29423005/diff/340001/tools/perf/metrics/smoot...
tools/perf/metrics/smoothness_unittest.py:11: from
telemetry.core.backends.chrome.tracing_backend import RawTraceResult
this is a no-no.

you should be able to construct a TimelineModel directly from events.

https://codereview.chromium.org/29423005/diff/340001/tools/perf/metrics/smoot...
tools/perf/metrics/smoothness_unittest.py:94: def
testCalcResultsTraceEvents(self):
there's something confusing here. This is a test that blurs testing of rendering
stats calculation from a timeline model from the operation of the metric object.

In my head, there should be a test of the metric object that uses a mocked tabs
and verifies that the right calls are made on the tabs etc.

Then there should be a test of the rendering stats that is checked in to a
rendering_stats_unittest.py and should operate purely on timeline model with a
passed in tab instance.

Where the tab instance is just a mock in all cases.

Again I wont block this patch on this but I am seeing too many strange changes
happening in this group of files that suggests we've grown a kind of spaghetti.

https://codereview.chromium.org/29423005/diff/340001/tools/perf/metrics/smoot...
tools/perf/metrics/smoothness_unittest.py:101: num_frames_sent = 0.0
as a bit of a comment, I think possibly the test is written a bit oddly...
you're having to create actual trace events. A simpler way would be to create a
TimelineModel yourself and populate it by adding data to the model. If you think
about it, this is testing the analysis of model not the trace parser. Thats what
we do in trace-viewer too (we didn't always, we started by writign out trace
events, but it became a lot cleaner when we made models).

you dont have to block this patch on this feedback but its sorta a passing
comment.

https://codereview.chromium.org/29423005/diff/340001/tools/telemetry/telemetr...
File tools/telemetry/telemetry/core/backends/chrome/chrome_browser_backend.py
(right):

https://codereview.chromium.org/29423005/diff/340001/tools/telemetry/telemetr...
tools/telemetry/telemetry/core/backends/chrome/chrome_browser_backend.py:257:
for (i, debugger_url) in enumerate(self._browser.tabs):
you need to make sure you close the inspector sessions after you enumerate them
so this code needs work.

background: telemetry has a different websocket connection to every tab. By
executing javascript, you open connections even for ones that were previously
not used. So you need to dig into the backend and figure out which ones were
opened just by this iteration and close them down again so we dont purturb the
measurement too much.

This also makes me wonder about whether StartTracing is the right place to do
this, or EndTracing is better. This is gonna be expensive, if you think about
it.

https://codereview.chromium.org/29423005/diff/340001/tools/telemetry/telemetr...
File tools/telemetry/telemetry/core/backends/chrome/trace_result.py (right):

https://codereview.chromium.org/29423005/diff/340001/tools/telemetry/telemetr...
tools/telemetry/telemetry/core/backends/chrome/trace_result.py:5: class
TraceResult(object):
So okay, I remember now why impl was there. We've got to undo the change you
made.

TraceResult is a public class. Its in the wrong place here. It should be in
core/trace_result.py

We have a rule in telemetry that you dont subclass public classes for their
implementation. If you look, for instance, browser has a backend object. That is
because if users do help(browser), they'll see the public methods for not only
Browser, but also its implementation. Thats bad because it lets the backends
accidentally leak implementation-specific data up to the  user.

In this instance, you're actually doing that: ÅddTabToMarkerMapping for
instance, is really a backend implementation detail, but its now callable by
users. Thats not what we want.

What you probably want to do here is move this file as was before to
telemetyr/core/trace_results.py and keep the impl thing. AsTimelineModel shoudl
be unmodified.

The RawTraceResult and LegacyTraceResult should subclass from ChromeTraceResult,
and if it helped, you could call them ChromeRawTraceResult and
ChromeLegacyTraceResult. inside those two implementations, you make the base
have roughly the interface you have here, as was done before.

https://codereview.chromium.org/29423005/diff/340001/tools/telemetry/telemetr...
tools/telemetry/telemetry/core/backends/chrome/trace_result.py:15: """Parses the
trace result into a timeline model for in-memory
If you pass the marker mapping in to this constructor then you can add the
mappings right away and avoid the odd AddTabToBlah call on this that is visible
to users of the api.

https://codereview.chromium.org/29423005/diff/340001/tools/telemetry/telemetr...
File tools/telemetry/telemetry/core/backends/chrome/tracing_backend.py (right):

https://codereview.chromium.org/29423005/diff/340001/tools/telemetry/telemetr...
tools/telemetry/telemetry/core/backends/chrome/tracing_backend.py:198: for key,
value in self._tab_to_marker_map.iteritems():
this is not quite right. you should be passing in the mapping to the result
object, not mutating the object after its created. if you do it that way, then
the Create method can do the mutation of markers.

https://codereview.chromium.org/29423005/diff/340001/tools/telemetry/telemetr...
File tools/telemetry/telemetry/core/timeline/model.py (right):

https://codereview.chromium.org/29423005/diff/340001/tools/telemetry/telemetr...
tools/telemetry/telemetry/core/timeline/model.py:177: def AddMapping(self,
core_object, container):
Name should be more precise -- mapping of what, a newbie to the code might
ask...

AddCoreObjectToContainerMapping

https://codereview.chromium.org/29423005/diff/340001/tools/telemetry/telemetr...
tools/telemetry/telemetry/core/timeline/model.py:177: def AddMapping(self,
core_object, container):
docstring plz

[ernstm, 2013-11-05 00:25:04]

Let's do the smoothness_unittest overhaul in a separate patch. Please take
another look at the "renderer process from tab" part.

https://codereview.chromium.org/29423005/diff/340001/tools/perf/metrics/smoot...
File tools/perf/metrics/smoothness.py (right):

https://codereview.chromium.org/29423005/diff/340001/tools/perf/metrics/smoot...
tools/perf/metrics/smoothness.py:55: timeline_marker_labels =
GetTimelineMarkerLabelsFromAction(
On 2013/10/31 07:32:02, nduca wrote:
> Labels is a bit funky. TimelineEvents have names, right, not labels. Maybe my
> fault in suggeseting this name.

Done.

https://codereview.chromium.org/29423005/diff/340001/tools/perf/metrics/smoot...
tools/perf/metrics/smoothness.py:74: def SetStats(self, stats):
On 2013/10/31 07:32:02, nduca wrote:
> This isn't quite the right direction to be pursuing for unit tests and exposes
> some architectural flaws that make testing hard. Often times when you find
> yourself adding a method here you should step back and figure out whats going
> wrong in your architecture that exposes the need for such a test-specific
call.
> 
> I think the thing that goes wrong here is that that the
> RenderingStats<->TimelineModel interface isn't very well defined. 
> 
> Stop should be stashing away timeline_marker_labels and the TimelineModel. You
> can compute a RenderingStats results completely from labels. And you can dump
> RenderingStats to a results object entirely without needing the tab, or any
> fields on the Metric object.
> 
> In my mind, SmoothnessMetrics.AddResults() should be a simple function that
says
> literally two lines:
>  stats = RenderingStats(self._timeline_model,
> filter_by=self._timeline_marker_labels)
>  stats.AddResults(results)
> 
> The bulk of Addresults, I think belongs in RenderingStats.
> 
> Then, the unit tests of this class should just test that the test is making
the
> right calls --- by creating a *real* browser and verifying simply that this
code
> does't crash.
> 
> The rest of the testing shoudl then go to RenderingStats where you give it an
> already-created model, and verify that the numbers it computes are correct,
AND
> that the nubmers added to the results are correct.

I agree that smoothness_unittest.py needs a major overhaul. Filed
crbug.com/314877 and will address in a separate patch.

I don't think we should have AddResults() on RenderingStats, though.
RenderingStats extracts the raw stats from a trace. Metrics use it to compute
higher-level statistical measures and add those to the results object. Different
metrics compute different statistical measures.

https://codereview.chromium.org/29423005/diff/340001/tools/perf/metrics/smoot...
File tools/perf/metrics/smoothness_unittest.py (right):

https://codereview.chromium.org/29423005/diff/340001/tools/perf/metrics/smoot...
tools/perf/metrics/smoothness_unittest.py:11: from
telemetry.core.backends.chrome.tracing_backend import RawTraceResult
On 2013/10/31 07:32:02, nduca wrote:
> this is a no-no.
> 
> you should be able to construct a TimelineModel directly from events.

crbug.com/314877

https://codereview.chromium.org/29423005/diff/340001/tools/perf/metrics/smoot...
tools/perf/metrics/smoothness_unittest.py:94: def
testCalcResultsTraceEvents(self):
On 2013/10/31 07:32:02, nduca wrote:
> there's something confusing here. This is a test that blurs testing of
rendering
> stats calculation from a timeline model from the operation of the metric
object.
> 
> In my head, there should be a test of the metric object that uses a mocked
tabs
> and verifies that the right calls are made on the tabs etc.
> 
> Then there should be a test of the rendering stats that is checked in to a
> rendering_stats_unittest.py and should operate purely on timeline model with a
> passed in tab instance.
> 
> Where the tab instance is just a mock in all cases.
> 
> Again I wont block this patch on this but I am seeing too many strange changes
> happening in this group of files that suggests we've grown a kind of
spaghetti.

Yes, the unittest has a pasta flavor -> crbug.com/314877

https://codereview.chromium.org/29423005/diff/340001/tools/perf/metrics/smoot...
tools/perf/metrics/smoothness_unittest.py:101: num_frames_sent = 0.0
On 2013/10/31 07:32:02, nduca wrote:
> as a bit of a comment, I think possibly the test is written a bit oddly...
> you're having to create actual trace events. A simpler way would be to create
a
> TimelineModel yourself and populate it by adding data to the model. If you
think
> about it, this is testing the analysis of model not the trace parser. Thats
what
> we do in trace-viewer too (we didn't always, we started by writign out trace
> events, but it became a lot cleaner when we made models).
> 
> you dont have to block this patch on this feedback but its sorta a passing
> comment.

crbug.com/314877

https://codereview.chromium.org/29423005/diff/340001/tools/telemetry/telemetr...
File tools/telemetry/telemetry/core/backends/chrome/chrome_browser_backend.py
(right):

https://codereview.chromium.org/29423005/diff/340001/tools/telemetry/telemetr...
tools/telemetry/telemetry/core/backends/chrome/chrome_browser_backend.py:257:
for (i, debugger_url) in enumerate(self._browser.tabs):
On 2013/10/31 07:32:02, nduca wrote:
> you need to make sure you close the inspector sessions after you enumerate
them
> so this code needs work.
> 
> background: telemetry has a different websocket connection to every tab. By
> executing javascript, you open connections even for ones that were previously
> not used. So you need to dig into the backend and figure out which ones were
> opened just by this iteration and close them down again so we dont purturb the
> measurement too much.
> 
> This also makes me wonder about whether StartTracing is the right place to do
> this, or EndTracing is better. This is gonna be expensive, if you think about
> it.

I moved the iteration to StopTracing. Do you think we still need to manually
close the connections there?

https://codereview.chromium.org/29423005/diff/340001/tools/telemetry/telemetr...
File tools/telemetry/telemetry/core/backends/chrome/trace_result.py (right):

https://codereview.chromium.org/29423005/diff/340001/tools/telemetry/telemetr...
tools/telemetry/telemetry/core/backends/chrome/trace_result.py:5: class
TraceResult(object):
On 2013/10/31 07:32:02, nduca wrote:
> So okay, I remember now why impl was there. We've got to undo the change you
> made.
> 
> TraceResult is a public class. Its in the wrong place here. It should be in
> core/trace_result.py
> 
> We have a rule in telemetry that you dont subclass public classes for their
> implementation. If you look, for instance, browser has a backend object. That
is
> because if users do help(browser), they'll see the public methods for not only
> Browser, but also its implementation. Thats bad because it lets the backends
> accidentally leak implementation-specific data up to the  user.
> 
> In this instance, you're actually doing that: ÅddTabToMarkerMapping for
> instance, is really a backend implementation detail, but its now callable by
> users. Thats not what we want.
> 
> What you probably want to do here is move this file as was before to
> telemetyr/core/trace_results.py and keep the impl thing. AsTimelineModel
shoudl
> be unmodified.
> 
> The RawTraceResult and LegacyTraceResult should subclass from
ChromeTraceResult,
> and if it helped, you could call them ChromeRawTraceResult and
> ChromeLegacyTraceResult. inside those two implementations, you make the base
> have roughly the interface you have here, as was done before.

Done.

https://codereview.chromium.org/29423005/diff/340001/tools/telemetry/telemetr...
tools/telemetry/telemetry/core/backends/chrome/trace_result.py:15: """Parses the
trace result into a timeline model for in-memory
On 2013/10/31 07:32:02, nduca wrote:
> If you pass the marker mapping in to this constructor then you can add the
> mappings right away and avoid the odd AddTabToBlah call on this that is
visible
> to users of the api.

Done.

https://codereview.chromium.org/29423005/diff/340001/tools/telemetry/telemetr...
File tools/telemetry/telemetry/core/timeline/model.py (right):

https://codereview.chromium.org/29423005/diff/340001/tools/telemetry/telemetr...
tools/telemetry/telemetry/core/timeline/model.py:177: def AddMapping(self,
core_object, container):
On 2013/10/31 07:32:02, nduca wrote:
> Name should be more precise -- mapping of what, a newbie to the code might
> ask...
> 
> AddCoreObjectToContainerMapping

Done.

https://codereview.chromium.org/29423005/diff/340001/tools/telemetry/telemetr...
tools/telemetry/telemetry/core/timeline/model.py:177: def AddMapping(self,
core_object, container):
On 2013/10/31 07:32:02, nduca wrote:
> docstring plz

Done.

[nduca, 2013-11-05 19:42:19]

lgtm

https://codereview.chromium.org/29423005/diff/420001/tools/telemetry/telemetr...
File tools/telemetry/telemetry/core/backends/chrome/chrome_trace_result.py
(right):

https://codereview.chromium.org/29423005/diff/420001/tools/telemetry/telemetr...
tools/telemetry/telemetry/core/backends/chrome/chrome_trace_result.py:7: def
__init__(self, tracing_data, tab_to_marker_mapping):
tab_to_marker_mapping=None as a default

if its none, then set self._tab_tlasadfasf = []

[ernstm, 2013-11-05 22:27:05]

https://codereview.chromium.org/29423005/diff/420001/tools/telemetry/telemetr...
File tools/telemetry/telemetry/core/backends/chrome/chrome_trace_result.py
(right):

https://codereview.chromium.org/29423005/diff/420001/tools/telemetry/telemetr...
tools/telemetry/telemetry/core/backends/chrome/chrome_trace_result.py:7: def
__init__(self, tracing_data, tab_to_marker_mapping):
On 2013/11/05 19:42:20, nduca wrote:
> tab_to_marker_mapping=None as a default
> 
> if its none, then set self._tab_tlasadfasf = []

Done.

[commit-bot: I haz the power, 2013-11-05 22:40:18]

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

[commit-bot: I haz the power, 2013-11-06 00:36:32]

Retried try job too often on mac_rel for step(s) telemetry_unittests
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=mac_rel&nu...

[commit-bot: I haz the power, 2013-11-06 00:40:19]

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

[commit-bot: I haz the power, 2013-11-06 02:05:06]

Retried try job too often on mac_rel for step(s) telemetry_unittests
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=mac_rel&nu...

[commit-bot: I haz the power, 2013-11-06 04:52:41]

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

[commit-bot: I haz the power, 2013-11-06 05:55:12]

Retried try job too often on mac_rel for step(s) telemetry_unittests
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=mac_rel&nu...

[commit-bot: I haz the power, 2013-11-06 06:04:20]

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

[commit-bot: I haz the power, 2013-11-06 07:03:00]

Retried try job too often on mac_rel for step(s) telemetry_unittests
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=mac_rel&nu...

[commit-bot: I haz the power, 2013-11-06 17:27:05]

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

[commit-bot: I haz the power, 2013-11-06 23:43:36]

Change committed as 233417