Add UMA for estimating disk bandwidth and the time saved with parallel download

content/browser/download/download_stats.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.
 
 #include "content/browser/download/download_stats.h"
 
 #include "base/macros.h"
 #include "base/metrics/histogram_macros.h"
 #include "base/metrics/sparse_histogram.h"
 #include "base/strings/string_util.h"
@@ skipping 328 matching lines @@
 
 // Maps extensions to their matching UMA histogram int value.
 int GetDangerousFileType(const base::FilePath& file_path) {
   for (size_t i = 0; i < arraysize(kDangerousFileTypes); ++i) {
     if (file_path.MatchesExtension(kDangerousFileTypes[i]))
       return i + 1;
   }
   return 0;  // Unknown extension.
 }
 
+// Helper method to calculate the bandwidth given the data length and time.
+int CalculateBandwidthBytesPerSecond(size_t length,
+                                     base::TimeDelta elapsed_time) {
+  size_t elapsed_time_ms = elapsed_time.InMilliseconds();
+  if (0u == elapsed_time_ms)
+    elapsed_time_ms = 1;
+
+  return 1000 * length / elapsed_time_ms;
+}
+
 } // namespace
 
 void RecordDownloadCount(DownloadCountTypes type) {
   UMA_HISTOGRAM_ENUMERATION(
       "Download.Counts", type, DOWNLOAD_COUNT_TYPES_LAST_ENTRY);
 }
 
 void RecordDownloadSource(DownloadSource source) {
   UMA_HISTOGRAM_ENUMERATION(
       "Download.Sources", source, DOWNLOAD_SOURCE_LAST_ENTRY);
@@ skipping 337 matching lines @@
         resource_handler_blocked_time * 100 / resource_handler_lifetime;
   }
 
   UMA_HISTOGRAM_COUNTS_100("Download.ResourceHandlerBlockedPercentage",
                            percentage);
 }
 
 void RecordFileBandwidth(size_t length,
                          base::TimeDelta disk_write_time,
                          base::TimeDelta elapsed_time) {
-  size_t elapsed_time_ms = elapsed_time.InMilliseconds();
-  if (0u == elapsed_time_ms)
-    elapsed_time_ms = 1;
-  size_t disk_write_time_ms = disk_write_time.InMilliseconds();
-  if (0u == disk_write_time_ms)
-    disk_write_time_ms = 1;
-
   UMA_HISTOGRAM_CUSTOM_COUNTS(
       "Download.BandwidthOverallBytesPerSecond",
-      (1000 * length / elapsed_time_ms), 1, 50000000, 50);
+      CalculateBandwidthBytesPerSecond(length, elapsed_time), 1, 50000000, 50);
   UMA_HISTOGRAM_CUSTOM_COUNTS(
       "Download.BandwidthDiskBytesPerSecond",
-      (1000 * length / disk_write_time_ms), 1, 50000000, 50);
+      CalculateBandwidthBytesPerSecond(length, disk_write_time), 1, 50000000,
+      50);
+}
+
+void RecordParallelDownloadStats(
+    size_t bytes_downloaded_with_parallel_streams,
+    base::TimeDelta time_with_parallel_streams,
+    size_t bytes_downloaded_without_parallel_streams,
+    base::TimeDelta time_without_parallel_streams) {
+  int bandwidth_without_parallel_streams = CalculateBandwidthBytesPerSecond(
+      bytes_downloaded_without_parallel_streams, time_without_parallel_streams);
+  UMA_HISTOGRAM_CUSTOM_COUNTS(

[David Trainor- moved to gerrit, 2017/03/14 15:28:38]

Might be cleaner to have a helper that logs the histogram as well.  Seems like
we pass the same constants and call the same other helper function for these
four.

LogBandwidthBytesPerSecond(const std::string& metric, size_t bytes, size_t time)
{
   ... 
}

[qinmin, 2017/03/15 00:17:19]

Done.

+      "Download.BandwidthWithParallelStreamsBytesPerSecond",
+      CalculateBandwidthBytesPerSecond(bytes_downloaded_with_parallel_streams,
+                                       time_with_parallel_streams),
+      1, 50000000, 50);
+  UMA_HISTOGRAM_CUSTOM_COUNTS(
+      "Download.BandwidthWithoutParallelStreamsBytesPerSecond",
+      bandwidth_without_parallel_streams, 1, 50000000, 50);
+
+  base::TimeDelta time_saved =
+      time_without_parallel_streams -

[xingliu, 2017/03/14 00:10:55]

Substraction between unsigned and signed integer is probably unsafe, that the
behavior depends on compiler implementation.

If signed is cast to unsigned, and later operand is larger, then we will have
very large result.

Also, it's weird to calculate the time saved based on substraction between the
writing time of one stream, and the writing time of multiple streams.

Maybe consider to just report a factor (speed of parallel / speed of
non-parallel). Probably it's more straight forward to view in UMA.

If we want to calculate time saved, maybe time_saved =
(total_bytes/non_parallel_bandwidth) - (total_bytes/parallel_bandwidth);

[qinmin, 2017/03/15 00:17:19]

TimeDelta uses a int64, which is signed, so I am not sure why this will cause
any issues.

We are estimating the ACTUAL time saved through parallel download, not the
theoretical time that can be saved if we have multiple streams at the beginning.
The ACTUAL total time we spent on downloading is time_without_parallel_streams +
time_with_parallel_streams.
Now if the data are all downloaded without parallel downloading, the total time
will be time_without_parallel_streams + bytes_downloaded_with_parallel_streams /
bandwidth_without_parallel_streams.

That's why I use this formula here.



On 2017/03/14 00:10:55, xingliu wrote:

[xingliu, 2017/03/15 01:07:22]

Sorry, I probably mistake the parameters here. Measure actual time is better
than theoretical time.

Based on the description here, should the time saved be 
 bytes_downloaded_with_parallel_streams /
bandwidth_without_parallel_streams - time_WITH_parallel_streams ?

So it's (time without parallel estimate) - actual time(with parallel download).
When parallel is faster, time safe is positive number.

[qinmin, 2017/03/15 16:29:09]

That's correct. Thanks, fixed

+      base::TimeDelta::FromMilliseconds(1000.0 *
+                                        bytes_downloaded_with_parallel_streams /
+                                        bandwidth_without_parallel_streams);
+  UMA_HISTOGRAM_CUSTOM_COUNTS("Download.TimeSavedWithParallelDownload",
+                              time_saved.InMilliseconds(), 0, 3600 * 1000, 50);

[David Trainor- moved to gerrit, 2017/03/14 15:28:38]

It looks like we're setting the min at 0.  Can this actually be negative like
the comment says below?

[qinmin, 2017/03/15 00:17:19]

Good catch, fixed

 }
 
 void RecordDownloadFileRenameResultAfterRetry(
     base::TimeDelta time_since_first_failure,
     DownloadInterruptReason interrupt_reason) {
   if (interrupt_reason == DOWNLOAD_INTERRUPT_REASON_NONE) {
     UMA_HISTOGRAM_TIMES("Download.TimeToRenameSuccessAfterInitialFailure",
                         time_since_first_failure);
   } else {
     UMA_HISTOGRAM_TIMES("Download.TimeToRenameFailureAfterInitialFailure",
@@ skipping 66 matching lines @@
   if (!page_transition)
     return;
 
   UMA_HISTOGRAM_ENUMERATION(
       "Download.PageTransition",
       ui::PageTransitionStripQualifier(page_transition.value()),
       ui::PAGE_TRANSITION_LAST_CORE + 1);
 }
 
 }  // namespace content