Reland "Throttle resource message requests during user interaction"

content/renderer/scheduler/throttled_message_sender.cc

+// Copyright 2015 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 "content/renderer/scheduler/throttled_message_sender.h"
+
+#include "base/auto_reset.h"
+#include "content/renderer/scheduler/renderer_scheduler.h"
+#include "ipc/ipc_message.h"
+
+namespace content {
+namespace {
+
+const int kReasonableFlushesPerSecond = 60;

[picksi1, 2015/01/13 11:24:44]

Is this 60 related to the 60*4 in
kMaxResourceMessageRequestsPerSecondWhenThrottled? Can you add a comment as to
how you arrived at this value?

[jdduke (slow), 2015/01/13 16:18:11]

It's not really related, but I'll add a comment.

+
+}  // namespace
+
+ThrottledMessageSender::ThrottledMessageSender(IPC::Sender* proxied_sender,
+                                               RendererScheduler* scheduler,
+                                               int max_sent_messages_per_second)
+    : proxied_sender_(proxied_sender),
+      scheduler_(scheduler),
+      max_sent_messages_per_flush_(
+          std::max(1,
+                   max_sent_messages_per_second / kReasonableFlushesPerSecond)),
+      flush_period_(base::TimeDelta::FromSecondsD(
+          1.f /
+          std::min(max_sent_messages_per_second, kReasonableFlushesPerSecond))),
+      sent_messages_since_last_flush_(0),
+      flush_timer_(FROM_HERE,

[Sami, 2015/01/13 14:47:35]

I wonder if we should build some kind of
use-idle-time-if-possible-but-fall-back-to-posting-after-a-timeout abstraction
to avoid adding one more timer that fires at some random time into the frame?

[rmcilroy, 2015/01/13 15:24:37]

I like this idea (if not the name... ;).  How about GuaranteedIdleTask, which
takes a delay by which it should run, and is implemented as a delayed task which
races with an idle task, and the first one to get executed cancels the other?

One issue with this for this particular situation is that it might cause
FlushThrottled to be called much earlier than it's delay, thus causing more
messages to be sent than would otherwise while the sender is throttled.

[jdduke (slow), 2015/01/13 16:18:11]

All solid options, I'm open to whatever you all think best. The randomness is
unfortunate, but at least we know that the dispatch time should be fairly cheap
(unless it's a sync IPC, in which case there's not much we can do anyhow).

+                   flush_period_,
+                   base::Bind(&ThrottledMessageSender::FlushThrottled,
+                              base::Unretained(this)),
+                   false /* is_repeating */),
+      is_sending_via_proxied_(false) {
+  DCHECK(proxied_sender);
+  DCHECK(scheduler);
+  DCHECK_GT(max_sent_messages_per_second, 0);
+  flush_timer_.SetTaskRunner(scheduler->DefaultTaskRunner());
+}
+
+ThrottledMessageSender::~ThrottledMessageSender() {
+  FlushAll();
+  DCHECK(throttled_messages_.empty());
+}
+
+bool ThrottledMessageSender::Send(IPC::Message* msg) {
+  DCHECK(!is_sending_via_proxied_);
+  thread_checker_.CalledOnValidThread();
+
+  if (msg->is_sync()) {
+    FlushAll();
+    return SendViaProxiedSender(msg);
+  }
+
+  if (!throttled_messages_.empty()) {
+    throttled_messages_.push_back(msg);

[picksi1, 2015/01/13 11:24:44]

cunning!

+    return true;
+  }
+
+  if (!scheduler_->ShouldYieldForHighPriorityWork())

[alex clarke (OOO till 29th), 2015/01/13 10:38:02]

I think you'd get better results if the scheduler exposed it's policy. 

[Sami, 2015/01/13 14:47:35]

What advantage were you thinking of? This feels like a generic class that
shouldn't be tied to any specific policy.

[rmcilroy, 2015/01/13 15:24:37]

I agree with Sami here - I think this shouldn't be tied to a specific policy
(and if necessary the scheduler should provide a more specific signal, but I
think ShouldYieldForHighPriorityWork is fine unless we have strong evidence that
something else is better).

[alex clarke (OOO till 29th), 2015/01/13 15:46:55]

As far as I understand it, the intent here is to throttle IO resource loads
which can trigger expensive
processing on the IO thread (this is the root bug).  If we get a sudden glut of
them a BeginFrame or Input message can get stuck behind 20+ resource loads
leading to janks.

I contend that the current scheduler policy is a more useful signal than
ShouldYieldForHighPriorityWork, because ShouldYieldForHighPriorityWork will
return false most of the time even in compositor mode.

[jdduke (slow), 2015/01/13 16:18:11]

I agree with Alex here, and was actually going to ask if we could expose an
additional bit from the RendererScheduler. I'm not sure we need to expose the
current policy, but perhaps something indicating that a high-priority/user
action is taking place would be useful.

+    return SendViaProxiedSender(msg);
+
+  if (Now() > (last_sent_message_time_ + flush_period_)) {
+    // If sufficient time has passed since the previous send, we can effectively
+    // mark the pipeline as flushed.
+    sent_messages_since_last_flush_ = 0;
+    return SendViaProxiedSender(msg);
+  }
+
+  if (sent_messages_since_last_flush_ < max_sent_messages_per_flush_)
+    return SendViaProxiedSender(msg);
+
+  throttled_messages_.push_back(msg);
+  DCHECK(!flush_timer_.IsRunning());
+  flush_timer_.Reset();
+  return true;
+}
+
+base::TimeTicks ThrottledMessageSender::Now() const {
+  return base::TimeTicks::Now();
+}
+
+void ThrottledMessageSender::FlushAll() {
+  sent_messages_since_last_flush_ = 0;

[picksi1, 2015/01/13 11:24:44]

Can you confirm that there are no threading issues here? i.e. additional
messages won't get added while the throttled messages are being sent?

[jdduke (slow), 2015/01/13 16:18:11]

Yeah, I've got the |DCHECK(!is_sending_via_proxied)| check in the |Send()|
function. I originally had this function swap the queue into a local copy, I can
go back to that if you think it an improvement.

+  flush_timer_.Stop();
+
+  for (auto& msg : throttled_messages_)
+    SendViaProxiedSender(msg);
+  throttled_messages_.clear();
+}
+
+void ThrottledMessageSender::FlushThrottled() {
+  sent_messages_since_last_flush_ = 0;
+
+  while (!throttled_messages_.empty() &&

[picksi1, 2015/01/13 11:24:44]

Suggestion for discussion: Rather than checking for empty() & max_sent_messages
every time you could create a function that flushes(n) messages. This could be
called here and also in FlushAll [with queue.size()]. It would probably make
FlushAll a bit slower (with all the queue manipulation) but would give a single
code location for sending flushed messages.

[jdduke (slow), 2015/01/13 16:18:11]

That's a solid option, though it would prevent the "swap to local queue then
dispatch" proposal for FlushAll. Any preference?

+         sent_messages_since_last_flush_ < max_sent_messages_per_flush_) {
+    IPC::Message* msg = throttled_messages_.front();
+    throttled_messages_.pop_front();
+    SendViaProxiedSender(msg);

[Sami, 2015/01/13 14:47:35]

Should we yield here too (after sending at least one message to avoid starving)
if the scheduler wants us to?

[jdduke (slow), 2015/01/13 16:18:11]

Hmm, the |Send| itself should be fairly cheap (max 30-40us/message), but that's
an option. 

+  }
+
+  if (!throttled_messages_.empty())
+    flush_timer_.Reset();
+}
+
+bool ThrottledMessageSender::SendViaProxiedSender(IPC::Message* msg) {
+  DCHECK(!is_sending_via_proxied_);
+  base::AutoReset<bool> is_sending_via_proxied_resetter(
+      &is_sending_via_proxied_, true);
+  last_sent_message_time_ = Now();
+  ++sent_messages_since_last_flush_;
+  return proxied_sender_->Send(msg);
+}
+
+}  // namespace content