Reduce CPU usage of download shelf UI. (both MD and pre-MD shelves)

content/browser/download/download_item_impl.cc

 // Copyright (c) 2012 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.
 
 // File method ordering: Methods in this file are in the same order as
 // in download_item_impl.h, with the following exception: The public
 // interface Start is placed in chronological order with the other
 // (private) routines that together define a DownloadItem's state
 // transitions as the download progresses.  See "Download progression
 // cascade" later in this file.
@@ skipping 1020 matching lines @@
     // Cancel on the UI thread with an update on the FILE thread.
     //
     // TODO(rdsmith): Arguably we should let this go through, as this means
     // the download really did get further than we know before it was
     // cancelled.  But the gain isn't very large, and the code is more
     // fragile if it has to support in progress updates in a non-in-progress
     // state.  This issue should be readdressed when we revamp performance
     // reporting.
     return;
   }
+
+  int previous_progress = PercentComplete();
   bytes_per_sec_ = bytes_per_sec;
   hash_state_ = hash_state;
   received_bytes_ = bytes_so_far;
 
   // If we've received more data than we were expecting (bad server info?),
   // revert to 'unknown size mode'.
   if (received_bytes_ > total_bytes_)
     total_bytes_ = 0;
 
   if (bound_net_log_.IsCapturing()) {
     bound_net_log_.AddEvent(
         net::NetLog::TYPE_DOWNLOAD_ITEM_UPDATED,
         net::NetLog::Int64Callback("bytes_so_far", received_bytes_));
   }
 
-  UpdateObservers();
+  // Don't bother to update observers unless the progress percentage has
+  // actually changed.
+  if (previous_progress != -1 && PercentComplete() != previous_progress)

[asanka, 2015/12/18 03:39:16]

This change places the responsibility of throttling on the download guts which
are supposed to be agnostic of any specific UI. In this case, the
DownloadItemImpl will suppress OnDownloadUpdated() calls despite there being an
observable change (i.e. received_bytes_).

Shall we make the UI code ignore OnDownloadUpdated() notifications which don't
result in any noticeable changes?

If the updates are too chatty, we should throttle them at the source, which is
DownloadFileImpl. The updates are currently sent every 500ms, which I don't
think is excessive.

[Evan Stade, 2015/12/18 19:01:38]

My justification for putting the responsibility here is that it's the same class
that implements PercentComplete(), which provides an obscured view of the amount
of progress (you can't see received_bytes_ from the UI). So if the state of
DownloadItemImpl hasn't changed in any way that's observable from the outside
(modulo bytes_per_sec_), why call UpdateObservers?

The problem with moving this logic to the UI code is that there aren't enough
details in OnDownloadUpdated to easily know whether we really need to repaint.
It would be difficult for the UI to take a snapshot of the download item on
every paint and compare it to the new state of the download item in
OnDownloadUpdated. An alternative would be to break out OnDownloadUpdated into
more distinct methods (OnDownloadProgressed, etc.). I thought this solution was
the best way to minimize complexity.

All that said, this optimization is less important than the others because it
only saves us work on large/slow downloads (where it takes more than 500ms to
get 1% further along). We can probably skip this change and still get the vast
majority of the improvements noted in the CL description, the only reason I
added it was for thoroughness (I noticed the issue and it /felt/ wasteful).

[Evan Stade, 2015/12/29 18:57:03]

reverted this change

+    UpdateObservers();
 }
 
 void DownloadItemImpl::DestinationError(DownloadInterruptReason reason) {
   // Postpone recognition of this error until after file name determination
   // has completed and the intermediate file has been renamed to simplify
   // resumption conditions.
   if (current_path_.empty() || target_path_.empty())
     destination_error_ = reason;
   else
     Interrupt(reason);
@@ skipping 727 matching lines @@
     case RESUME_MODE_USER_CONTINUE:
       return "USER_CONTINUE";
     case RESUME_MODE_USER_RESTART:
       return "USER_RESTART";
   }
   NOTREACHED() << "Unknown resume mode " << mode;
   return "unknown";
 }
 
 }  // namespace content