[Mac] Reduce timer CPU use in MessagePumpCFRunLoopBase.

base/message_loop/message_pump_mac.mm

Index: base/message_loop/message_pump_mac.mm
diff --git a/base/message_loop/message_pump_mac.mm b/base/message_loop/message_pump_mac.mm
index d924ead7cbd9b934341ab6314d059fe769c172fa..5556be3f6cd4ff94deb8034493c6e4e7f3a2bf80 100644
--- a/base/message_loop/message_pump_mac.mm
+++ b/base/message_loop/message_pump_mac.mm
@@ -69,6 +69,74 @@ const CFTimeInterval kCFTimeIntervalMax =
 // Set to true if MessagePumpMac::Create() is called before NSApp is
 // initialized.  Only accessed from the main thread.
 bool g_not_using_cr_app = false;
+
+// Various CoreFoundation definitions.
+typedef struct __CFRuntimeBase {
+  uintptr_t _cfisa;
+  uint8_t _cfinfo[4];
+#if __LP64__
+  uint32_t _rc;
+#endif
+} CFRuntimeBase;
+
+#if defined(__BIG_ENDIAN__)
+#define __CF_BIG_ENDIAN__ 1
+#define __CF_LITTLE_ENDIAN__ 0
+#endif
+
+#if defined(__LITTLE_ENDIAN__)
+#define __CF_LITTLE_ENDIAN__ 1
+#define __CF_BIG_ENDIAN__ 0
+#endif
+
+#define CF_INFO_BITS (!!(__CF_BIG_ENDIAN__)*3)
+
+#define __CFBitfieldMask(N1, N2) \
+  ((((UInt32)~0UL) << (31UL - (N1) + (N2))) >> (31UL - N1))
+#define __CFBitfieldSetValue(V, N1, N2, X)   \
+  ((V) = ((V) & ~__CFBitfieldMask(N1, N2)) | \
+         (((X) << (N2)) & __CFBitfieldMask(N1, N2)))
+
+void ChromeCFSetValid(void* cf) {
+  __CFBitfieldSetValue(((CFRuntimeBase*)cf)->_cfinfo[CF_INFO_BITS], 3, 3, 1);
+}
+
+void ChromeCFUnsetValid(void* cf) {

[Mark Mentovai, 2017/02/21 17:41:22]

Might be better as a single function that takes a “bool valid”.

[shrike, 2017/02/22 00:57:52]

Acknowledged.

+  __CFBitfieldSetValue(((CFRuntimeBase*)cf)->_cfinfo[CF_INFO_BITS], 3, 3, 0);
+}
+
+// Marking timers as invalid at the right time helps significantly reduce power
+// use (see the explanation in RunDelayedWorkTimer()), however there is no
+// public API for doing so. CFRuntime.h states that CFRuntimeBase, upon which
+// the above timer invalidation functions are based, can change from release to
+// release and should not be accessed directly (this struct last changed in
+// 2008 in CF-476).

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

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:52]

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 :-/.

[Mark Mentovai, 2017/02/23 02:34:36]

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.
I meant that CF-476 was form 10.5. It puts the version number into context.

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

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.
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.

+//
+// This function uses private API to modify a test timer's valid state and
+// uses public API to confirm that the private API changed the right bit.
+bool CanInvalidateTimers() {
+  CFRunLoopTimerContext timer_context = CFRunLoopTimerContext();
+  timer_context.info = NULL;

[Mark Mentovai, 2017/02/21 17:41:22]

Use nullptr in new Chrome code whenever you have NULL now.

[shrike, 2017/02/22 00:57:52]

Thank you. Fixed.

+  ScopedCFTypeRef<CFRunLoopTimerRef> test_timer(
+      CFRunLoopTimerCreate(NULL,                // allocator
+                           kCFTimeIntervalMax,  // fire time
+                           kCFTimeIntervalMax,  // interval
+                           0,                   // flags
+                           0,                   // priority
+                           NULL, &timer_context));
+  // Should be valid from the start.
+  if (!CFRunLoopTimerIsValid(test_timer)) {
+    return false;
+  }
+  // Confirm that the private API can mark the timer invalid.
+  ChromeCFUnsetValid(test_timer);
+  if (CFRunLoopTimerIsValid(test_timer)) {
+    return false;
+  }
+  // Confirm that the private API can mark the timer valid.
+  ChromeCFSetValid(test_timer);
+  return CFRunLoopTimerIsValid(test_timer);
+}
 #endif
 
 }  // namespace
