Upgrade loading_model to use the new request_track and loading_trace.

Upgrade loading_model to use the new request_track and loading_trace.

Committed: https://crrev.com/6a14e6fe7c6bc7a0b355c2a3812179fa5cde4534
Cr-Commit-Position: refs/heads/master@{#370673}

[mattcary, 2016-01-20 20:29:47]

mattcary@chromium.org changed reviewers:
+ blundell@chromium.org, lizeb@chromium.org

[mattcary, 2016-01-20 20:29:47]

After this CL, analyze.py will need to be updated or perhaps folded in to the
new driver.

[mattcary, 2016-01-20 20:31:51]

On 2016/01/20 20:29:47, mattcary wrote:
> After this CL, analyze.py will need to be updated or perhaps folded in to the
> new driver.

Sorry, hit "publish" a little too quickly. I shouldn't do this at night... What
I meant to say was,

Hi Benôit and Colin:

This CL is the most annoying part, I think, of refactoring the current model to
use the new request_track, etc. analyze.py and others will be broken, but I
thought it would be cleaner to consolidate all the end scripts in a separate CL.

Thanks

Matt

[Benoit L, 2016-01-21 09:09:26]

lgtm with a few nits, thanks!

https://codereview.chromium.org/1610273002/diff/1/tools/android/loading/loadi...
File tools/android/loading/loading_model.py (right):

https://codereview.chromium.org/1610273002/diff/1/tools/android/loading/loadi...
tools/android/loading/loading_model.py:31: REQUEST_LENS = None
nit: Can you add a comment saying that this a a class, and meant for testing?

https://codereview.chromium.org/1610273002/diff/1/tools/android/loading/loadi...
tools/android/loading/loading_model.py:361: def ShortName(self):
Might be better in "Request", but I don't have a strong preference. Up to you.

https://codereview.chromium.org/1610273002/diff/1/tools/android/loading/loadi...
tools/android/loading/loading_model.py:455: index_by_request = {}
Since we now identify everything by the request_id (and requests are hashable),
would that make sense to not use an index, and use Request.request_id instead?

Unfortunately, that means more work for no visible benefits, so it's really up
to you.

https://codereview.chromium.org/1610273002/diff/1/tools/android/loading/loadi...
tools/android/loading/loading_model.py:476: # fair amount in practice.
Especially for "parser" dependencies, I suppose?
(due to incremental parsing)

https://codereview.chromium.org/1610273002/diff/1/tools/android/loading/loadi...
File tools/android/loading/loading_model_unittest.py (right):

https://codereview.chromium.org/1610273002/diff/1/tools/android/loading/loadi...
tools/android/loading/loading_model_unittest.py:14: class SimpleLens(object):
nit: 2 lines between top-level declarations.

https://codereview.chromium.org/1610273002/diff/1/tools/android/loading/loadi...
tools/android/loading/loading_model_unittest.py:60: 
nit: extra blank line.

[blundell, 2016-01-21 09:22:53]

I'm not keyed in enough yet to have substantive comments, but it's instructive
to look over the changes :).

https://codereview.chromium.org/1610273002/diff/1/tools/android/loading/loadi...
File tools/android/loading/loading_model.py (left):

https://codereview.chromium.org/1610273002/diff/1/tools/android/loading/loadi...
tools/android/loading/loading_model.py:185: cost[next.Index()] >
cost[costliest.Index()])
hmm, was this a bug?

https://codereview.chromium.org/1610273002/diff/1/tools/android/loading/loadi...
File tools/android/loading/loading_model.py (right):

https://codereview.chromium.org/1610273002/diff/1/tools/android/loading/loadi...
tools/android/loading/loading_model.py:9: CostMs(): return the cost of the cost
in milliseconds.
the cost of the cost?

https://codereview.chromium.org/1610273002/diff/1/tools/android/loading/loadi...
tools/android/loading/loading_model.py:10: Set(): set model-specifical
parameters.
nit: specific

https://codereview.chromium.org/1610273002/diff/1/tools/android/loading/loadi...
tools/android/loading/loading_model.py:13: This creates a DAG of resource
dependancies from loading.log_requests to model
nit: dependencies

[mattcary, 2016-01-21 10:04:22]

https://codereview.chromium.org/1610273002/diff/1/tools/android/loading/loadi...
File tools/android/loading/loading_model.py (left):

https://codereview.chromium.org/1610273002/diff/1/tools/android/loading/loadi...
tools/android/loading/loading_model.py:185: cost[next.Index()] >
cost[costliest.Index()])
On 2016/01/21 09:22:53, blundell wrote:
> hmm, was this a bug?

Yes, I'm not sure how that got introduced, because cost is an int, not a list,
so it totally fails.

https://codereview.chromium.org/1610273002/diff/1/tools/android/loading/loadi...
File tools/android/loading/loading_model.py (right):

https://codereview.chromium.org/1610273002/diff/1/tools/android/loading/loadi...
tools/android/loading/loading_model.py:9: CostMs(): return the cost of the cost
in milliseconds.
On 2016/01/21 09:22:53, blundell wrote:
> the cost of the cost?

https://memegen.googleplex.com/5342822322929664

https://codereview.chromium.org/1610273002/diff/1/tools/android/loading/loadi...
tools/android/loading/loading_model.py:10: Set(): set model-specifical
parameters.
On 2016/01/21 09:22:53, blundell wrote:
> nit: specific

https://memegen.googleplex.com/5322689026195456

