Microoptimisation for OnLibeventNotification.

Microoptimisation for OnLibeventNotification.

OnLibeventNotification created a weak ptr to check that
OnFileCanWriteWithoutBlocking() has not deleted theFileDescriptorWatcher
object.

Creating a WeakPtr performs an allocation.

OnLibeventNotification is called often enough that it accounted for
roughly 5% of the allocations performed by the browser.

The allocation can be avoided by using a boolean variable to
flag whether or not the FileDescriptorWatcher object has been destroyed.

Furthermore, when only one of OnFileCanWriteWithoutBlocking() and
OnFileCanReadWithoutBlocking() is going to be called, it is not necessary to
check whether the FileDescriptorWatcher has been destroyed at all.

Results from running the IPCChannelPerfTest.ChannelPingPong performance
test with and without this change (all times in ms):

IPC_Channel_Perf_50000x_12: 542.8 (s = 6.1) to 524.5 (s = 4.0)
IPC_Channel_Perf_50000x_144: 552.8 (s = 6.8) to 537.2 (s = 5.1)
IPC_Channel_Perf_50000x_1728: 572.5 (s = 6.7) to 553.7 (s = 6.7)
IPC_Channel_Perf_12000x_20736: 305.7 (s = 4.2) to 305.1 (s = 3.2)
IPC_Channel_Perf_1000x_248832: 365.0 (s = 3.7) to 361.3 (s = 2.9)

My test environment was too noisy to demonstrate a statistically
significant improvement on the last two cases, but I think it should be
possible.

BUG=
TEST=base_unittests, net_unittests

