Replace std::map with base::SmallMap in ui::LatencyInfo

Replace std::map with base::SmallMap in ui::LatencyInfo

The typical number of components per LatencyInfo instance is quite small. Avoid
unnecessary heap allocation by using a SmallMap to store component entries.
This reduces the average browser send time cost per touch or scroll event by
~15us on a Nexus 4, and ~22us on a Galaxy Nexus.

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

[jdduke (slow), 2014-02-13 20:45:43]

jbauman@: What do you think? This change eliminates heap allocation during event
copy entirely on Android, which is a nice win (we probably copy a given
LatencyInfo object 4-5x *per* event lifetime).

[jdduke (slow), 2014-02-13 20:48:42]

https://codereview.chromium.org/157003005/diff/70001/content/common/content_p...
File content/common/content_param_traits.h (right):

https://codereview.chromium.org/157003005/diff/70001/content/common/content_p...
content/common/content_param_traits.h:76: typedef base::SmallMap<NormalMap,
kArraySize, EqualKey, MapInit> param_type;
Hmm, this should probably live in common_param_traits, (or perhaps even in
base/)... I'll move and add a unit test.

[jdduke (slow), 2014-02-14 17:16:26]

tsepez@: Review for ipc/? I originally had the ParamTraits<base::SmallMap> impl
local to content/, as I hesitate to add yet more code to base/, but SmallMap is
already in base/... I can move it back if you like.

[Tom Sepez, 2014-02-14 19:05:37]

https://codereview.chromium.org/157003005/diff/300001/ipc/ipc_message_utils.h
File ipc/ipc_message_utils.h (right):

https://codereview.chromium.org/157003005/diff/300001/ipc/ipc_message_utils.h...
ipc/ipc_message_utils.h:683: WriteParam(m, static_cast<int>(p.size()));
we ought to be careful here if p.size() > INT_MAX, if such a thing is possible
in the underlying type, because later we are using the iterator, not the size to
bound the loop. I'm not sure whether we can make this happen in practice.