https://codereview.chromium.org/1610273002/diff/1/tools/android/loading/loadi...
tools/android/loading/loading_model.py:13: This creates a DAG of resource
dependancies from loading.log_requests to model
On 2016/01/21 09:22:53, blundell wrote:
> nit: dependencies

Done.

https://codereview.chromium.org/1610273002/diff/1/tools/android/loading/loadi...
tools/android/loading/loading_model.py:31: REQUEST_LENS = None
On 2016/01/21 09:09:26, Benoit L wrote:
> nit: Can you add a comment saying that this a a class, and meant for testing?

Oops, I meant to remove this after you landed your CL. Changed around to use the
real class & mock out in the unitest.

https://codereview.chromium.org/1610273002/diff/1/tools/android/loading/loadi...
tools/android/loading/loading_model.py:361: def ShortName(self):
On 2016/01/21 09:09:26, Benoit L wrote:
> Might be better in "Request", but I don't have a strong preference. Up to you.

I wasn't sure what you intended either. I tweaked this method, though, to always
provide domain names to make the graphviz more informative. So since this has
become graphviz-specific, let's leave in here for now.

https://codereview.chromium.org/1610273002/diff/1/tools/android/loading/loadi...
tools/android/loading/loading_model.py:455: index_by_request = {}
On 2016/01/21 09:09:26, Benoit L wrote:
> Since we now identify everything by the request_id (and requests are
hashable),
> would that make sense to not use an index, and use Request.request_id instead?
> 
> Unfortunately, that means more work for no visible benefits, so it's really up
> to you.

DAG nodes don't have a request id, so I think we have to have an index for them
at some point anyway (eg, request-id to Node). The advantage of the list-based
index is that we only have to do the assertion that request ids are unique here
when we build it.

https://codereview.chromium.org/1610273002/diff/1/tools/android/loading/loadi...
tools/android/loading/loading_model.py:476: # fair amount in practice.
On 2016/01/21 09:09:26, Benoit L wrote:
> Especially for "parser" dependencies, I suppose?
> (due to incremental parsing)

Ah, yeah, that would make sense.

https://codereview.chromium.org/1610273002/diff/1/tools/android/loading/loadi...
File tools/android/loading/loading_model_unittest.py (right):

https://codereview.chromium.org/1610273002/diff/1/tools/android/loading/loadi...
tools/android/loading/loading_model_unittest.py:14: class SimpleLens(object):
On 2016/01/21 09:09:26, Benoit L wrote:
> nit: 2 lines between top-level declarations.

Done.

https://codereview.chromium.org/1610273002/diff/1/tools/android/loading/loadi...
tools/android/loading/loading_model_unittest.py:60: 
On 2016/01/21 09:09:26, Benoit L wrote:
> nit: extra blank line.

Done.

[Benoit L, 2016-01-21 10:38:01]

https://codereview.chromium.org/1610273002/diff/1/tools/android/loading/loadi...
File tools/android/loading/loading_model.py (right):

https://codereview.chromium.org/1610273002/diff/1/tools/android/loading/loadi...
tools/android/loading/loading_model.py:361: def ShortName(self):
On 2016/01/21 10:04:22, mattcary wrote:
> On 2016/01/21 09:09:26, Benoit L wrote:
> > Might be better in "Request", but I don't have a strong preference. Up to
you.
> 
> I wasn't sure what you intended either. I tweaked this method, though, to
always
> provide domain names to make the graphviz more informative. So since this has
> become graphviz-specific, let's leave in here for now.

Yes, you're right.

https://codereview.chromium.org/1610273002/diff/1/tools/android/loading/loadi...
tools/android/loading/loading_model.py:455: index_by_request = {}
On 2016/01/21 10:04:22, mattcary wrote:
> On 2016/01/21 09:09:26, Benoit L wrote:
> > Since we now identify everything by the request_id (and requests are
> hashable),
> > would that make sense to not use an index, and use Request.request_id
instead?
> > 
> > Unfortunately, that means more work for no visible benefits, so it's really
up
> > to you.
> 
> DAG nodes don't have a request id, so I think we have to have an index for
them
> at some point anyway (eg, request-id to Node). The advantage of the list-based
> index is that we only have to do the assertion that request ids are unique
here
> when we build it.

Acknowledged.

[mattcary, 2016-01-21 12:01:49]

The CQ bit was checked by mattcary@chromium.org

[mattcary, 2016-01-21 12:01:49]

The patchset sent to the CQ was uploaded after l-g-t-m from lizeb@chromium.org
Link to the patchset: https://codereview.chromium.org/1610273002/#ps40001
(title: "rebase")

[commit-bot: I haz the power, 2016-01-21 12:02:46]

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

[commit-bot: I haz the power, 2016-01-21 12:18:27]

Committed patchset #3 (id:40001)

[commit-bot: I haz the power, 2016-01-21 12:19:44]

Description was changed from

==========
Upgrade loading_model to use the new request_track and loading_trace.
==========

to

==========
Upgrade loading_model to use the new request_track and loading_trace.

Committed: https://crrev.com/6a14e6fe7c6bc7a0b355c2a3812179fa5cde4534
Cr-Commit-Position: refs/heads/master@{#370673}
==========

[commit-bot: I haz the power, 2016-01-21 12:19:45]

Patchset 3 (id:??) landed as
https://crrev.com/6a14e6fe7c6bc7a0b355c2a3812179fa5cde4534
Cr-Commit-Position: refs/heads/master@{#370673}