Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2750713003:
Add UMA for estimating disk bandwidth and the time saved with parallel download (Closed)
DescriptionAdd UMA for estimating disk bandwidth and the time saved with parallel download
This CL adds UMAs for disk bandwiths for parallel download when:
1. there are only 1 request active
2. there are multiple requests active
It also adds a UMA to estimate the time saved through parallel download
BUG=644352
Review-Url: https://codereview.chromium.org/2750713003
Cr-Commit-Position: refs/heads/master@{#458217}
Committed: https://chromium.googlesource.com/chromium/src/+/0c5570d5cf495210b600a0b6fa3f3ec0b53a7b33
Patch Set 1 #
Total comments: 10
Patch Set 2 : address comments #Patch Set 3 : fix UMA calculation #
Total comments: 17
Patch Set 4 : addressing comments #Patch Set 5 : don't record UMA after pause #
Total comments: 8
Patch Set 6 : address comments #
Messages
Total messages: 28 (11 generated)
qinmin@chromium.org changed reviewers: + dtrainor@chromium.org, isherman@chromium.org, xingliu@chromium.org
PTAL
Discussion on the formula of time_saved. https://codereview.chromium.org/2750713003/diff/1/content/browser/download/do... File content/browser/download/download_stats.cc (right): https://codereview.chromium.org/2750713003/diff/1/content/browser/download/do... content/browser/download/download_stats.cc:742: time_without_parallel_streams - 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);
https://codereview.chromium.org/2750713003/diff/1/content/browser/download/do... File content/browser/download/download_stats.cc (right): https://codereview.chromium.org/2750713003/diff/1/content/browser/download/do... content/browser/download/download_stats.cc:732: UMA_HISTOGRAM_CUSTOM_COUNTS( 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) { ... } https://codereview.chromium.org/2750713003/diff/1/content/browser/download/do... content/browser/download/download_stats.cc:747: time_saved.InMilliseconds(), 0, 3600 * 1000, 50); It looks like we're setting the min at 0. Can this actually be negative like the comment says below? https://codereview.chromium.org/2750713003/diff/1/tools/metrics/histograms/hi... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/2750713003/diff/1/tools/metrics/histograms/hi... tools/metrics/histograms/histograms.xml:13620: +<histogram name="Download.TimeSavedWithParallelDownload" units="ms"> Can we describe how we expect these to work over the course of a download (e.g. it looks like the last stream will be counted as a single stream if the others have finished, etc.)?
https://codereview.chromium.org/2750713003/diff/1/content/browser/download/do... File content/browser/download/download_stats.cc (right): https://codereview.chromium.org/2750713003/diff/1/content/browser/download/do... content/browser/download/download_stats.cc:732: UMA_HISTOGRAM_CUSTOM_COUNTS( On 2017/03/14 15:28:38, David Trainor-ping if over 24h wrote: > 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) > { > ... > } Done. https://codereview.chromium.org/2750713003/diff/1/content/browser/download/do... content/browser/download/download_stats.cc:742: time_without_parallel_streams - 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: > 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); https://codereview.chromium.org/2750713003/diff/1/content/browser/download/do... content/browser/download/download_stats.cc:747: time_saved.InMilliseconds(), 0, 3600 * 1000, 50); On 2017/03/14 15:28:38, David Trainor-ping if over 24h wrote: > It looks like we're setting the min at 0. Can this actually be negative like > the comment says below? Good catch, fixed https://codereview.chromium.org/2750713003/diff/1/tools/metrics/histograms/hi... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/2750713003/diff/1/tools/metrics/histograms/hi... tools/metrics/histograms/histograms.xml:13620: +<histogram name="Download.TimeSavedWithParallelDownload" units="ms"> On 2017/03/14 15:28:38, David Trainor-ping if over 24h wrote: > Can we describe how we expect these to work over the course of a download (e.g. > it looks like the last stream will be counted as a single stream if the others > have finished, etc.)? Done.
https://codereview.chromium.org/2750713003/diff/1/content/browser/download/do... File content/browser/download/download_stats.cc (right): https://codereview.chromium.org/2750713003/diff/1/content/browser/download/do... content/browser/download/download_stats.cc:742: time_without_parallel_streams - On 2017/03/15 00:17:19, qinmin wrote: > 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: > > 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); > 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.
https://codereview.chromium.org/2750713003/diff/1/content/browser/download/do... File content/browser/download/download_stats.cc (right): https://codereview.chromium.org/2750713003/diff/1/content/browser/download/do... content/browser/download/download_stats.cc:742: time_without_parallel_streams - On 2017/03/15 01:07:22, xingliu wrote: > On 2017/03/15 00:17:19, qinmin wrote: > > 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: > > > 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); > > > > 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. That's correct. Thanks, fixed
lgtm
https://codereview.chromium.org/2750713003/diff/40001/content/browser/downloa... File content/browser/download/download_stats.cc (right): https://codereview.chromium.org/2750713003/diff/40001/content/browser/downloa... content/browser/download/download_stats.cc:354: base::TimeDelta elapsed_time) { Optional nit: I find this function a bit hard to think about, because it does two things -- compute a value, and record it -- but the name only describes one of those. I'd suggest splitting it into two functions: one to compute the value, and one leetle wrapper function for recording it. https://codereview.chromium.org/2750713003/diff/40001/content/browser/downloa... content/browser/download/download_stats.cc:360: UMA_HISTOGRAM_CUSTOM_COUNTS( The UMA_HISTOGRAM macros (other than the SPARSE macro) require metric names to be runtime constants. Please use base::UmaHistogramCustomCounts() instead. https://codereview.chromium.org/2750713003/diff/40001/content/browser/downloa... content/browser/download/download_stats.cc:361: metric, bandwidth, 1, 50000000, 50); nit: Could you please write this as "50 * 1000 * 1000" to help make the constant easier to read? Also, are you sure that you want to use 1000 rather than 1024 as a factor here? https://codereview.chromium.org/2750713003/diff/40001/content/browser/downloa... content/browser/download/download_stats.cc:746: int microseconds_per_hour = nit: I'd name this kMicrosecondsPerHour, and use base::checked_cast for the cast. https://codereview.chromium.org/2750713003/diff/40001/content/browser/downloa... content/browser/download/download_stats.cc:750: time_saved.InMilliseconds(), -microseconds_per_hour, UMA histograms do not support negative buckets (again, sparse histograms are the exception). You could shift the entire range up by kMicrosecondsPerHour, or record two separate histograms for gains vs. losses, or various other workarounds. Feel free to ping me if you're not sure how to proceed here. https://codereview.chromium.org/2750713003/diff/40001/tools/metrics/histogram... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/2750713003/diff/40001/tools/metrics/histogram... tools/metrics/histograms/histograms.xml:13039: +<histogram name="Download.BandwidthWithoutParallelStreamsBytesPerSecond" Optional/FYI: We typically do not include units in metric names (to keep the names more scannable), but I see that you're matching a previously used convention for similar histograms, so it's probably reasonable to keep the suffix here. https://codereview.chromium.org/2750713003/diff/40001/tools/metrics/histogram... tools/metrics/histograms/histograms.xml:13040: + units="Bytes/second"> nit: For the units, we prefer lowercase, i.e. "bytes/second" https://codereview.chromium.org/2750713003/diff/40001/tools/metrics/histogram... tools/metrics/histograms/histograms.xml:13050: + units="Bytes/second"> Ditto https://codereview.chromium.org/2750713003/diff/40001/tools/metrics/histogram... tools/metrics/histograms/histograms.xml:13620: +<histogram name="Download.TimeSavedWithParallelDownload" units="ms"> Optional nit: I'd include "Estimated" in this metric name, to emphasize that it's making some assumptions that might not be entirely accurate/should be taken with a grain of salt.
https://codereview.chromium.org/2750713003/diff/40001/content/browser/downloa... File content/browser/download/download_stats.cc (right): https://codereview.chromium.org/2750713003/diff/40001/content/browser/downloa... content/browser/download/download_stats.cc:354: base::TimeDelta elapsed_time) { On 2017/03/15 20:54:43, Ilya Sherman wrote: > Optional nit: I find this function a bit hard to think about, because it does > two things -- compute a value, and record it -- but the name only describes one > of those. I'd suggest splitting it into two functions: one to compute the > value, and one leetle wrapper function for recording it. Done. https://codereview.chromium.org/2750713003/diff/40001/content/browser/downloa... content/browser/download/download_stats.cc:360: UMA_HISTOGRAM_CUSTOM_COUNTS( On 2017/03/15 20:54:43, Ilya Sherman wrote: > The UMA_HISTOGRAM macros (other than the SPARSE macro) require metric names to > be runtime constants. Please use base::UmaHistogramCustomCounts() instead. Done. https://codereview.chromium.org/2750713003/diff/40001/content/browser/downloa... content/browser/download/download_stats.cc:361: metric, bandwidth, 1, 50000000, 50); On 2017/03/15 20:54:43, Ilya Sherman wrote: > nit: Could you please write this as "50 * 1000 * 1000" to help make the constant > easier to read? Also, are you sure that you want to use 1000 rather than 1024 > as a factor here? Done. still keeping 1000 as the existing UMAs are using 1000 instead of 1024 https://codereview.chromium.org/2750713003/diff/40001/content/browser/downloa... content/browser/download/download_stats.cc:746: int microseconds_per_hour = On 2017/03/15 20:54:43, Ilya Sherman wrote: > nit: I'd name this kMicrosecondsPerHour, and use base::checked_cast for the > cast. Done. https://codereview.chromium.org/2750713003/diff/40001/content/browser/downloa... content/browser/download/download_stats.cc:750: time_saved.InMilliseconds(), -microseconds_per_hour, On 2017/03/15 20:54:43, Ilya Sherman wrote: > UMA histograms do not support negative buckets (again, sparse histograms are the > exception). You could shift the entire range up by kMicrosecondsPerHour, or > record two separate histograms for gains vs. losses, or various other > workarounds. Feel free to ping me if you're not sure how to proceed here. Use 2 separate histogram instead. https://codereview.chromium.org/2750713003/diff/40001/tools/metrics/histogram... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/2750713003/diff/40001/tools/metrics/histogram... tools/metrics/histograms/histograms.xml:13040: + units="Bytes/second"> On 2017/03/15 20:54:43, Ilya Sherman wrote: > nit: For the units, we prefer lowercase, i.e. "bytes/second" Done. https://codereview.chromium.org/2750713003/diff/40001/tools/metrics/histogram... tools/metrics/histograms/histograms.xml:13050: + units="Bytes/second"> On 2017/03/15 20:54:43, Ilya Sherman wrote: > Ditto Done. https://codereview.chromium.org/2750713003/diff/40001/tools/metrics/histogram... tools/metrics/histograms/histograms.xml:13620: +<histogram name="Download.TimeSavedWithParallelDownload" units="ms"> On 2017/03/15 20:54:43, Ilya Sherman wrote: > Optional nit: I'd include "Estimated" in this metric name, to emphasize that > it's making some assumptions that might not be entirely accurate/should be taken > with a grain of salt. Done.
Metrics LGTM, thanks.
changed the logic to stop recording UMA if the download was paused, dtrainor@, xingliu@, PTAL again
https://codereview.chromium.org/2750713003/diff/80001/content/browser/downloa... File content/browser/download/download_file.h (right): https://codereview.chromium.org/2750713003/diff/80001/content/browser/downloa... content/browser/download/download_file.h:79: virtual void Pause() {} The methods here are all pure virtual, can we also make it pure virtual? It only has two subclasses, so shouldn't be too much work. nit%: Maybe add a comment here, since Pause call is not actually pause the download, but mainly for metric purpose. https://codereview.chromium.org/2750713003/diff/80001/content/browser/downloa... File content/browser/download/download_file_impl.cc (right): https://codereview.chromium.org/2750713003/diff/80001/content/browser/downloa... content/browser/download/download_file_impl.cc:288: record_stream_bandwidth_ = false; I think we didn't set it to true again, and didn't use this in the file. If set it to true again, maybe a Resume call? and maybe guard the bandwidth metric logic with if (record_stream_bandwidth_). If we discard the bandwidth metric after a pause, I think that's fine too, so we only record contiguous download. But still need to if (record_stream_bandwidth_) somewhere.
lgtm https://codereview.chromium.org/2750713003/diff/80001/content/browser/downloa... File content/browser/download/download_file.h (right): https://codereview.chromium.org/2750713003/diff/80001/content/browser/downloa... content/browser/download/download_file.h:79: virtual void Pause() {} On 2017/03/17 00:02:15, xingliu wrote: > The methods here are all pure virtual, can we also make it pure virtual? > It only has two subclasses, so shouldn't be too much work. > > nit%: Maybe add a comment here, since Pause call is not actually pause the > download, but mainly for metric purpose. WasPaused? https://codereview.chromium.org/2750713003/diff/80001/content/browser/downloa... File content/browser/download/download_file_impl.cc (right): https://codereview.chromium.org/2750713003/diff/80001/content/browser/downloa... content/browser/download/download_file_impl.cc:288: record_stream_bandwidth_ = false; On 2017/03/17 00:02:15, xingliu wrote: > I think we didn't set it to true again, and didn't use this in the file. > > If set it to true again, maybe a Resume call? and maybe guard the bandwidth > metric logic with if (record_stream_bandwidth_). > > > If we discard the bandwidth metric after a pause, I think that's fine too, so we > only record contiguous download. But still need to if (record_stream_bandwidth_) > somewhere. Do we need to reset last_update_time_ on resume as well?
https://codereview.chromium.org/2750713003/diff/80001/content/browser/downloa... File content/browser/download/download_file.h (right): https://codereview.chromium.org/2750713003/diff/80001/content/browser/downloa... content/browser/download/download_file.h:79: virtual void Pause() {} On 2017/03/17 07:25:40, David Trainor-ping if over 24h wrote: > On 2017/03/17 00:02:15, xingliu wrote: > > The methods here are all pure virtual, can we also make it pure virtual? > > It only has two subclasses, so shouldn't be too much work. > > > > nit%: Maybe add a comment here, since Pause call is not actually pause the > > download, but mainly for metric purpose. > > WasPaused? Done. https://codereview.chromium.org/2750713003/diff/80001/content/browser/downloa... content/browser/download/download_file.h:79: virtual void Pause() {} On 2017/03/17 00:02:15, xingliu wrote: > The methods here are all pure virtual, can we also make it pure virtual? > It only has two subclasses, so shouldn't be too much work. > > nit%: Maybe add a comment here, since Pause call is not actually pause the > download, but mainly for metric purpose. Done. Changed to WasPaused() as suggested by David https://codereview.chromium.org/2750713003/diff/80001/content/browser/downloa... File content/browser/download/download_file_impl.cc (right): https://codereview.chromium.org/2750713003/diff/80001/content/browser/downloa... content/browser/download/download_file_impl.cc:288: record_stream_bandwidth_ = false; On 2017/03/17 00:02:15, xingliu wrote: > I think we didn't set it to true again, and didn't use this in the file. > > If set it to true again, maybe a Resume call? and maybe guard the bandwidth > metric logic with if (record_stream_bandwidth_). > > > If we discard the bandwidth metric after a pause, I think that's fine too, so we > only record contiguous download. But still need to if (record_stream_bandwidth_) > somewhere. we should stop recording if a download is ever paused. The variable is now used in the logic to determine if we should record the download stats. https://codereview.chromium.org/2750713003/diff/80001/content/browser/downloa... content/browser/download/download_file_impl.cc:288: record_stream_bandwidth_ = false; On 2017/03/17 07:25:40, David Trainor-ping if over 24h wrote: > On 2017/03/17 00:02:15, xingliu wrote: > > I think we didn't set it to true again, and didn't use this in the file. > > > > If set it to true again, maybe a Resume call? and maybe guard the bandwidth > > metric logic with if (record_stream_bandwidth_). > > > > > > If we discard the bandwidth metric after a pause, I think that's fine too, so > we > > only record contiguous download. But still need to if > (record_stream_bandwidth_) > > somewhere. > > Do we need to reset last_update_time_ on resume as well? We don't record UMA even if resume is called, the value will contain too many noises and could go very inaccurate.
The CQ bit was checked by qinmin@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: cast_shell_linux on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/cast_shell_linu...)
Patchset #6 (id:100001) has been deleted
Patchset #6 (id:120001) has been deleted
New change lgtm.
The CQ bit was checked by qinmin@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from isherman@chromium.org, dtrainor@chromium.org Link to the patchset: https://codereview.chromium.org/2750713003/#ps140001 (title: "address comments")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch.
Bot data: {"patchset_id": 140001, "attempt_start_ts": 1490043091097880,
"parent_rev": "18bf53b4ccb6351759ca4a0bdc30a814aeb61137", "commit_rev":
"0c5570d5cf495210b600a0b6fa3f3ec0b53a7b33"}
Message was sent while issue was closed.
Description was changed from ========== Add UMA for estimating disk bandwidth and the time saved with parallel download This CL adds UMAs for disk bandwiths for parallel download when: 1. there are only 1 request active 2. there are multiple requests active It also adds a UMA to estimate the time saved through parallel download BUG=644352 ========== to ========== Add UMA for estimating disk bandwidth and the time saved with parallel download This CL adds UMAs for disk bandwiths for parallel download when: 1. there are only 1 request active 2. there are multiple requests active It also adds a UMA to estimate the time saved through parallel download BUG=644352 Review-Url: https://codereview.chromium.org/2750713003 Cr-Commit-Position: refs/heads/master@{#458217} Committed: https://chromium.googlesource.com/chromium/src/+/0c5570d5cf495210b600a0b6fa3f... ==========
Message was sent while issue was closed.
Committed patchset #6 (id:140001) as https://chromium.googlesource.com/chromium/src/+/0c5570d5cf495210b600a0b6fa3f... |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
