Add MediaServiceThrottler

media/base/android/media_service_throttler.cc

Index: media/base/android/media_service_throttler.cc
diff --git a/media/base/android/media_service_throttler.cc b/media/base/android/media_service_throttler.cc
new file mode 100644
index 0000000000000000000000000000000000000000..c0074cad343adda1d780af7817f7648bd889a9ae
--- /dev/null
+++ b/media/base/android/media_service_throttler.cc
@@ -0,0 +1,186 @@
+// Copyright 2016 The Chromium Authors. All rights reserved.
+// Use of this source code is governed by a BSD-style license that can be
+// found in the LICENSE file.
+
+#include "media/base/android/media_service_throttler.h"
+
+#include "base/bind.h"
+#include "base/threading/thread_task_runner_handle.h"
+#include "base/time/default_tick_clock.h"
+#include "media/base/android/media_server_crash_listener.h"
+
+namespace media {
+
+// Elapsed time between crashes needed to completely reset the media server
+// crash count.
+const base::TimeDelta kTimeUntilCrashReset = base::TimeDelta::FromMinutes(1);

[DaleCurtis, 2016/11/08 22:35:41]

constexpr on all base::TimeDelta? Otherwise I think all of these will add
static-initializers.

[sandersd (OOO until July 31), 2016/11/08 22:53:44]

These should probably all be in an anonymous namespace.

[tguilbert, 2016/11/11 03:50:29]

Done.

+
+// Elapsed time between schedule calls needed to completely reset the
+// scheduling clock.
+const base::TimeDelta kTimeUntilScheduleReset = base::TimeDelta::FromMinutes(1);
+
+// Decay rate of server crashes, corresponding to a tolerable 'normal' crash
+// rate. This means that we will decrement our crash rate by ~1 crash/minute.
+const uint kCrashDecayRateInMs = 60000;

[sandersd (OOO until July 31), 2016/11/08 22:53:44]

This is a period (as opposed to a rate).

[tguilbert, 2016/11/11 03:50:29]

Done.

+
+// Rate at which client creations will be exponentially throttled based on the
+// number of media server crashes.
+// NOTE: Since our exponential delay formula is 2^(server crashes), 0 server
+// crashes still result in this delay being added once.

[sandersd (OOO until July 31), 2016/11/08 22:53:44]

An alternative would be to use |n ? 2^(n-1) : 0|. It matches intuition a little
better, but whether that's overall simpler is for you to decide.

[tguilbert, 2016/11/11 03:50:29]

That was how I had originally written it, but I found the code to look somehat
cleaner this way (even though I agree that there is a tradeoff). I might revisit
this decision before the end of the review.

+const base::TimeDelta kBaseExponentialDelay =
+    base::TimeDelta::FromMilliseconds(120);
+
+// Base rate at which we schedule client creations.
+// The minimal delay is |kLinearThrottlingRate| + |kBaseExponentialDelay|.
+// This corresponds to 0.2s.
+const base::TimeDelta kLinearThrottlingRate =
+    base::TimeDelta::FromMilliseconds(80);

[sandersd (OOO until July 31), 2016/11/08 22:53:44]

Also a period ("delay" and other synonyms are fine too).

[tguilbert, 2016/11/11 03:50:29]

Done.

+
+// Max exponential throttling rate from media server crashes.
+// The max delay will still be |kLinearThrottlingRate| + |kMaxExponentialDelay|.
+// This corresponds to 3s.
+const base::TimeDelta kMaxExponentialDelay =
+    base::TimeDelta::FromMilliseconds(2920);
+
+// Max number of clients to schedule immediately (e.g when loading a new page).
+const uint kMaxBurstClients = 10;
+
+// Sliding window of time during which we allow clients to be scheduled
+// immediately, to accomodate for a "bursts" of requests when loading new pages.
+const base::TimeDelta kMinDelayWindow =
+    (kLinearThrottlingRate + kBaseExponentialDelay) * kMaxBurstClients;
+
+// The throttling progression based on number of crashes looks as follows:
+//
+// | # crashes | period  | clients/sec | clients/mins | # burst clients
+// | 0         | 200  ms | 5.0         | 300          | 10
+// | 1         | 320  ms | 3.1         | 188          | 6
+// | 2         | 560  ms | 1.8         | 107          | 4
+// | 3         | 1040 ms | 1.0         | 58           | 2
+// | 4         | 2000 ms | 0.5         | 30           | 1
+// | 5         | 3000 ms | 0.3         | 20           | 1
+// | 6         | 3000 ms | 0.3         | 20           | 1
+//
+// NOTE: Since we use the floor function and a decay rate of 1 crash/minute when
+// calculating the effective # of crashes, a single crash per minute will result
+// in 0 effective crashes (since floor(1.0 - 'tiny decay') is 0). If we
+// experience slightly more than 1 crash per 60 seconds, the effective number of
+// crashes will go up as expected.
+
+// static
+MediaServiceThrottler* MediaServiceThrottler::GetInstance() {
+  return base::Singleton<

[DaleCurtis, 2016/11/08 22:35:41]

Hmm, why do both classes need a singleton? Seems one of them should be able to
own the other? Also, LazyInstance. If possible it'd be great if LazyInstance is
avoidable and instead the lifetime is bound to some existing object.

[tguilbert, 2016/11/11 03:50:29]

Updated. The MediaServiceThrottler is now a lazy instance. The
MediaServiceCrashListener is now managed by the MediaServiceThrottler.

+      MediaServiceThrottler,
+      base::LeakySingletonTraits<MediaServiceThrottler>>::get();
+}
+
+MediaServiceThrottler::~MediaServiceThrottler() {}
+
+MediaServiceThrottler::MediaServiceThrottler()
+    : clock_(new base::DefaultTickClock()), current_crashes_(0.0) {
+  // base::Unretained is safe here because both the MediaServiceThrottler and
+  // the MediaServerCrashListener live until the process is terminated.
+  MediaServerCrashListener::GetInstance()->SetOnServerCrashCallback(
+      base::Bind(&MediaServiceThrottler::OnMediaServerCrash,
+                 base::Unretained(this)),
+      base::ThreadTaskRunnerHandle::Get());
+
+  EnsureCrashListenerStarted();
+}
+
+void MediaServiceThrottler::SetTickClockForTesting(base::TickClock* clock) {
+  clock_.reset(clock);
+}
+
+base::TimeDelta MediaServiceThrottler::GetBaseThrottlingRateForTesting() {
+  return kBaseExponentialDelay + kLinearThrottlingRate;
+}
+
+void MediaServiceThrottler::ResetInternalStateForTesting() {
+  last_server_crash_ = base::TimeTicks();
+  last_schedule_call_ = base::TimeTicks();
+  next_schedulable_slot_ = clock_->NowTicks();
+  last_update_time_ = clock_->NowTicks();
+  current_crashes_ = 0.0;
+}
+
+base::TimeDelta MediaServiceThrottler::ScheduleClientCreation() {

[DaleCurtis, 2016/11/08 22:35:41]

I'd rename this to GetCreationDelay() or something similar. You're not really
scheduling anything.

[tguilbert, 2016/11/11 03:50:29]

Done.

+  // Make sure the listener is started and the crashes decayed.
+  EnsureCrashListenerStarted();
+  UpdateServerCrashes();
+
+  base::TimeTicks now = clock_->NowTicks();
+
+  // If we are passed the next time slot or if it has been 1 minute since the
+  // last call to ScheduleClientCreation(), reset the next time to now.
+  if (now > next_schedulable_slot_ ||

[DaleCurtis, 2016/11/08 22:35:41]

{} for multi-line if.

[tguilbert, 2016/11/11 03:50:29]

Done.

+      (now - last_schedule_call_) > kTimeUntilScheduleReset)
+    next_schedulable_slot_ = now;
+
+  last_schedule_call_ = now;
+
+  // Increment the next scheduled time between 0.2s and 3s, which allows the
+  // creation of between 50 and 3 clients per 10s.
+  next_schedulable_slot_ +=
+      kLinearThrottlingRate + GetThrottlingDelayFromServerCrashes();
+
+  // Calculate how long to delay the creation so it isn't scheduled before
+  // |next_schedulable_slot_|.
+  base::TimeDelta delay = next_schedulable_slot_ - now;
+
+  // If the scheduling delay is low enough, schedule it immediately instead.
+  // This allows up to kMaxBurstClients clients to be scheduled immediately.
+  if (delay <= kMinDelayWindow)
+    return base::TimeDelta();
+
+  return delay;
+}
+
+base::TimeDelta MediaServiceThrottler::GetThrottlingDelayFromServerCrashes() {
+  // The combination of rounding down the number of crashes down and decaying
+  // at the rate of 1 crash / min means that a single crash will very quickly be
+  // rounded down to 0. Effectively, this means that we only start exponentially
+  // backing off if we have more than 1 crash in a 60 second window.
+  uint number_crashes = std::floor(current_crashes_);

[DaleCurtis, 2016/11/08 22:35:41]

No uint. Specify an actual type.

[tguilbert, 2016/11/11 03:50:29]

Done.

+  DCHECK(number_crashes >= 0);

[DaleCurtis, 2016/11/08 22:35:41]

DCHECK_GE

[tguilbert, 2016/11/11 03:50:29]

Done.

+
+  // Prevents overflow/undefined behavior. We already reach kMaxExponentialDelay
+  // at 5 crashes in any case.
+  number_crashes = std::min(number_crashes, (uint)10);

[DaleCurtis, 2016/11/08 22:35:41]

No c style cast. static_cast<> if you need. This should be fine as 10u -- might
want to extract that to a constant.

[tguilbert, 2016/11/11 03:50:29]

Done.

+
+  return std::min(kBaseExponentialDelay * (1 << number_crashes),
+                  kMaxExponentialDelay);
+}
+
+void MediaServiceThrottler::OnMediaServerCrash() {
+  UpdateServerCrashes();
+
+  last_server_crash_ = clock_->NowTicks();
+  current_crashes_ += 1.0;
+}
+
+void MediaServiceThrottler::UpdateServerCrashes() {
+  base::TimeTicks now = clock_->NowTicks();
+  base::TimeDelta time_since_last_crash = now - last_server_crash_;
+
+  if (time_since_last_crash > kTimeUntilCrashReset) {
+    // Reset the number of crashes if we haven't had a crash in the past minute.
+    current_crashes_ = 0.0;

[DaleCurtis, 2016/11/08 22:35:41]

Just 0 is fine.

[tguilbert, 2016/11/11 03:50:29]

Done.

+  } else {
+    // Decay at the rate of 1 crash/minute otherwise.
+    double decay =
+        (now - last_update_time_).InMillisecondsF() / kCrashDecayRateInMs;
+    current_crashes_ = std::max(0.0, current_crashes_ - decay);
+  }
+
+  last_update_time_ = now;
+}
+
+void MediaServiceThrottler::EnsureCrashListenerStarted() {

[DaleCurtis, 2016/11/08 22:35:41]

Seems is_media_server_crash_listener_running_ is kind of unnecessary, just
trigger a StartListening() or similar call during ScheduleClientCreation() (and
queue a shutdown task probably).

[tguilbert, 2016/11/11 03:50:29]

Updated.

+  if (!is_media_server_crash_listener_running_) {
+    is_media_server_crash_listener_running_ =
+        MediaServerCrashListener::GetInstance()->StartListening();
+  }
+}
+
+}  // namespace media