Cache vectors to avoid heap allocations on each message pump iteration.

Cache vectors to avoid heap allocations on each message pump iteration.

In Chrome, MessagePumpMojo::DoInternalWork causes ~50,000 heap
allocations when loading The Verge. This change drops that to ~5000,
measured on a debug build.

On each loop, WaitState has two vectors which are allocated, and resized
multiple times. Caching the WaitState between iterations largely
eliminates those allocations.

The story with |cloned_handlers| is a bit different. HandleToHandler is
a hash map, and GCC's implementation does a heap allocation for every
new element added. That means at least handlers_.size() allocations on
every iteration. This replaces that with a single allocaiton in a
vector.

Chrome's MojoChannelPerfTest doesn't show a significant improvement.

Showing (min/max/mean/median/stddev) in ms per run over 10 runs:
IPC_Channel_Perf_50000x_12
before
1016 1046 1030 1030 7.5
after
998 1032 1013 1011 10.3

IPC_ChannelProxy_Perf_50000x_12
before
1804 3809 2633 2486 637
after
2019 3690 2625 2413 489

And in case you're wondering, the remaining ~5000 heap allocations are
mostly in mojo::system::Core::WaitMany().

R=qsr@chromium.org

Committed: https://chromium.googlesource.com/external/mojo/+/ecf671725e139e27d8e07073e0722d10829e28c7

[Anand Mistry (off Chromium), 2015-05-28 08:22:18]

amistry@chromium.org changed reviewers:
+ abarth@chromium.org, qsr@chromium.org

[Anand Mistry (off Chromium), 2015-05-28 08:22:19]

abarth@chromium.org, qsr@chromium.org: Sending to you since you made the last
significant change to this code.

I don't know whether this is acceptable or not, but I figured I'd send it out
anyways. I was looking at the worst heap allocators in Chrome, loading a single
site. This is close to the top.

[qsr, 2015-05-28 09:33:19]

qsr@chromium.org changed reviewers:
+ viettrungluu@chromium.org

[qsr, 2015-05-28 09:33:37]

On 2015/05/28 08:22:19, Anand Mistry wrote:
> mailto:abarth@chromium.org, mailto:qsr@chromium.org: Sending to you since you
made the last
> significant change to this code.
> 
> I don't know whether this is acceptable or not, but I figured I'd send it out
> anyways. I was looking at the worst heap allocators in Chrome, loading a
single
> site. This is close to the top.

+vtl as I think he is the original writer of this code.

Overall, this seems fine to me, but wait for Trung to comment.

[qsr, 2015-05-28 09:33:48]

https://codereview.chromium.org/1156923005/diff/1/mojo/common/message_pump_mo...
File mojo/common/message_pump_mojo.cc (right):

