Added member data to classes to support download resumption.

content/browser/download/download_item.h

Index: content/browser/download/download_item.h
diff --git a/content/browser/download/download_item.h b/content/browser/download/download_item.h
index 0e53e8d35d9e5522621760ae13708013d09e1548..0e1e811c1fefa978fa8b412139c1f84f49a1673c 100644
--- a/content/browser/download/download_item.h
+++ b/content/browser/download/download_item.h
@@ -24,6 +24,7 @@
 #include "base/file_path.h"
 #include "base/memory/scoped_ptr.h"
 #include "base/observer_list.h"
+#include "base/pickle.h"
 #include "base/time.h"
 #include "content/browser/download/download_id.h"
 #include "content/browser/download/download_request_handle.h"
@@ -146,8 +147,10 @@ class CONTENT_EXPORT DownloadItem {
   // Called when the user has validated the download of a dangerous file.
   void DangerousDownloadValidated();
 
-  // Received a new chunk of data
-  void Update(int64 bytes_so_far);
+  // Received a new chunk of data.
+  // |bytes_so_far| is the number of bytes received so far.
+  // |hash_state| is the current hash state.
+  void Update(int64 bytes_so_far, const Pickle& hash_state);

[Randy Smith (Not in Mondays), 2011/11/21 02:10:17]

So this makes me concerned, as pickling and unpickling can be something of a
heavyweight operation, and I'd rather not be doing it on a regular, updating
basis.  I'm not sure what the right answer is here, but by code smell I'm
tempted to reverse myself on my earlier acceptance and say that we should avoid
updating the size/hash pair in the DB except on interrupt and completion (and
I'd leave the hash blank on completion--I don't think it'd be used on restart,
and as previously noted, the complete hash is a completely different beast than
the intermediate hash).  All of which will take some plumbing :-{.  

I could be argued out of this--it's probably not *too* expensive to pickle on
every timer interrupt on the FILE thread.  But it really doesn't feel like the
right thing to be doing.

[ahendrickson, 2011/11/21 20:34:47]

FWIW, the size of the pickle payload is around 300 bytes (for the larger hash
scheme), and the overhead is on the order of 32 more bytes.

Pickle copying is mainly a call to memcpy(), and WriteData() is a memcpy() plus
some accounting.

That doesn't seem to me an expensive operation; what do you think?

[Randy Smith (Not in Mondays), 2011/11/22 17:37:49]

That sounds ok.  Could you figure out about how often (in time or bytes, either
way) the operation will occur?  I'd like to have a handle on the likely
performance implications before signing off.

[ahendrickson, 2011/11/22 23:46:44]

Operations are in bytes

SHA256ContextStr has 3 fields:

Type      Name                 Size
--------- -------------------- ----
byte[]    u:                    256
int32[]   h:                     32
int32     sizeHi:                 4
int32     sizeLo:                 4
                               -----
Total:                          296

We add to the pickle:

Type      Name                 Size
--------- -------------------- ----
int32     version:                4
char[]    "NSS":                  4
                               -----
Total:                            8

Pickle overhead (depending on 32/64-bit arch):

Type      Name                 Size
--------- -------------------- ----
Header*   header_                 4/8
size_t    header_size_            4/8
size_t    capacity_               4/8
size_t    variable_buffer_offset_ 4/8
                               -------
Total:                           16/32

Pickle size:                    296
                                  8
                                 16/32
                               -------
Total:                          320/352

SecureHashSHA256NSS::Serialize()
    fills pickle:                                                   320/352
  BaseFile::GetSha256HashState()
      creates string:                                               320/352
  BaseFile::GetSha256HashState()
      copies string to return:                                      320/352
    DownloadFileManager::UpdateInProgressDownloads()
        copies returned string:                                     320/352
    DownloadFileManager::UpdateInProgressDownloads()
        POSTs copy of string:                                       320/352
      DownloadManagerImpl::UpdateDownload()
        DownloadItemImpl::Update()
          DownloadItemImpl::UpdateSizeAndHashState()
            copies string:                                          320/352
        ChromeDownloadManagerDelegate::UpdateItemInPersistentStore()
              copies string:                                        320/352
          DownloadHistory::UpdateEntry()
            ...
            HistoryBackend::UpdateDownload()
              DownloadDatabase::UpdateDownload()
                  saves data in DB                                  320/352
                                                                   ---------
Total:                                                             2560/2816

This happens twice a second.

[Randy Smith (Not in Mondays), 2011/11/23 19:25:13]

I'm not too worried about the space costs; more about the overhead.  OTOH, it's
sounding like the copying of the bytes is the major cost.  And that's a fair
amount of copying.  

I think what I'll say is: Do some manual testing (before/after) using jar's
performance monitor--how much extra CPU time does this cost us for some
representative downloads?  If it's negligible, great.  If not, raise the flag
again--we probably don't want to do something extravagant to avoid this, but we
might (e.g.) reduce the frequency with which we send updates.

[ahendrickson, 2011/12/20 00:04:13]

I downloaded a 50MB file, which took about 10-15 seconds.

http://ftp.belnet.be/packages/damnsmalllinux/current/dsl-4.4.10-syslinux.iso

From the profiler:

DRH::OnReadCompleted() was called 1,694 times, for a total run time of 2.29 sec
(avg 0.001 sec).

DFM::UpdateInProgressDownloads() was called 56 times, for a total run time of
0.090 sec (avg 0.002 sec).

Other points of interest, not related to the above:

SBS::StartDownloadCheck() was called twice, with a total run time 0f 0.5 sec
(avg 0.26 sec).

CDMD::CheckIfSuggestedPathExists() was called once, with a run time of 0.27 sec.

DFM::StartDownload() was called once, with a run time of 1.67 sec.

[Randy Smith (Not in Mondays), 2011/12/20 17:05:40]

Cool.  What's the comparison?  (I.e. what that would look like without your
changes?)

[ahendrickson, 2011/12/20 22:06:54]

Without the hash state being passed around:


DRH::OnReadCompleted() was called 1,688 times, for a total run time of 2.20 sec
(avg 0.001 sec).

DFM::UpdateInProgressDownloads() was called 40 times, for a total run time of
0.20 sec (avg 0.005 sec).

Other points of interest, not related to the above:

SBS::StartDownloadCheck() was called twice, with a total run time 0f 1.5 sec
(avg 0.78 sec).

CDMD::CheckIfSuggestedPathExists() was called once, with a run time of 0.011
sec.

DFM::StartDownload() was called once, with a run time of 1.11 sec.

--

I'd say the hash copying is lost in the noise.

[Randy Smith (Not in Mondays), 2011/12/21 19:14:49]

Agreed.  Thanks for the due diligence.

 
   // Cancel the download operation. We need to distinguish between cancels at
   // exit (DownloadManager destructor) from user interface initiated cancels
@@ -176,8 +179,11 @@ class CONTENT_EXPORT DownloadItem {
 
   // Download operation had an error.
   // |size| is the amount of data received at interruption.
+  // |hash_state| is the current hash state at interruption.
   // |reason| is the download interrupt reason code that the operation received.
-  void Interrupted(int64 size, InterruptReason reason);
+  void Interrupted(int64 size,
+                   const Pickle& hash_state,
+                   InterruptReason reason);
 
   // Deletes the file from disk and removes the download from the views and
   // history.  |user| should be true if this is the result of the user clicking
@@ -297,6 +303,9 @@ class CONTENT_EXPORT DownloadItem {
   void set_opened(bool opened) { opened_ = opened; }
   bool opened() const { return opened_; }
 
+  const std::string& last_modified_time() const { return last_modified_time_; }
+  const std::string& etag() const { return etag_; }
+
   InterruptReason last_reason() const { return last_reason_; }
 
   DownloadPersistentStoreInfo GetPersistentStoreInfo() const;
@@ -338,8 +347,13 @@ class CONTENT_EXPORT DownloadItem {
   // downloads and false for downloads from the history.
   void Init(bool active);
 
+  // Internal helper for maintaining consistent received and total sizes, and
+  // hash state.
+  void UpdateSizeAndHashState(int64 bytes_so_far, const Pickle& hash_state);
+
   // Internal helper for maintaining consistent received and total sizes.
-  void UpdateSize(int64 size);
+  // Should only be called from |UpdateSizeAndHashState| and |OnAllDataSaved|.
+  void UpdateSize(int64 bytes_so_far);
 
   // Called when the entire download operation (including renaming etc)
   // is completed.
@@ -401,42 +415,50 @@ class CONTENT_EXPORT DownloadItem {
   // It's used to construct a suggested filename.
   std::string referrer_charset_;
 
-  // Total bytes expected
+  // Total bytes expected.
   int64 total_bytes_;
 
-  // Current received bytes
+  // Current received bytes.
   int64 received_bytes_;
 
   // Sha256 hash of the content.  This might be empty either because
-  // the download isn't done yet or because the hash isn't needed
+  // the download hasn't started yet or because the hash isn't needed
   // (ChromeDownloadManagerDelegate::GenerateFileHash() returned false).
   std::string hash_;
 
+  Pickle hash_state_;
+
+  // Server's time stamp for the file.
+  std::string last_modified_time_;
+
+  // Server's ETAG for the file.
+  std::string etag_;
+
   // Last reason.
   InterruptReason last_reason_;
 
-  // Start time for calculating remaining time
+  // Start time for calculating remaining time.
   base::TimeTicks start_tick_;
 
-  // The current state of this download
+  // The current state of this download.
   DownloadState state_;
 
-  // The views of this item in the download shelf and download tab
+  // The views of this item in the download shelf and download tab.
   ObserverList<Observer> observers_;
 
-  // Time the download was started
+  // Time the download was started.
   base::Time start_time_;
 
-  // Time the download completed
+  // Time the download completed.
   base::Time end_time_;
 
-  // Our persistent store handle
+  // Our persistent store handle.
   int64 db_handle_;
 
-  // Our owning object
+  // Our owning object.
   DownloadManager* download_manager_;
 
-  // In progress downloads may be paused by the user, we note it here
+  // In progress downloads may be paused by the user, we note it here.
   bool is_paused_;
 
   // A flag for indicating if the download should be opened at completion.