[Mac] Reduce timer CPU use in MessagePumpCFRunLoopBase.

[Mac] Reduce timer CPU use in MessagePumpCFRunLoopBase.

MessagePumpCFRunLoopBase's approach to timers, as recommended by Apple,
leads to two timer reschedulings per timer firing. Timer reschedulings
are quite expensive, and the extra rescheduling can cause Chrome to
consume up to 2x the CPU of Safari on the same page. This change uses
private (but not secret) API to temporarily invalidate a
MessagePumpCFRunLoopBase's CFRunLoopTimer, causing the run loop to bypass
the extra rescheduling.

R=mark@chromium.org, rsesek@chromium.org
BUG=511406
Review-Url: https://codereview.chromium.org/2709813003
Cr-Commit-Position: refs/heads/master@{#452852}
Committed: https://chromium.googlesource.com/chromium/src/+/3a16446d4864b4ad7159df1534357c9cc8ab21d7

[shrike, 2017-02-21 15:25:48]

I found a simpler way to prevent the extra timer reschedulings that consume
extra CPU on each timer firing, that doesn't involve outright replacing
CFRunLoopTimer.

PTAL

[Mark Mentovai, 2017-02-21 17:41:23]

https://codereview.chromium.org/2709813003/diff/1/base/BUILD.gn
File base/BUILD.gn (right):

https://codereview.chromium.org/2709813003/diff/1/base/BUILD.gn#newcode1
base/BUILD.gn:1: # Copyright (c) 2013 The Chromium Authors. All rights reserved.
CL description: MessagePumpCRunLoopBase → MessagePumpCFRunLoopBase

https://codereview.chromium.org/2709813003/diff/1/base/message_loop/message_p...
File base/message_loop/message_pump_mac.mm (right):

https://codereview.chromium.org/2709813003/diff/1/base/message_loop/message_p...
base/message_loop/message_pump_mac.mm:104: void ChromeCFUnsetValid(void* cf) {
Might be better as a single function that takes a “bool valid”.

https://codereview.chromium.org/2709813003/diff/1/base/message_loop/message_p...
base/message_loop/message_pump_mac.mm:119: timer_context.info = NULL;
Use nullptr in new Chrome code whenever you have NULL now.

https://codereview.chromium.org/2709813003/diff/1/base/message_loop/message_p...
base/message_loop/message_pump_mac.mm:170: can_invalidate_timers_(false),
Might as well wrap this and the member variable in the header in a
!defined(OS_IOS) ifdef too, since everything else that would touch it is already
in an #ifdef.

https://codereview.chromium.org/2709813003/diff/1/base/message_loop/message_p...
base/message_loop/message_pump_mac.mm:321: if (can_invalidate_timers_) {
Actually, I think that we can make this unconditional here, both in terms of the
#ifdef and the if(). Since the only thing you do with !defined(OS_IOS) &&
can_invalidate_timers_ is make a call that you otherwise wouldn’t make, you
could centralize everything. Instead of having a can_invalidate_timers_ member,
we could write ChromeCFSetValid() like this:

ChromeCFSetValid(void* cf, bool valid) {
#if !defined(OS_IOS)
  static bool can_invalidate_timers = CanInvalidateTimers();
  if (can_invalidate_timers) {
    __CFBitfieldSetValue(((CFRuntimeBase*)cf)->_cfinfo[CF_INFO_BITS], 3, 3,
valid);
  }
#endif
}

And then here you can just ChromeCFSetValid(delayed_work_timer_, true) without
gnarly conditionals.

https://codereview.chromium.org/2709813003/diff/1/base/message_loop/message_p...
base/message_loop/message_pump_mac.mm:322:
ChromeCFSetValid(delayed_work_timer_);
Should this happen before (as done here) or after setting the next fire date? Is
one way more efficient? Might one way cause a premature fire? If any of this is
relevant, do things in the proper order and include a comment explaining the
significance.

https://codereview.chromium.org/2709813003/diff/1/base/message_loop/message_p...
File base/message_loop/message_pump_mac_unittest.cc (right):

https://codereview.chromium.org/2709813003/diff/1/base/message_loop/message_p...
base/message_loop/message_pump_mac_unittest.cc:22: // Catch if the use of
private API ever starts failing.
whether

[shrike, 2017-02-21 17:54:00]

Description was changed from

==========
[Mac] Reduce timer CPU use in MessagePumpCRunLoopBase.

MessagePumpCRunLoopBase's approach to timers, as recommended by Apple,
leads to two timer reschedulings per timer firing. Timer reschedulings
are quite expensive, and the extra rescheduling can cause Chrome to
consume up to 2x the CPU of Safari on the same page. This change uses
private (but not secret) API to temporarily invalidate a
MessagePumpCRunLoopBase's CFRunLoopTimer, causing the run loop to bypass
the extra rescheduling.

R=mark@chromium.org, rsesek@chromium.org
BUG=511406
==========

to

==========
[Mac] Reduce timer CPU use in MessagePumpCFRunLoopBase.

MessagePumpCRunLoopBase's approach to timers, as recommended by Apple,
leads to two timer reschedulings per timer firing. Timer reschedulings
are quite expensive, and the extra rescheduling can cause Chrome to
consume up to 2x the CPU of Safari on the same page. This change uses
private (but not secret) API to temporarily invalidate a
MessagePumpCRunLoopBase's CFRunLoopTimer, causing the run loop to bypass
the extra rescheduling.

R=mark@chromium.org, rsesek@chromium.org
BUG=511406
==========

[shrike, 2017-02-21 17:54:14]

Description was changed from

==========
[Mac] Reduce timer CPU use in MessagePumpCFRunLoopBase.

MessagePumpCRunLoopBase's approach to timers, as recommended by Apple,
leads to two timer reschedulings per timer firing. Timer reschedulings
are quite expensive, and the extra rescheduling can cause Chrome to
consume up to 2x the CPU of Safari on the same page. This change uses
private (but not secret) API to temporarily invalidate a
MessagePumpCRunLoopBase's CFRunLoopTimer, causing the run loop to bypass
the extra rescheduling.

R=mark@chromium.org, rsesek@chromium.org
BUG=511406
==========

to

==========
[Mac] Reduce timer CPU use in MessagePumpCFRunLoopBase.

MessagePumpCFRunLoopBase's approach to timers, as recommended by Apple,
leads to two timer reschedulings per timer firing. Timer reschedulings
are quite expensive, and the extra rescheduling can cause Chrome to
consume up to 2x the CPU of Safari on the same page. This change uses
private (but not secret) API to temporarily invalidate a
MessagePumpCFRunLoopBase's CFRunLoopTimer, causing the run loop to bypass
the extra rescheduling.

R=mark@chromium.org, rsesek@chromium.org
BUG=511406
==========

[Mark Mentovai, 2017-02-21 18:10:57]

https://codereview.chromium.org/2709813003/diff/1/base/message_loop/message_p...
File base/message_loop/message_pump_mac.mm (right):

https://codereview.chromium.org/2709813003/diff/1/base/message_loop/message_p...
base/message_loop/message_pump_mac.mm:113: // 2008 in CF-476).
Call out 10.5 by name too.

But this is kind of funny, because we haven’t seen a CF source release in a
couple of years, so we can’t easily verify that this is true.

The CanInvalidateTimers() check below is good for making sure that the bit that
we think represents validity is used for validity. What it doesn’t do is check
whether that bit is the source of truth for validity. My concern is that CF
could change so that the struct is still the same and the bit is still the same,
but internally, it would (for example) make a timer-enabling system call when
setting that bit, and a disabling one when clearing it.

I think that if you added a test that made sure that a
would-fire-but-for-our-invalidation timer doesn’t actually fire, and that it
starts firing again once we set it to valid. In other words,
CFRunLoopTimerIsValid() just checks the bit that we’re setting, but we should be
making sure that there isn’t anything else that we’re missing.

A check for this might be too much or too heavy for CanInvalidateTimers(), but
at the very least, we should be able to have a unit test.

[shrike, 2017-02-22 00:57:53]

PTAL

https://codereview.chromium.org/2709813003/diff/1/base/BUILD.gn
File base/BUILD.gn (right):

https://codereview.chromium.org/2709813003/diff/1/base/BUILD.gn#newcode1
base/BUILD.gn:1: # Copyright (c) 2013 The Chromium Authors. All rights reserved.
On 2017/02/21 17:41:22, Mark Mentovai wrote:
> CL description: MessagePumpCRunLoopBase → MessagePumpCFRunLoopBase

Done.

https://codereview.chromium.org/2709813003/diff/1/base/message_loop/message_p...
File base/message_loop/message_pump_mac.mm (right):

https://codereview.chromium.org/2709813003/diff/1/base/message_loop/message_p...
base/message_loop/message_pump_mac.mm:104: void ChromeCFUnsetValid(void* cf) {
On 2017/02/21 17:41:22, Mark Mentovai wrote:
> Might be better as a single function that takes a “bool valid”.

Acknowledged.

https://codereview.chromium.org/2709813003/diff/1/base/message_loop/message_p...
base/message_loop/message_pump_mac.mm:113: // 2008 in CF-476).
On 2017/02/21 18:10:57, Mark Mentovai wrote:
> Call out 10.5 by name too.
> 
> But this is kind of funny, because we haven’t seen a CF source release in a
> couple of years, so we can’t easily verify that this is true.
> 
> The CanInvalidateTimers() check below is good for making sure that the bit
that
> we think represents validity is used for validity. What it doesn’t do is check
> whether that bit is the source of truth for validity. My concern is that CF
> could change so that the struct is still the same and the bit is still the
same,
> but internally, it would (for example) make a timer-enabling system call when
> setting that bit, and a disabling one when clearing it.
> 
> I think that if you added a test that made sure that a
> would-fire-but-for-our-invalidation timer doesn’t actually fire, and that it
> starts firing again once we set it to valid. In other words,
> CFRunLoopTimerIsValid() just checks the bit that we’re setting, but we should
be
> making sure that there isn’t anything else that we’re missing.
> 
> A check for this might be too much or too heavy for CanInvalidateTimers(), but
> at the very least, we should be able to have a unit test.

I added a unit test to make sure there's no side effect of twiddling the bit,
but this test does not check for "would-fire-but-for-our-invalidation timer
doesn’t actually fire". Preventing the timer from firing is not actually a goal
of the code, and it should never be the case that the timer is being told to
fire but we're using the invalid bit to prevent that. Also, while I 100% agree
there's value in testing for side effects, I don't see how a timer-enabling
system call can occur by flipping a bit. I can see that happening if I call some
CF function (and in fact CFRunLoopTimerInvalidate() does this), but not if I
flip a bit in memory using code that's solely within Chrome.

Re: CF, what do you mean about 10.5 (was that when the struct changed before)?

I also corrected the comment about source changes. I looked at all of the
instances of CFInternals.h (or whatever it was) that I could find, and I saw
that there was open source up to 10.12, but didn't notice that there was no CF
source for the last two releases of macOS :-/.

https://codereview.chromium.org/2709813003/diff/1/base/message_loop/message_p...
base/message_loop/message_pump_mac.mm:119: timer_context.info = NULL;
On 2017/02/21 17:41:22, Mark Mentovai wrote:
> Use nullptr in new Chrome code whenever you have NULL now.

Thank you. Fixed.

https://codereview.chromium.org/2709813003/diff/1/base/message_loop/message_p...
base/message_loop/message_pump_mac.mm:170: can_invalidate_timers_(false),
On 2017/02/21 17:41:22, Mark Mentovai wrote:
> Might as well wrap this and the member variable in the header in a
> !defined(OS_IOS) ifdef too, since everything else that would touch it is
already
> in an #ifdef.

This has been removed, per your feedback below.

https://codereview.chromium.org/2709813003/diff/1/base/message_loop/message_p...
base/message_loop/message_pump_mac.mm:321: if (can_invalidate_timers_) {
On 2017/02/21 17:41:22, Mark Mentovai wrote:
> Actually, I think that we can make this unconditional here, both in terms of
the
> #ifdef and the if(). Since the only thing you do with !defined(OS_IOS) &&
> can_invalidate_timers_ is make a call that you otherwise wouldn’t make, you
> could centralize everything. Instead of having a can_invalidate_timers_
member,
> we could write ChromeCFSetValid() like this:
> 
> ChromeCFSetValid(void* cf, bool valid) {
> #if !defined(OS_IOS)
>   static bool can_invalidate_timers = CanInvalidateTimers();
>   if (can_invalidate_timers) {
>     __CFBitfieldSetValue(((CFRuntimeBase*)cf)->_cfinfo[CF_INFO_BITS], 3, 3,
> valid);
>   }
> #endif
> }
> 
> And then here you can just ChromeCFSetValid(delayed_work_timer_, true) without
> gnarly conditionals.

Got it. That's clean :-).

https://codereview.chromium.org/2709813003/diff/1/base/message_loop/message_p...
base/message_loop/message_pump_mac.mm:322:
ChromeCFSetValid(delayed_work_timer_);
On 2017/02/21 17:41:22, Mark Mentovai wrote:
> Should this happen before (as done here) or after setting the next fire date?
Is
> one way more efficient? Might one way cause a premature fire? If any of this
is
> relevant, do things in the proper order and include a comment explaining the
> significance.

Done.

https://codereview.chromium.org/2709813003/diff/1/base/message_loop/message_p...
File base/message_loop/message_pump_mac_unittest.cc (right):

https://codereview.chromium.org/2709813003/diff/1/base/message_loop/message_p...
base/message_loop/message_pump_mac_unittest.cc:22: // Catch if the use of
private API ever starts failing.
On 2017/02/21 17:41:22, Mark Mentovai wrote:
> whether

Done.

[Mark Mentovai, 2017-02-23 02:34:37]

LGTM

https://codereview.chromium.org/2709813003/diff/1/base/message_loop/message_p...
File base/message_loop/message_pump_mac.mm (right):

https://codereview.chromium.org/2709813003/diff/1/base/message_loop/message_p...
base/message_loop/message_pump_mac.mm:113: // 2008 in CF-476).
On 2017/02/22 00:57:52, shrike wrote:
> Also, while I 100% agree
> there's value in testing for side effects, I don't see how a timer-enabling
> system call can occur by flipping a bit. I can see that happening if I call
some
> CF function (and in fact CFRunLoopTimerInvalidate() does this), but not if I
> flip a bit in memory using code that's solely within Chrome.

I’m mean that we’re flipping an internal bit that will perhaps come to be not
meant to be flipped without a corresponding system call. That’s a way that this
might break down that we wouldn’t easily see because the test would still pass.
We’re only flipping the bit, we’re not making the hypothetical system call. We
know that this is fine today and in the CF source that we can see, but the point
of the test is to check that we can continue abusing undocumented and
subject-to-change internals in the way we’ve come to expect.

But I also see that the way you’re using it now, it shouldn’t be a problem for
our particular use. Probably the worst invisible thing that would happen in the
hypothetical future I’m positing would be that the invalidation just doesn’t
work, even though it looks like it works, and we wind up in the same situation
that we’re in today, before this lands.

> Re: CF, what do you mean about 10.5 (was that when the struct changed before)?

I meant that CF-476 was form 10.5. It puts the version number into context.

[shrike, 2017-02-23 17:51:11]

https://codereview.chromium.org/2709813003/diff/1/base/message_loop/message_p...
File base/message_loop/message_pump_mac.mm (right):

https://codereview.chromium.org/2709813003/diff/1/base/message_loop/message_p...
base/message_loop/message_pump_mac.mm:113: // 2008 in CF-476).
On 2017/02/23 02:34:36, Mark Mentovai wrote:
> On 2017/02/22 00:57:52, shrike wrote:
> > Also, while I 100% agree
> > there's value in testing for side effects, I don't see how a timer-enabling
> > system call can occur by flipping a bit. I can see that happening if I call
> some
> > CF function (and in fact CFRunLoopTimerInvalidate() does this), but not if I
> > flip a bit in memory using code that's solely within Chrome.
> 
> I’m mean that we’re flipping an internal bit that will perhaps come to be not
> meant to be flipped without a corresponding system call. That’s a way that
this
> might break down that we wouldn’t easily see because the test would still
pass.
> We’re only flipping the bit, we’re not making the hypothetical system call. We
> know that this is fine today and in the CF source that we can see, but the
point
> of the test is to check that we can continue abusing undocumented and
> subject-to-change internals in the way we’ve come to expect.

OK, I agree that there is a small chance of what you're saying. Currently full
invalidation does involve a system call which ends up preventing the timer's
reuse in the future, bit or no bit. So when you were saying that we might miss
out on a system call I was thinking we already are missing out on a system call
and any system call that would occur would be counter to our purposes, but it is
conceivable that in the future, invalidation consists of flipping the bit and
doing something else that is required for the timer to be at all reusable by us.

> > Re: CF, what do you mean about 10.5 (was that when the struct changed
before)?
> 
> I meant that CF-476 was form 10.5. It puts the version number into context.

Thank you (I wasn't sure what release CF-476 corresponded to). I will add that
note to the comment.

What is your sense of the risk with landing this patch one week before branch?
The very conservative part of me wants to wait until after branch to get maximum
air time, but if this patch has the effect I expect, we will see dramatic
reductions in CPU use that would be great to get in ASAP.

[Robert Sesek, 2017-02-23 18:06:05]

LGTM

On 2017/02/23 17:51:11, shrike wrote:
> What is your sense of the risk with landing this patch one week before branch?
> The very conservative part of me wants to wait until after branch to get
maximum
> air time, but if this patch has the effect I expect, we will see dramatic
> reductions in CPU use that would be great to get in ASAP.

I'm not too concerned about risk here, since we can verify on all OSes that we
run on currently, and we don't have a new OS on the horizon for the next few
Mstones. I checked the disassembly of CFRunLoopTimerIsValid() on 10.10.4, 10.11,
and 10.12.2 and it hasn't changed at all.

[Mark Mentovai, 2017-02-23 18:25:25]

I’m also not worried about the risk. We know what all of the shipping OSes look
like. It’ll either work or not work.

This kind of thing is scarier for OSes that we haven’t seen yet, but we’re many
months away from having to worry about that.

[shrike, 2017-02-23 18:34:48]

On 2017/02/23 18:25:25, Mark Mentovai wrote:
> I’m also not worried about the risk. We know what all of the shipping OSes
look
> like. It’ll either work or not work.
> 
> This kind of thing is scarier for OSes that we haven’t seen yet, but we’re
many
> months away from having to worry about that.

OK, thanks to both of you.

[shrike, 2017-02-23 18:34:55]

The CQ bit was checked by shrike@chromium.org

[shrike, 2017-02-23 18:34:55]

The patchset sent to the CQ was uploaded after l-g-t-m from mark@chromium.org,
rsesek@chromium.org
Link to the patchset: https://codereview.chromium.org/2709813003/#ps80001
(title: "Restore comment, fix typos.")

[commit-bot: I haz the power, 2017-02-23 18:35:23]

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, 2017-02-23 18:46:35]

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

[commit-bot: I haz the power, 2017-02-23 18:46:37]

Try jobs failed on following builders:
  ios-device on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device/builds...)
  ios-simulator-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator-xco...)

[shrike, 2017-02-23 22:05:33]

The CQ bit was checked by shrike@chromium.org

[shrike, 2017-02-23 22:05:33]

The patchset sent to the CQ was uploaded after l-g-t-m from mark@chromium.org,
rsesek@chromium.org
Link to the patchset: https://codereview.chromium.org/2709813003/#ps100001
(title: "Fix iOS compile problems.")

[commit-bot: I haz the power, 2017-02-23 22:06:27]

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, 2017-02-24 00:16:33]

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

[commit-bot: I haz the power, 2017-02-24 00:16:35]

Try jobs failed on following builders:
  linux_chromium_tsan_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[shrike, 2017-02-24 16:59:55]

The CQ bit was checked by shrike@chromium.org

[commit-bot: I haz the power, 2017-02-24 17:00:24]

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, 2017-02-24 17:05:23]

CQ is committing da patch.

Bot data: {"patchset_id": 100001, "attempt_start_ts": 1487955595222950,
"parent_rev": "a72385a6addeb6194d3568beda3f352e07c97260", "commit_rev":
"3a16446d4864b4ad7159df1534357c9cc8ab21d7"}

[commit-bot: I haz the power, 2017-02-24 17:06:01]

Description was changed from

==========
[Mac] Reduce timer CPU use in MessagePumpCFRunLoopBase.

MessagePumpCFRunLoopBase's approach to timers, as recommended by Apple,
leads to two timer reschedulings per timer firing. Timer reschedulings
are quite expensive, and the extra rescheduling can cause Chrome to
consume up to 2x the CPU of Safari on the same page. This change uses
private (but not secret) API to temporarily invalidate a
MessagePumpCFRunLoopBase's CFRunLoopTimer, causing the run loop to bypass
the extra rescheduling.

R=mark@chromium.org, rsesek@chromium.org
BUG=511406
==========

to

==========
[Mac] Reduce timer CPU use in MessagePumpCFRunLoopBase.

MessagePumpCFRunLoopBase's approach to timers, as recommended by Apple,
leads to two timer reschedulings per timer firing. Timer reschedulings
are quite expensive, and the extra rescheduling can cause Chrome to
consume up to 2x the CPU of Safari on the same page. This change uses
private (but not secret) API to temporarily invalidate a
MessagePumpCFRunLoopBase's CFRunLoopTimer, causing the run loop to bypass
the extra rescheduling.

R=mark@chromium.org, rsesek@chromium.org
BUG=511406

Review-Url: https://codereview.chromium.org/2709813003
Cr-Commit-Position: refs/heads/master@{#452852}
Committed:
https://chromium.googlesource.com/chromium/src/+/3a16446d4864b4ad7159df153435...
==========

[commit-bot: I haz the power, 2017-02-24 17:06:02]

Committed patchset #6 (id:100001) as
https://chromium.googlesource.com/chromium/src/+/3a16446d4864b4ad7159df153435...