Check time budget before starting new requests, and unittest.

components/offline_pages/background/request_coordinator.cc

 // 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 "components/offline_pages/background/request_coordinator.h"
 
 #include <utility>
 
 #include "base/bind.h"
 #include "base/callback.h"
@@ skipping 17 matching lines @@
                             request_status,
                             Offliner::RequestStatus::STATUS_COUNT);
 }
 
 // TODO(dougarnett/petewil): Move to Policy object. Also consider lower minimum
 // battery percentage once there is some processing time limits in place.
 const Scheduler::TriggerConditions kUserRequestTriggerConditions(
     false /* require_power_connected */,
     50 /* minimum_battery_percentage */,
     false /* require_unmetered_network */);
-
-// Timeout is 2.5 minutes based on the size of Marshmallow doze mode
-// maintenance window (3 minutes)
-// TODO(petewil): Find the optimal timeout based on data for 2G connections and
-// common EM websites. crbug.com/625204
-// TODO(petewil): Move this value into the policy object.
-long OFFLINER_TIMEOUT_SECONDS = 150;
-
 }  // namespace
 
 RequestCoordinator::RequestCoordinator(std::unique_ptr<OfflinerPolicy> policy,
                                        std::unique_ptr<OfflinerFactory> factory,
                                        std::unique_ptr<RequestQueue> queue,
                                        std::unique_ptr<Scheduler> scheduler)
     : is_busy_(false),
       is_canceled_(false),
-      offliner_timeout_(base::TimeDelta::FromSeconds(OFFLINER_TIMEOUT_SECONDS)),
       offliner_(nullptr),
       policy_(std::move(policy)),
       factory_(std::move(factory)),
       queue_(std::move(queue)),
       scheduler_(std::move(scheduler)),
       last_offlining_status_(Offliner::RequestStatus::UNKNOWN),
+      offliner_timeout_(base::TimeDelta::FromSeconds(
+          policy_->GetSinglePageTimeBudgetInSeconds())),
       weak_ptr_factory_(this) {
   DCHECK(policy_ != nullptr);
   picker_.reset(new RequestPicker(queue_.get(), policy_.get()));
 }
 
 RequestCoordinator::~RequestCoordinator() {}
 
 bool RequestCoordinator::SavePageLater(
     const GURL& url, const ClientId& client_id, bool was_user_requested) {
   DVLOG(2) << "URL is " << url << " " << __FUNCTION__;
@@ skipping 50 matching lines @@
 }
 
 // Returns true if the caller should expect a callback, false otherwise. For
 // instance, this would return false if a request is already in progress.
 bool RequestCoordinator::StartProcessing(
     const DeviceConditions& device_conditions,
     const base::Callback<void(bool)>& callback) {
   current_conditions_.reset(new DeviceConditions(device_conditions));
   if (is_busy_) return false;
 
+  // Mark the time at which we started processing so we can check our time
+  // budget.
+  operation_start_time_ = base::Time::Now();
+
   is_canceled_ = false;
   scheduler_callback_ = callback;
   // TODO(petewil): Check existing conditions (should be passed down from
   // BackgroundTask)
 
   TryNextRequest();
 
   return true;
 }
 
 void RequestCoordinator::TryNextRequest() {
+  // If there is no time left in the budget, return to the scheduler.

[dougarnett, 2016/07/26 16:11:34]

So are we willing to overrun the time budget by just less than page time budget?
Perhaps more specifically, do you see the page budget as a hard limit (timeout)
and the processing limit as a soft limit (just up front check) or will we also
set a timeout for the processing time?

[Pete Williamson, 2016/07/26 18:42:19]

We are totally willing to overrun the time budget here.  The problem is that we
don't really have a good idea how long pages will take, 2 minutes might be par
for the course for a poor 2G network.  I expect to tune all of these numbers as
time goes by.  When we tune them, we might decide not to start a page unless a
we know that we are likely to finish it, and we might change this logic.  For
the moment, I have fairly lenient settings for while we are testing out the
product.

We can also tune this by tuning the budget number.  For instance, if we think it
will take two minutes, but we have a 3 minute window, we can make the budget
limit one minute, so we refuse to start a request unless we expect to finish it
in the worst case.

There's also the thought of worst case vs average case.  If worst case is 2
minutes, we will only ever get one page done in a 3 minute maintenance window if
we refuse to start with less than two minutes left.  However, the average case
might be better, so we might get 2-3 pages in the same time if we don't have
that limit.

[dougarnett, 2016/07/26 20:26:44]

I guess my meta point was that we have two things named "budget" that are used
differently which I didn't expect so want to check if behavior is what you
intended. Sounds like it is - so then I recommend making that a bit more clear
even if it is just terminology tweak such as *ProcessingTimeBudget* vs.
*PageTimeLimit* or some comment treatment.

[Pete Williamson, 2016/07/26 20:59:39]

Done, see how you like new names.

+  // We do not remove the pending task that was set up earlier in case
+  // we run out of time, so the background scheduler will return to us
+  // at the next opportunity to run background tasks.
+  if (base::Time::Now() - operation_start_time_ >
+      base::TimeDelta::FromSeconds(
+          policy_->GetBackgroundProcessingTimeBudgetSeconds())) {
+    // Let the scheduler know we are done processing.
+    scheduler_callback_.Run(true);
+
+    return;
+  }
+
   // Choose a request to process that meets the available conditions.
   // This is an async call, and returns right away.
   picker_->ChooseNextRequest(
       base::Bind(&RequestCoordinator::RequestPicked,
                  weak_ptr_factory_.GetWeakPtr()),
       base::Bind(&RequestCoordinator::RequestQueueEmpty,
                  weak_ptr_factory_.GetWeakPtr()),
       current_conditions_.get());
 }
 
@@ skipping 70 matching lines @@
     updated_request.set_last_attempt_time(base::Time::Now());
     RequestQueue::UpdateRequestCallback update_callback =
         base::Bind(&RequestCoordinator::UpdateRequestCallback,
                    weak_ptr_factory_.GetWeakPtr());
     queue_->UpdateRequest(
         updated_request,
         base::Bind(&RequestCoordinator::UpdateRequestCallback,
                    weak_ptr_factory_.GetWeakPtr()));
   }
 
-  // TODO(petewil): Check time budget. Return to the scheduler if we are out.
-  // Start another request if we have time.
   TryNextRequest();
 }
 
 const Scheduler::TriggerConditions&
 RequestCoordinator::GetTriggerConditionsForUserRequest() {
   return kUserRequestTriggerConditions;
 }
 
 void RequestCoordinator::GetOffliner() {
   if (!offliner_) {
     offliner_ = factory_->GetOffliner(policy_.get());
   }
 }
 
 }  // namespace offline_pages