Reland "Throttle resource message requests during user interaction"

content/renderer/resource_dispatch_throttler.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/resource_dispatch_throttler.h"
+
+#include "base/auto_reset.h"
+#include "base/debug/trace_event.h"
+#include "content/common/resource_messages.h"
+#include "content/renderer/scheduler/renderer_scheduler.h"
+#include "ipc/ipc_message_macros.h"
+
+namespace content {
+namespace {
+
+int GetRequestId(const IPC::Message& msg) {
+  int request_id = -1;
+  switch (msg.type()) {
+    case ResourceHostMsg_RequestResource::ID: {
+      PickleIterator iter(msg);
+      int routing_id = -1;
+      if (!iter.ReadInt(&routing_id) || !iter.ReadInt(&request_id))
+        NOTREACHED() << "Invalid id for resource request message.";
+    } break;
+
+    case ResourceHostMsg_DidChangePriority::ID:
+    case ResourceHostMsg_ReleaseDownloadedFile::ID:
+    case ResourceHostMsg_CancelRequest::ID:
+      if (!PickleIterator(msg).ReadInt(&request_id))
+        NOTREACHED() << "Invalid id for resource message.";
+      break;
+
+    default:
+      NOTREACHED() << "Invalid message for resource throttling.";
+      break;
+  }
+  return request_id;
+}
+
+bool UpdateRequestPriority(const IPC::Message& priority_msg,
+                           IPC::Message* request_msg) {
+  ResourceHostMsg_DidChangePriority::Param priority_params;
+  if (!ResourceHostMsg_DidChangePriority::Read(&priority_msg, &priority_params))
+    return false;
+
+  ResourceHostMsg_RequestResource::Param request_params;
+  if (!ResourceHostMsg_RequestResource::Read(request_msg, &request_params))
+    return false;
+
+  int routing_id = get<0>(request_params);
+  int request_id = get<1>(request_params);
+  DCHECK_EQ(request_id, get<0>(priority_params));
+
+  ResourceHostMsg_Request& updated_request = get<2>(request_params);
+  updated_request.priority = get<1>(priority_params);
+  *request_msg =
+      ResourceHostMsg_RequestResource(routing_id, request_id, updated_request);
+  return true;
+}
+
+bool UpdateRequestToReleaseDownloadedFile(IPC::Message* request_msg) {
+  ResourceHostMsg_RequestResource::Param request_params;
+  if (!ResourceHostMsg_RequestResource::Read(request_msg, &request_params))
+    return false;
+
+  int routing_id = get<0>(request_params);
+  int request_id = get<1>(request_params);
+  ResourceHostMsg_Request& updated_request = get<2>(request_params);
+  // No need to update the request message if no file download was specified.
+  if (!updated_request.download_to_file)
+    return true;
+
+  updated_request.download_to_file = false;
+  *request_msg =
+      ResourceHostMsg_RequestResource(routing_id, request_id, updated_request);
+  return true;
+}
+
+}  // namespace
+
+ResourceDispatchThrottler::ResourceDispatchThrottler(
+    IPC::Sender* proxied_sender,
+    RendererScheduler* scheduler,
+    base::TimeDelta flush_period,
+    uint32 max_requests_per_flush)
+    : proxied_sender_(proxied_sender),
+      scheduler_(scheduler),
+      flush_period_(flush_period),
+      max_requests_per_flush_(max_requests_per_flush),
+      flush_timer_(
+          FROM_HERE,
+          flush_period_,
+          base::Bind(&ResourceDispatchThrottler::Flush, base::Unretained(this)),
+          false /* is_repeating */),
+      sent_requests_since_last_flush_(0),
+      is_forwarding_request_(false) {
+  DCHECK(proxied_sender);
+  DCHECK(scheduler);
+  DCHECK(flush_period_ != base::TimeDelta());
+  DCHECK(max_requests_per_flush_);
+  flush_timer_.SetTaskRunner(scheduler->DefaultTaskRunner());

[Sami, 2015/01/27 14:06:06]

Should we make this use the loading task runner instead?

[jdduke (slow), 2015/01/27 16:59:09]

Done.

+}
+
+ResourceDispatchThrottler::~ResourceDispatchThrottler() {
+  for (auto& request : throttled_requests_)
+    ForwardRequest(request.second);
+  throttled_requests_.clear();
+}
+
+bool ResourceDispatchThrottler::Send(IPC::Message* msg) {
+  thread_checker_.CalledOnValidThread();
+  switch (msg->type()) {
+    case ResourceHostMsg_RequestResource::ID:

[Sami, 2015/01/27 14:06:06]

Could we use IPC_BEGIN_MESSAGE_MAP etc. here?

[jdduke (slow), 2015/01/27 16:59:09]

Hmm, I don't want to deserialize the whole message so I'd still want to use
IPC_MESSAGE_HANDLER_GENERIC. I'm also not sure how common it is to use that
macro in a |Send| call, as it has slightly different semantics (handled vs
sent), but I can make the switch if you prefer.

[Sami, 2015/01/27 17:50:12]

Oh, right, those macros are clearly meant more for the receiving side. The
current code looks fine, I was just used to seeing macros for this type of
stuff.

+      return OnRequestResource(msg);
+
+    case ResourceHostMsg_DidChangePriority::ID:
+      return OnDidChangePriority(msg);
+
+    case ResourceHostMsg_ReleaseDownloadedFile::ID:
+      return OnReleaseDownloadedFile(msg);
+
+    case ResourceHostMsg_CancelRequest::ID:
+      return OnCancelRequest(msg);
+
+    default:
+      return proxied_sender_->Send(msg);
+  }
+}
+
+base::TimeTicks ResourceDispatchThrottler::Now() const {
+  return base::TimeTicks::Now();
+}
+
+void ResourceDispatchThrottler::ScheduleFlush() {
+  DCHECK(!flush_timer_.IsRunning());
+  flush_timer_.Reset();
+}
+
+void ResourceDispatchThrottler::Flush() {
+  TRACE_EVENT1("loader", "ResourceDispatchThrottler::Flush",
+               "total_throttled_requests", throttled_requests_.size());
+  sent_requests_since_last_flush_ = 0;
+
+  // If high-priority work is no longer anticipated, dispatch can be safely
+  // accelerated. Avoid completely flushing in such case in the event that
+  // a large number of requests have been throttled.
+  uint32 max_requests = scheduler_->IsHighPriorityWorkAnticipated()
+                            ? max_requests_per_flush_
+                            : max_requests_per_flush_ * 2;
+
+  while (!throttled_requests_.empty() &&
+         sent_requests_since_last_flush_ < max_requests) {
+    auto request_it = throttled_requests_.begin();
+    scoped_ptr<IPC::Message> forwarded_msg(request_it->second);
+    throttled_requests_.erase(request_it);
+    ForwardRequest(forwarded_msg.release());
+  }
+
+  if (!throttled_requests_.empty())
+    ScheduleFlush();
+}
+
+bool ResourceDispatchThrottler::ForwardRequest(IPC::Message* msg) {
+  DCHECK(!is_forwarding_request_);
+  base::AutoReset<bool> is_forwarding_request_resetter(&is_forwarding_request_,
+                                                       true);
+  last_sent_request_time_ = Now();
+  ++sent_requests_since_last_flush_;
+  return proxied_sender_->Send(msg);
+}
+
+bool ResourceDispatchThrottler::OnRequestResource(IPC::Message* msg) {
+  DCHECK(!is_forwarding_request_);
+  const int request_id = GetRequestId(*msg);
+
+  // Shift responsibility for handling an invalid request ID downstream.
+  if (request_id == -1)
+    return ForwardRequest(msg);
+
+  if (!throttled_requests_.empty()) {
+    // Valid request ids must be monotonically increasing.

[Sami, 2015/01/27 14:06:06]

Do we need to worry about wrapping?

[jdduke (slow), 2015/01/27 16:59:09]

Yeah, I raised this concern to mmenke@ last week. I too don't feel comfortable
with an int here, and wonder if we should go ahead and make the switch to int64.
I should note, other parts of the resource pipeline also assume this
monotonicity, and it's a very real possibility (albeit very likely quite rare)
that we could run into wrapping and the whole system would break.

[Sami, 2015/01/27 17:50:12]

Right. It seems simple enough to use an int64 here, which would make at least
this part of the pipeline safe. 

+    DCHECK_LT(throttled_requests_.rbegin()->first, request_id);
+    throttled_requests_.insert(std::make_pair(request_id, msg));
+    TRACE_EVENT_INSTANT0("loader", "ResourceDispatchThrottler::ThrottleRequest",
+                         TRACE_EVENT_SCOPE_THREAD);
+    return true;
+  }
+
+  if (!scheduler_->IsHighPriorityWorkAnticipated())
+    return ForwardRequest(msg);
+
+  if (Now() > (last_sent_request_time_ + flush_period_)) {
+    // If sufficient time has passed since the previous send, we can effectively
+    // mark the pipeline as flushed.
+    sent_requests_since_last_flush_ = 0;
+    return ForwardRequest(msg);
+  }
+
+  if (sent_requests_since_last_flush_ < max_requests_per_flush_)
+    return ForwardRequest(msg);
+
+  TRACE_EVENT_INSTANT0("loader", "ResourceDispatchThrottler::ThrottleRequest",
+                       TRACE_EVENT_SCOPE_THREAD);
+  throttled_requests_.insert(std::make_pair(request_id, msg));
+  ScheduleFlush();
+  return true;
+}
+
+bool ResourceDispatchThrottler::OnDidChangePriority(IPC::Message* msg) {
+  scoped_ptr<IPC::Message> scoped_msg(msg);
+  auto request_it = throttled_requests_.find(GetRequestId(*msg));
+  if (request_it == throttled_requests_.end())
+    return proxied_sender_->Send(scoped_msg.release());
+
+  return UpdateRequestPriority(*scoped_msg, request_it->second);
+}
+
+bool ResourceDispatchThrottler::OnReleaseDownloadedFile(IPC::Message* msg) {
+  scoped_ptr<IPC::Message> scoped_msg(msg);
+  auto request_it = throttled_requests_.find(GetRequestId(*msg));
+  if (request_it == throttled_requests_.end())
+    return proxied_sender_->Send(scoped_msg.release());
+
+  // TODO(jdduke): Should this simply cancel the outstanding request?
+  return UpdateRequestToReleaseDownloadedFile(request_it->second);
+}
+
+bool ResourceDispatchThrottler::OnCancelRequest(IPC::Message* msg) {
+  scoped_ptr<IPC::Message> scoped_msg(msg);
+  auto request_it = throttled_requests_.find(GetRequestId(*msg));
+  if (request_it == throttled_requests_.end())
+    return proxied_sender_->Send(scoped_msg.release());
+
+  scoped_ptr<IPC::Message> cancelled_msg(request_it->second);
+  throttled_requests_.erase(request_it);
+  return true;
+}
+
+}  // namespace content