LatencyInfo mojo: Avoid extra copy of LatencyComponents

LatencyInfo mojo: Avoid extra copy of LatencyComponents

Prior to ArrayDataView, during deserialization, we read latency components outs
into a mojo::Array and then read the mojo array out into LatencyInfo's
latency_components_. This results in an unnecessary extra copy and is not
compatible with usage in blink. This CL moves us towards using ArrayDataView.

BUG=629566
Committed: https://crrev.com/b8cd385fb7004724f431d99af4f410adcd51888e
Cr-Commit-Position: refs/heads/master@{#414157}

[Fady Samuel, 2016-08-23 20:19:23]

fsamuel@chromium.org changed reviewers:
+ sadrul@chromium.org, xlai@chromium.org

[xlai (Olivia), 2016-08-23 20:42:54]

Thanks for the change! Non-owner lgtm.

[Fady Samuel, 2016-08-23 20:47:14]

fsamuel@chromium.org changed reviewers:
+ tsepez@chromium.org, yzshen@chromium.org

[Fady Samuel, 2016-08-23 20:47:15]

+tsepez@ for security review
+yzshen@ FYI

[yzshen1, 2016-08-23 21:23:05]

https://codereview.chromium.org/2276743002/diff/1/ui/events/mojo/latency_info...
File ui/events/mojo/latency_info_struct_traits.cc (right):

https://codereview.chromium.org/2276743002/diff/1/ui/events/mojo/latency_info...
ui/events/mojo/latency_info_struct_traits.cc:283: components.GetDataView(i,
&component_pair);
This change itself LGTM.

Performance fyi: It would be more efficient to store keys and values in two
parallel arrays, rather than putting them in a single array of pair struct.
Because that would save the cost of serializing/validating pair structs.

(But Daniel discussed with me about his concern that checking size of the two
parallel arrays matches would be one more thing to consider. So if this is
really performance critical and you decide to change it to two parallel arrays,
you might want to discuss with him. There is a feature request filed to support
object keys for mojo map.)

[Tom Sepez, 2016-08-24 01:10:30]

On 2016/08/23 21:23:05, yzshen1 wrote:
>
https://codereview.chromium.org/2276743002/diff/1/ui/events/mojo/latency_info...
> File ui/events/mojo/latency_info_struct_traits.cc (right):
> 
>
https://codereview.chromium.org/2276743002/diff/1/ui/events/mojo/latency_info...
> ui/events/mojo/latency_info_struct_traits.cc:283: components.GetDataView(i,
> &component_pair);
> This change itself LGTM.
> 
> Performance fyi: It would be more efficient to store keys and values in two
> parallel arrays, rather than putting them in a single array of pair struct.
> Because that would save the cost of serializing/validating pair structs.
> 
> (But Daniel discussed with me about his concern that checking size of the two
> parallel arrays matches would be one more thing to consider. So if this is
> really performance critical and you decide to change it to two parallel
arrays,
> you might want to discuss with him. There is a feature request filed to
support
> object keys for mojo map.)

From a security perspective, we always discourage the parallel array design.

[Fady Samuel, 2016-08-24 01:20:31]

On 2016/08/24 01:10:30, Tom Sepez (Offsite til 8-23) wrote:
> On 2016/08/23 21:23:05, yzshen1 wrote:
> >
>
https://codereview.chromium.org/2276743002/diff/1/ui/events/mojo/latency_info...
> > File ui/events/mojo/latency_info_struct_traits.cc (right):
> > 
> >
>
https://codereview.chromium.org/2276743002/diff/1/ui/events/mojo/latency_info...
> > ui/events/mojo/latency_info_struct_traits.cc:283: components.GetDataView(i,
> > &component_pair);
> > This change itself LGTM.
> > 
> > Performance fyi: It would be more efficient to store keys and values in two
> > parallel arrays, rather than putting them in a single array of pair struct.
> > Because that would save the cost of serializing/validating pair structs.
> > 
> > (But Daniel discussed with me about his concern that checking size of the
two
> > parallel arrays matches would be one more thing to consider. So if this is
> > really performance critical and you decide to change it to two parallel
> arrays,
> > you might want to discuss with him. There is a feature request filed to
> support
> > object keys for mojo map.)
> 
> From a security perspective, we always discourage the parallel array design.

How do you feel about the CL as it is now Tom?

[Tom Sepez, 2016-08-24 16:02:54]

Currently LGTM.

[Fady Samuel, 2016-08-24 16:05:12]

On 2016/08/24 16:02:54, Tom Sepez wrote:
> Currently LGTM.

Thanks all! I'm CQ'ing now.

[Fady Samuel, 2016-08-24 16:05:16]

The CQ bit was checked by fsamuel@chromium.org

[commit-bot: I haz the power, 2016-08-24 16:05:38]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-08-24 18:16:02]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-08-24 18:16:04]

Try jobs failed on following builders:
  linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)

[Fady Samuel, 2016-08-24 18:17:38]

The CQ bit was checked by fsamuel@chromium.org

[commit-bot: I haz the power, 2016-08-24 18:18:45]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-08-24 21:48:00]

Committed patchset #1 (id:1)

[commit-bot: I haz the power, 2016-08-24 21:49:26]

Description was changed from

==========
LatencyInfo mojo: Avoid extra copy of LatencyComponents

Prior to ArrayDataView, during deserialization, we read latency components outs
into a mojo::Array and then read the mojo array out into LatencyInfo's
latency_components_. This results in an unnecessary extra copy and is not
compatible with usage in blink. This CL moves us towards using ArrayDataView.

BUG=629566
==========

to

==========
LatencyInfo mojo: Avoid extra copy of LatencyComponents

Prior to ArrayDataView, during deserialization, we read latency components outs
into a mojo::Array and then read the mojo array out into LatencyInfo's
latency_components_. This results in an unnecessary extra copy and is not
compatible with usage in blink. This CL moves us towards using ArrayDataView.

BUG=629566

Committed: https://crrev.com/b8cd385fb7004724f431d99af4f410adcd51888e
Cr-Commit-Position: refs/heads/master@{#414157}
==========

[commit-bot: I haz the power, 2016-08-24 21:49:27]

Patchset 1 (id:??) landed as
https://crrev.com/b8cd385fb7004724f431d99af4f410adcd51888e
Cr-Commit-Position: refs/heads/master@{#414157}