Committed: https://crrev.com/9ea8ee354b50ab621ea663636c5fc7a91c8e3226
Cr-Commit-Position: refs/heads/master@{#342076}

[Adam Rice, 2015-07-30 13:05:56]

ricea@chromium.org changed reviewers:
+ hiroshige@chromium.org

[hiroshige, 2015-07-31 06:36:57]

On 2015/07/30 13:05:57, Adam Rice wrote:
Looks working, but can we avoid introducing a custom mechanism for watching
destruction of objects (i.e. by |was_destroyed_|), because using WeakPtr is
easier to read and infer correctness for me.
> However, this check is only needed when both
> OnFileCanWriteWithoutBlocking() and OnFileCanReadWithoutBlocking() are
> going to be called, which is not normally the case.
If this is sufficiently rare, then can the code like below achieve similar
performance gain by completely removing WeakPtr allocation in non-(Read&Write)
cases without introducing |was_destroyed_|?

if (flags & (EV_WRITE | EV_READ)) {
  WeakPtr<FileDescriptorWatcher> weak_controller =
controller->weak_factory_.GetWeakPtr();
  weak_controller->OnFileCanWriteWithoutBlocking();
  if (weak_controller.get())
    weak_controller->OnFileCanReadWithoutBlocking();
} else if (flags & EV_READ) {
  controller->OnFileCanReadWithoutBlocking();
} else if (flags & EV_WRITE) {
  controller->OnFileCanWriteWithoutBlocking();
}

[Adam Rice, 2015-07-31 07:06:40]

On 2015/07/31 06:36:57, hiroshige wrote:
> On 2015/07/30 13:05:57, Adam Rice wrote:
> Looks working, but can we avoid introducing a custom mechanism for watching
> destruction of objects (i.e. by |was_destroyed_|), because using WeakPtr is
> easier to read and infer correctness for me.
> > However, this check is only needed when both
> > OnFileCanWriteWithoutBlocking() and OnFileCanReadWithoutBlocking() are
> > going to be called, which is not normally the case.
> If this is sufficiently rare, then can the code like below achieve similar
> performance gain by completely removing WeakPtr allocation in non-(Read&Write)
> cases without introducing |was_destroyed_|?
> 
> if (flags & (EV_WRITE | EV_READ)) {
>   WeakPtr<FileDescriptorWatcher> weak_controller =
> controller->weak_factory_.GetWeakPtr();
>   weak_controller->OnFileCanWriteWithoutBlocking();
>   if (weak_controller.get())
>     weak_controller->OnFileCanReadWithoutBlocking();
> } else if (flags & EV_READ) {
>   controller->OnFileCanReadWithoutBlocking();
> } else if (flags & EV_WRITE) {
>   controller->OnFileCanWriteWithoutBlocking();
> }

Unfortunately, the case of checking the socket for read and write at the same
time happens when sending large IPC messages. The impact of the extra logic was
enough to cause a performance degradation in the 12000x_20736 and 1000x_248832
cases. So I had to get rid of the memory allocation.

I agree that the boolean destruction flag is unusual and hard to follow.
Unfortunately, the standard "destruction observer" pattern implies memory
allocation
of a destruction observer object, and so would be subject to the same
performance
degradation issue as the WeakPtr implementation.

One alternative would be to make the file_descriptor_watcher_was_destroyed
boolean
a member variable of MessagePumpLibevent. Do you think that would be easier to
read?

[hiroshige, 2015-08-03 08:32:39]

> Unfortunately, the case of checking the socket for read and write at the same
> time happens when sending large IPC messages. The impact of the extra logic
was
> enough to cause a performance degradation in the 12000x_20736 and 1000x_248832
> cases. So I had to get rid of the memory allocation.
> 
> I agree that the boolean destruction flag is unusual and hard to follow.
> Unfortunately, the standard "destruction observer" pattern implies memory
> allocation
> of a destruction observer object, and so would be subject to the same
> performance
> degradation issue as the WeakPtr implementation.

I see. So the fundamental optimization in this CL is removing WeakPtr
allocation, not fast-path optimization for non-(read&write) cases, and the
performance of (read&write)-cases still matters.
Then, it might be better to modify the paragraph below, from which I thought
non-(read&write) cases didn't have much impact.
> However, this check is only needed when both
> OnFileCanWriteWithoutBlocking() and OnFileCanReadWithoutBlocking() are
> going to be called, which is not normally the case.

> One alternative would be to make the file_descriptor_watcher_was_destroyed
> boolean
> a member variable of MessagePumpLibevent. Do you think that would be easier to
> read?
I prefer the current CL (Patch Set 3), because
|FileDescriptorWatcher::was_destroyed_| directly corresponds to
|WeakPtrFactory<FileDescriptorWatcher> FileDescriptorWatcher::weak_factory_|.

lgtm.

[Adam Rice, 2015-08-04 20:43:52]

Sorry to be a nuisance. Could you look at Patch Set 4 and see if you
think it is better or worse than Patch Set 3?

[hiroshige, 2015-08-05 05:02:56]

On 2015/08/04 20:43:52, Adam Rice wrote:
> Sorry to be a nuisance. Could you look at Patch Set 4 and see if you
> think it is better or worse than Patch Set 3?

I think PS4 is better as logic around |checking_for_destruction| is gone and
replaced with simpler |if (!controller_was_destroyed)|.
The call to OnFileCanWriteWithoutBlocking() is duplicated, but this wouldn't be
a problem since the whole code is quite compact (<15 lines).

lgtm PS4.

[Adam Rice, 2015-08-05 23:16:25]

ricea@chromium.org changed reviewers:
+ thestig@chromium.org

[Adam Rice, 2015-08-05 23:16:25]

+thestig, could you review this for //base OWNERS?

[Lei Zhang, 2015-08-05 23:59:58]

Neat find, lgtm.

https://codereview.chromium.org/1255293006/diff/60001/base/message_loop/messa...
File base/message_loop/message_pump_libevent.cc (right):

https://codereview.chromium.org/1255293006/diff/60001/base/message_loop/messa...
base/message_loop/message_pump_libevent.cc:359: if ((flags & (EV_WRITE |
EV_READ)) == (EV_WRITE | EV_READ)) {
silly nit: put EV_READ ahead of EV_WRITE. "R/W" is much more common than "W/R".
As in DVD-RW.

https://codereview.chromium.org/1255293006/diff/60001/base/message_loop/messa...
base/message_loop/message_pump_libevent.cc:368: controller->was_destroyed_ =
NULL;
Please use nullptr in new code. I'm "meh" about line 58 since it's being
consistent with the lines above it.

https://codereview.chromium.org/1255293006/diff/60001/base/message_loop/messa...
File base/message_loop/message_pump_libevent.h (right):

https://codereview.chromium.org/1255293006/diff/60001/base/message_loop/messa...
base/message_loop/message_pump_libevent.h:10: #include "base/memory/weak_ptr.h"
Remove. Though this may cause other parts of the build to fail if they did not
IWYU.

[Adam Rice, 2015-08-06 00:46:46]

https://codereview.chromium.org/1255293006/diff/60001/base/message_loop/messa...
File base/message_loop/message_pump_libevent.cc (right):

https://codereview.chromium.org/1255293006/diff/60001/base/message_loop/messa...
base/message_loop/message_pump_libevent.cc:359: if ((flags & (EV_WRITE |
EV_READ)) == (EV_WRITE | EV_READ)) {
On 2015/08/05 23:59:58, Lei Zhang wrote:
> silly nit: put EV_READ ahead of EV_WRITE. "R/W" is much more common than
"W/R".
> As in DVD-RW.

Best nitpick ever. Done.

https://codereview.chromium.org/1255293006/diff/60001/base/message_loop/messa...
base/message_loop/message_pump_libevent.cc:368: controller->was_destroyed_ =
NULL;
On 2015/08/05 23:59:58, Lei Zhang wrote:
> Please use nullptr in new code. I'm "meh" about line 58 since it's being
> consistent with the lines above it.

Done.

https://codereview.chromium.org/1255293006/diff/60001/base/message_loop/messa...
File base/message_loop/message_pump_libevent.h (right):

https://codereview.chromium.org/1255293006/diff/60001/base/message_loop/messa...
base/message_loop/message_pump_libevent.h:10: #include "base/memory/weak_ptr.h"
On 2015/08/05 23:59:58, Lei Zhang wrote:
> Remove. Though this may cause other parts of the build to fail if they did not
> IWYU.

Done.

[Lei Zhang, 2015-08-06 02:18:21]

++lgtm

[Adam Rice, 2015-08-06 02:26:42]

The CQ bit was checked by ricea@chromium.org

[Adam Rice, 2015-08-06 02:26:42]

The patchset sent to the CQ was uploaded after l-g-t-m from
hiroshige@chromium.org
Link to the patchset: https://codereview.chromium.org/1255293006/#ps80001
(title: "Minor changes requested by thestig@.")

[commit-bot: I haz the power, 2015-08-06 02:27:13]

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

[commit-bot: I haz the power, 2015-08-06 05:25:20]

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

[commit-bot: I haz the power, 2015-08-06 05:25:21]

Try jobs failed on following builders:
  win_chromium_rel_ng on tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)

[Adam Rice, 2015-08-06 05:51:13]

The CQ bit was checked by ricea@chromium.org

[commit-bot: I haz the power, 2015-08-06 05:51:38]

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

[commit-bot: I haz the power, 2015-08-06 07:19:22]

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

[commit-bot: I haz the power, 2015-08-06 07:19:23]

Try jobs failed on following builders:
  win_chromium_rel_ng on tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)

[Lei Zhang, 2015-08-06 07:21:21]

The CQ bit was checked by thestig@chromium.org

[commit-bot: I haz the power, 2015-08-06 07:21:38]

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

[commit-bot: I haz the power, 2015-08-06 08:19:18]

Committed patchset #5 (id:80001)

[commit-bot: I haz the power, 2015-08-06 08:19:57]

Patchset 5 (id:??) landed as
https://crrev.com/9ea8ee354b50ab621ea663636c5fc7a91c8e3226
Cr-Commit-Position: refs/heads/master@{#342076}