https://codereview.chromium.org/1156923005/diff/1/mojo/common/message_pump_mo...
mojo/common/message_pump_mojo.cc:225: 2 *
run_state_->wait_state->wait_signals.size()) {
Not sure the || is needed here. The 2 vectors should follow the same pattern, or
do we expect some randomization on vector about when it does its resizing?

https://codereview.chromium.org/1156923005/diff/1/mojo/common/message_pump_mo...
mojo/common/message_pump_mojo.cc:233:
run_state_->cloned_handlers->reserve(handlers_.size());
Isn't the reserve always useful, whether the list is new or not?

https://codereview.chromium.org/1156923005/diff/1/mojo/common/message_pump_mo...
mojo/common/message_pump_mojo.cc:295: wait_state->wait_signals.clear();
Do you want to use resize here?

https://codereview.chromium.org/1156923005/diff/1/mojo/common/message_pump_mo...
File mojo/common/message_pump_mojo.h (right):

https://codereview.chromium.org/1156923005/diff/1/mojo/common/message_pump_mo...
mojo/common/message_pump_mojo.h:86: typedef std::vector<std::pair<Handle,
Handler>> HandleToHandlerList;
This doesn't seem to be needed in the .h file.

[viettrungluu, 2015-05-28 17:26:54]

On 2015/05/28 09:33:37, qsr wrote:
> On 2015/05/28 08:22:19, Anand Mistry wrote:
> > mailto:abarth@chromium.org, mailto:qsr@chromium.org: Sending to you since
you
> made the last
> > significant change to this code.
> > 
> > I don't know whether this is acceptable or not, but I figured I'd send it
out
> > anyways. I was looking at the worst heap allocators in Chrome, loading a
> single
> > site. This is close to the top.
> 
> +vtl as I think he is the original writer of this code.

I think sky originally wrote this.

> 
> Overall, this seems fine to me, but wait for Trung to comment.

[Anand Mistry (off Chromium), 2015-05-28 23:27:45]

https://codereview.chromium.org/1156923005/diff/1/mojo/common/message_pump_mo...
File mojo/common/message_pump_mojo.cc (right):

https://codereview.chromium.org/1156923005/diff/1/mojo/common/message_pump_mo...
mojo/common/message_pump_mojo.cc:225: 2 *
run_state_->wait_state->wait_signals.size()) {
On 2015/05/28 09:33:48, qsr wrote:
> Not sure the || is needed here. The 2 vectors should follow the same pattern,
or
> do we expect some randomization on vector about when it does its resizing?

Good point. They should be in sync.

https://codereview.chromium.org/1156923005/diff/1/mojo/common/message_pump_mo...
mojo/common/message_pump_mojo.cc:233:
run_state_->cloned_handlers->reserve(handlers_.size());
On 2015/05/28 09:33:48, qsr wrote:
> Isn't the reserve always useful, whether the list is new or not?

Done.

https://codereview.chromium.org/1156923005/diff/1/mojo/common/message_pump_mo...
mojo/common/message_pump_mojo.cc:295: wait_state->wait_signals.clear();
On 2015/05/28 09:33:48, qsr wrote:
> Do you want to use resize here?

Nah. That would change the logic here and I don't think it's worth the
insignificant performance gain.

https://codereview.chromium.org/1156923005/diff/1/mojo/common/message_pump_mo...
File mojo/common/message_pump_mojo.h (right):

https://codereview.chromium.org/1156923005/diff/1/mojo/common/message_pump_mo...
mojo/common/message_pump_mojo.h:86: typedef std::vector<std::pair<Handle,
Handler>> HandleToHandlerList;
On 2015/05/28 09:33:48, qsr wrote:
> This doesn't seem to be needed in the .h file.

Handler is a private member, so the compiler barfs when I try to move it out.

[abarth-chromium, 2015-05-28 23:32:17]

abarth@chromium.org changed reviewers:
- abarth@chromium.org

[abarth-chromium, 2015-05-28 23:32:17]

I'm not the right reviewer for this change.

[qsr, 2015-05-29 07:56:08]

LGTM with optional nit.

https://codereview.chromium.org/1156923005/diff/1/mojo/common/message_pump_mo...
File mojo/common/message_pump_mojo.cc (right):

https://codereview.chromium.org/1156923005/diff/1/mojo/common/message_pump_mo...
mojo/common/message_pump_mojo.cc:295: wait_state->wait_signals.clear();
On 2015/05/28 23:27:45, Anand Mistry wrote:
> On 2015/05/28 09:33:48, qsr wrote:
> > Do you want to use resize here?
> 
> Nah. That would change the logic here and I don't think it's worth the
> insignificant performance gain.

Sorry, I meant reserve, not resize. That will not change the logic and ensure
you won't have any allocation while doing push_back.

[Anand Mistry (off Chromium), 2015-05-29 09:00:57]

https://codereview.chromium.org/1156923005/diff/1/mojo/common/message_pump_mo...
File mojo/common/message_pump_mojo.cc (right):

https://codereview.chromium.org/1156923005/diff/1/mojo/common/message_pump_mo...
mojo/common/message_pump_mojo.cc:295: wait_state->wait_signals.clear();
On 2015/05/29 07:56:08, qsr wrote:
> On 2015/05/28 23:27:45, Anand Mistry wrote:
> > On 2015/05/28 09:33:48, qsr wrote:
> > > Do you want to use resize here?
> > 
> > Nah. That would change the logic here and I don't think it's worth the
> > insignificant performance gain.
> 
> Sorry, I meant reserve, not resize. That will not change the logic and ensure
> you won't have any allocation while doing push_back.

Done.

[Anand Mistry (off Chromium), 2015-06-01 04:21:18]

vtl: Any comments?

[Anand Mistry (off Chromium), 2015-06-03 03:07:49]

Committed patchset #4 (id:60001) manually as
ecf671725e139e27d8e07073e0722d10829e28c7 (presubmit successful).