@@ -99,11 +167,16 @@ MessagePumpCFRunLoopBase::MessagePumpCFRunLoopBase()
     : delegate_(NULL),
       delayed_work_fire_time_(kCFTimeIntervalMax),
       timer_slack_(base::TIMER_SLACK_NONE),
+      can_invalidate_timers_(false),

[Mark Mentovai, 2017/02/21 17:41:22]

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.

[shrike, 2017/02/22 00:57:52]

This has been removed, per your feedback below.

       nesting_level_(0),
       run_nesting_level_(0),
       deepest_nesting_level_(0),
       delegateless_work_(false),
       delegateless_idle_work_(false) {
+#if !defined(OS_IOS)
+  can_invalidate_timers_ = CanInvalidateTimers();
+#endif  // !defined(OS_IOS)
+
   run_loop_ = CFRunLoopGetCurrent();
   CFRetain(run_loop_);
 
@@ -243,6 +316,12 @@ void MessagePumpCFRunLoopBase::ScheduleDelayedWork(
     const TimeTicks& delayed_work_time) {
   TimeDelta delta = delayed_work_time - TimeTicks::Now();
   delayed_work_fire_time_ = CFAbsoluteTimeGetCurrent() + delta.InSecondsF();
+#if !defined(OS_IOS)
+  // Please see the comment in RunDelayedWorkTimer() for more info.
+  if (can_invalidate_timers_) {

[Mark Mentovai, 2017/02/21 17:41:22]

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.

[shrike, 2017/02/22 00:57:52]

Got it. That's clean :-).

+    ChromeCFSetValid(delayed_work_timer_);

[Mark Mentovai, 2017/02/21 17:41:22]

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.

[shrike, 2017/02/22 00:57:52]

Done.

+  }
+#endif  // !defined(OS_IOS)
   CFRunLoopTimerSetNextFireDate(delayed_work_timer_, delayed_work_fire_time_);
   if (timer_slack_ == TIMER_SLACK_MAXIMUM) {
     CFRunLoopTimerSetTolerance(delayed_work_timer_, delta.InSecondsF() * 0.5);
@@ -264,6 +343,35 @@ void MessagePumpCFRunLoopBase::RunDelayedWorkTimer(CFRunLoopTimerRef timer,
   // The timer won't fire again until it's reset.
   self->delayed_work_fire_time_ = kCFTimeIntervalMax;
 
+#if !defined(OS_IOS)
+  // The message pump's timer needs to fire at changing and unpredictable
+  // intervals. Creating a new timer for each firing time is very expensive, so
+  // the message pump instead uses a repeating timer with a very large repeat
+  // rate. After each firing of the timer, the run loop sets the timer's next
+  // firing time to the distant future, essentially pausing the timer until the
+  // pump sets the next firing time. This is the solution recommended by Apple.
+  //
+  // It turns out, however, that scheduling timers is also quite expensive, and
+  // that every one of the message pump's timer firings incurs two
+  // reschedulings. The first rescheduling occurs in ScheduleDelayedWork(),
+  // which sets the desired next firing time. The second comes after exiting
+  // this method (the timer's callback method), when the run loop sets the
+  // timer's next firing time to far in the future.
+  //
+  // The code in __CFRunLoopDoTimer() inside CFRunLoop.c calls the timer's
+  // callback, confirms that the timer is valid, and then sets its future
+  // firing time based on its repeat frequency. Flipping the valid bit here
+  // causes the __CFRunLoopDoTimer() to skip setting the future firing time.
+  // Note that there's public API to invaildate a timer but it goes beyond
+  // flipping the valid bit, making the timer unusable in the future.
+  //
+  // ScheduleDelayedWork() flips the valid bit back just before setting the
+  // timer's new firing time.
+  if (self->can_invalidate_timers_) {
+    ChromeCFUnsetValid(self->delayed_work_timer_);
+  }
+#endif  // !defined(OS_IOS)
+
   // CFRunLoopTimers fire outside of the priority scheme for CFRunLoopSources.
   // In order to establish the proper priority in which work and delayed work
   // are processed one for one, the timer used to schedule delayed work must