https://codereview.chromium.org/157003005/diff/300001/ipc/ipc_message_utils.h...
ipc/ipc_message_utils.h:694: for (int i = 0; i < size; ++i) {
This one is more important: You'll want to return false here if size < 0, to be
safe if a compromised renderer should send a bad message.

[Tom Sepez, 2014-02-14 19:14:41]

https://codereview.chromium.org/157003005/diff/300001/ipc/ipc_message_utils.h
File ipc/ipc_message_utils.h (right):

https://codereview.chromium.org/157003005/diff/300001/ipc/ipc_message_utils.h...
ipc/ipc_message_utils.h:683: WriteParam(m, static_cast<int>(p.size()));
On 2014/02/14 19:05:37, Tom Sepez wrote:
> we ought to be careful here if p.size() > INT_MAX, if such a thing is possible
> in the underlying type, because later we are using the iterator, not the size
to
> bound the loop. I'm not sure whether we can make this happen in practice.

Though std::map has this same issue, so this is probably OK for now, but I need
to think about both of these.

[jdduke (slow), 2014-02-18 01:42:21]

sadrul@: Review for ui/events? Thanks.

https://codereview.chromium.org/157003005/diff/300001/ipc/ipc_message_utils.h
File ipc/ipc_message_utils.h (right):

https://codereview.chromium.org/157003005/diff/300001/ipc/ipc_message_utils.h...
ipc/ipc_message_utils.h:683: WriteParam(m, static_cast<int>(p.size()));
On 2014/02/14 19:14:42, Tom Sepez wrote:
> On 2014/02/14 19:05:37, Tom Sepez wrote:
> > we ought to be careful here if p.size() > INT_MAX, if such a thing is
possible
> > in the underlying type, because later we are using the iterator, not the
size
> to
> > bound the loop. I'm not sure whether we can make this happen in practice.
> 
> Though std::map has this same issue, so this is probably OK for now, but I
need
> to think about both of these.

Good point.  I think some other container types may experience this as well
(std::vector, std::set, ScopedVector).  I'm not quite sure what the best
solution is here, so I'll defer to your judgement.

https://codereview.chromium.org/157003005/diff/300001/ipc/ipc_message_utils.h...
ipc/ipc_message_utils.h:694: for (int i = 0; i < size; ++i) {
On 2014/02/14 19:05:37, Tom Sepez wrote:
> This one is more important: You'll want to return false here if size < 0, to
be
> safe if a compromised renderer should send a bad message.

Hmm, I was under the impression that |ReadLength()| performs that check already?

[sadrul, 2014-02-18 12:42:23]

events/ lgtm

https://codereview.chromium.org/157003005/diff/400001/ui/events/latency_info.h
File ui/events/latency_info.h (right):

https://codereview.chromium.org/157003005/diff/400001/ui/events/latency_info....
ui/events/latency_info.h:89: 
It would be interesting to have a perf-test for this sort of thing.

[Tom Sepez, 2014-02-18 18:49:00]

lgtm

[jdduke (slow), 2014-03-13 22:27:31]

miletus@: Could you double-check the change in
content/renderer/render_widget.cc? Looking at the code, it appears we only
really need to return the swap LatencyInfo component when ack'ing the event, is
that true?

[jdduke (slow), 2014-03-13 22:34:16]

https://codereview.chromium.org/157003005/diff/940001/content/renderer/render...
File content/renderer/render_widget.cc (right):

https://codereview.chromium.org/157003005/diff/940001/content/renderer/render...
content/renderer/render_widget.cc:1062:
compositor_->CreateLatencyInfoSwapPromiseMonitor(&swap_latency_info)
Hmm, actually it looks like the full |latency_info| gets consumed here and
potentially pushed onto the CompositorFrameMetadata, so we'll want to construct
|swap_latency_info| from |latency_info|.  

That said, it's a shame we have to include the full latency info in the ack,
when we're really only interested in returning a single component.

[jdduke (slow), 2014-03-13 22:42:21]

jamesr@: Review for content/renderer/render_widget.{cc,h}? Thanks.

[jamesr, 2014-03-13 22:44:07]

lgtm

[Yufeng Shen (Slow to review), 2014-03-13 23:05:43]

https://codereview.chromium.org/157003005/diff/960001/content/renderer/render...
File content/renderer/render_widget.cc (right):

https://codereview.chromium.org/157003005/diff/960001/content/renderer/render...
content/renderer/render_widget.cc:1059: ui::LatencyInfo
swap_latency_info(latency_info);
So why is pass by reference better than pass by value if you are doing
the copy here anyway ?

I would like to keep the full latencyinfo in the ack since there could be
different components added when the event is processed in impl/main thread. And
in the case if the latencyinfo is not pushed into CompositorFrameMetadata (the
event does not cause any rendering and should be ended when it gets acked back
to browser), we still in browser side have all those components added in
renderer.

[jdduke (slow), 2014-03-13 23:14:07]

I updated the usage to copy the full LatencyInfo.

https://codereview.chromium.org/157003005/diff/960001/content/renderer/render...
File content/renderer/render_widget.cc (right):

https://codereview.chromium.org/157003005/diff/960001/content/renderer/render...
content/renderer/render_widget.cc:1059: ui::LatencyInfo
swap_latency_info(latency_info);
On 2014/03/13 23:05:44, Yufeng Shen wrote:
> So why is pass by reference better than pass by value if you are doing
> the copy here anyway ?
> 
> I would like to keep the full latencyinfo in the ack since there could be
> different components added when the event is processed in impl/main thread.
And
> in the case if the latencyinfo is not pushed into CompositorFrameMetadata (the
> event does not cause any rendering and should be ended when it gets acked back
> to browser), we still in browser side have all those components added in
> renderer.
> 

Visual Studio doesn't allow pass-by-value formal parameters for data types with
aligned memory (SmallMap).  The function must use a reference for the formal
parameter.

[Yufeng Shen (Slow to review), 2014-03-13 23:19:34]

On 2014/03/13 23:14:07, jdduke wrote:
> I updated the usage to copy the full LatencyInfo.
> 
>
https://codereview.chromium.org/157003005/diff/960001/content/renderer/render...
> File content/renderer/render_widget.cc (right):
> 
>
https://codereview.chromium.org/157003005/diff/960001/content/renderer/render...
> content/renderer/render_widget.cc:1059: ui::LatencyInfo
> swap_latency_info(latency_info);
> On 2014/03/13 23:05:44, Yufeng Shen wrote:
> > So why is pass by reference better than pass by value if you are doing
> > the copy here anyway ?
> > 
> > I would like to keep the full latencyinfo in the ack since there could be
> > different components added when the event is processed in impl/main thread.
> And
> > in the case if the latencyinfo is not pushed into CompositorFrameMetadata
(the
> > event does not cause any rendering and should be ended when it gets acked
back
> > to browser), we still in browser side have all those components added in
> > renderer.
> > 
> 
> Visual Studio doesn't allow pass-by-value formal parameters for data types
with
> aligned memory (SmallMap).  The function must use a reference for the formal
> parameter.

content/renderer/render_widget.cc  LGTM.

[jdduke (slow), 2014-03-13 23:25:53]

The CQ bit was checked by jdduke@chromium.org

[commit-bot: I haz the power, 2014-03-13 23:27:16]

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

[commit-bot: I haz the power, 2014-03-14 00:25:36]

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

[jdduke (slow), 2014-03-14 01:44:31]

estade@: Review for 
chrome/browser/ui/views/autofill/autofill_popup_base_view_browsertest.cc?
Thanks.

[Evan Stade, 2014-03-17 15:45:26]

lgtm

[jdduke (slow), 2014-03-18 09:56:46]

The CQ bit was checked by jdduke@chromium.org

[commit-bot: I haz the power, 2014-03-18 09:56:51]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/jdduke@chromium.org/157003005/1000001

[commit-bot: I haz the power, 2014-03-18 09:57:52]

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

[commit-bot: I haz the power, 2014-03-18 09:57:53]

Try jobs failed on following builders:
  tryserver.chromium on linux_chromium_clang_dbg

[jdduke (slow), 2014-03-18 13:43:12]

The CQ bit was checked by jdduke@chromium.org

[commit-bot: I haz the power, 2014-03-18 13:43:24]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/jdduke@chromium.org/157003005/1000001

[commit-bot: I haz the power, 2014-03-18 14:29:21]

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

[commit-bot: I haz the power, 2014-03-18 14:29:22]

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

[jdduke (slow), 2014-03-18 14:33:26]

The CQ bit was checked by jdduke@chromium.org

[commit-bot: I haz the power, 2014-03-18 14:33:45]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/jdduke@chromium.org/157003005/1000001

[commit-bot: I haz the power, 2014-03-18 18:45:35]

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

[commit-bot: I haz the power, 2014-03-18 18:45:35]

Commit queue rejected this change because the description was changed
between the time the change entered the commit queue and the time it
was ready to commit. You can safely check the commit box again.

[jdduke (slow), 2014-03-18 19:19:53]

The CQ bit was checked by jdduke@chromium.org

[commit-bot: I haz the power, 2014-03-18 19:31:31]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/jdduke@chromium.org/157003005/1000001

[commit-bot: I haz the power, 2014-03-18 20:48:39]

Change committed as 257740