downloads: Improve download rate estimation.

downloads: Improve download rate estimation.

Previously, download rate was estimated as bytes_received/(current_time - download_start_time)

Although simple, this would react slowly to dynamic changes in download time, or user initiated pauses.

With this change, a ring buffer of the past 10 seconds of download time is used instead, and estimates are derived from that. It handles dynamic changes to bandwidth well. Pauses are handled reasonably well, as estimates are accurate after ten seconds - a better solution would be to clear the data rates when the user resumes from a pause. The main downside with this approach is it takes about 100 bytes of memory per active download, as well as adds some complexity.

BUG=16390
TEST=content_unittests

Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=202874

[cbentzel, 2013-05-11 23:07:19]

I'm mainly interested in if you think it's worth adding this, and how it will
work with download resumption.

I still want to do a closer pass over integer overflow and unsigned/signed
issues in this - shouldn't happen in practice, but want to make sure it's
bulletproof.

Also need to bulk up tests and the header comments a bit.

[asanka, 2013-05-13 17:37:12]

Looks good. Have you considered whether you want to attach the RateEstimator at
BaseFile or DownloadFileImpl? The latter is, I think, a better place to collect
statistics since it's directly aware of stream events and is already responsible
for notifying observers on download events.

Also, if you have a couple of net-internals logs of long/interesting downloads,
the DOWNLOAD_STREAM_DRAINED events in the DOWNLOAD log would be useful for
modeling the behavior of RateEstimator. The event is logged after each batch of
writes to the download file and includes the number of bytes written.

When an interrupted download is resumed (either automatically or manually), we
create a new DownloadFile (and by implication, a BaseFile). So a resumed
download would be roughly identical to a new download as far as RateEstimator is
concerned.

Pause()/Resume() is a bit tricky. If a download is being resumed after a manual
pause, then we could call DownloadFile and reset the RateEstimator state.

https://codereview.chromium.org/14697023/diff/10005/content/browser/download/...
File content/browser/download/rate_estimator.cc (right):

https://codereview.chromium.org/14697023/diff/10005/content/browser/download/...
content/browser/download/rate_estimator.cc:9: using base::Time;
Consider using TimeTicks? It's monotonically increasing and I think better
suited for this use case.

[cbentzel, 2013-05-14 10:39:55]

On 2013/05/13 17:37:12, asanka wrote:
> Looks good. Have you considered whether you want to attach the RateEstimator
at
> BaseFile or DownloadFileImpl? The latter is, I think, a better place to
collect
> statistics since it's directly aware of stream events and is already
responsible
> for notifying observers on download events.

I looked at DownloadFileImpl. One downside is that it doesn't capture the bytes
being written to the file.

> 
> Also, if you have a couple of net-internals logs of long/interesting
downloads,
> the DOWNLOAD_STREAM_DRAINED events in the DOWNLOAD log would be useful for
> modeling the behavior of RateEstimator. The event is logged after each batch
of
> writes to the download file and includes the number of bytes written.

This is a good idea. I did lots of manual tests of downloading files (as well as
pausing/resuming).

> 
> When an interrupted download is resumed (either automatically or manually), we
> create a new DownloadFile (and by implication, a BaseFile). So a resumed
> download would be roughly identical to a new download as far as RateEstimator
is
> concerned.
> 
> Pause()/Resume() is a bit tricky. If a download is being resumed after a
manual
> pause, then we could call DownloadFile and reset the RateEstimator state.

I'm not too worried about this. Arguably we could send a resume signal to get
the estimator in good shape, but that might be a bad idea. The other option is
to move the estimator into DownloadItemImpl and provide the data from the
updates, and add explicit support for clearing on resumption.

> 
>
https://codereview.chromium.org/14697023/diff/10005/content/browser/download/...
> File content/browser/download/rate_estimator.cc (right):
> 
>
https://codereview.chromium.org/14697023/diff/10005/content/browser/download/...
> content/browser/download/rate_estimator.cc:9: using base::Time;
> Consider using TimeTicks? It's monotonically increasing and I think better
> suited for this use case.

Yeah, went with TimeTicks. I agree it's on a monotonic clock, although
defensively I still worry about accessing invalid memory if it happens to go
back in time.

[asanka, 2013-05-14 15:27:03]

On 2013/05/14 10:39:55, cbentzel wrote:
https://codereview.chromium.org/14697023/diff/10005/content/browser/download/...
> > content/browser/download/rate_estimator.cc:9: using base::Time;
> > Consider using TimeTicks? It's monotonically increasing and I think better
> > suited for this use case.
> 
> Yeah, went with TimeTicks. I agree it's on a monotonic clock, although
> defensively I still worry about accessing invalid memory if it happens to go
> back in time.

Understood. The raw timeGetTime()/GetTimeTicks() call on Windows does rollover
every 49.7 days. But base::TimeTicks adjusts for rollover, provided somebody
calls TimeTicks::Now() at least once every 49.7 days.

Interestingly, the checked build of Windows starts the system clock at 49 days
on startup so that rollover bugs will be tripped earlier.

[cbentzel, 2013-05-14 16:28:54]

On 2013/05/14 15:27:03, asanka wrote:
> On 2013/05/14 10:39:55, cbentzel wrote:
>
https://codereview.chromium.org/14697023/diff/10005/content/browser/download/...
> > > content/browser/download/rate_estimator.cc:9: using base::Time;
> > > Consider using TimeTicks? It's monotonically increasing and I think better
> > > suited for this use case.
> > 
> > Yeah, went with TimeTicks. I agree it's on a monotonic clock, although
> > defensively I still worry about accessing invalid memory if it happens to go
> > back in time.
> 
> Understood. The raw timeGetTime()/GetTimeTicks() call on Windows does rollover
> every 49.7 days. But base::TimeTicks adjusts for rollover, provided somebody
> calls TimeTicks::Now() at least once every 49.7 days.
> 
> Interestingly, the checked build of Windows starts the system clock at 49 days
> on startup so that rollover bugs will be tripped earlier.

On other OS'es it does clock_gettime() with monotonic clock. Rollover likely
won't happen, but still can. Think I may put the extra safety checks back in.

[cbentzel, 2013-05-15 19:55:38]

OK, I cleaned up comments and tests and think this is ready to go.

[asanka, 2013-05-16 22:23:10]

LGTM

https://codereview.chromium.org/14697023/diff/36001/content/browser/download/...
File content/browser/download/rate_estimator.cc (right):

https://codereview.chromium.org/14697023/diff/36001/content/browser/download/...
content/browser/download/rate_estimator.cc:97: // clearn all the buckets.
Nit: clear

[commit-bot: I haz the power, 2013-05-17 09:41:02]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/cbentzel@chromium.org/14697023/51001

[commit-bot: I haz the power, 2013-05-17 09:56:47]

Retried try job too often on chromium_presubmit for step(s) presubmit
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=chromium_p...

[cbentzel, 2013-05-17 10:08:02]

+jam for content_browser.gypi + content_tests.gypi

[jam, 2013-05-28 13:18:30]

lgtm

[commit-bot: I haz the power, 2013-05-28 14:39:52]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/cbentzel@chromium.org/14697023/51001

[commit-bot: I haz the power, 2013-05-28 16:53:35]

Retried try job too often on linux_chromeos for step(s) browser_tests
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=linux_chro...

[commit-bot: I haz the power, 2013-05-29 11:13:39]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/cbentzel@chromium.org/14697023/51001

[commit-bot: I haz the power, 2013-05-29 15:19:19]

Change committed as 202874

[tfarina, 2013-05-29 16:31:18]

https://chromiumcodereview.appspot.com/14697023/diff/51001/content/browser/do...
File content/browser/download/rate_estimator.h (right):

https://chromiumcodereview.appspot.com/14697023/diff/51001/content/browser/do...
content/browser/download/rate_estimator.h:8: #include <string>
unused, you can remove.

https://chromiumcodereview.appspot.com/14697023/diff/51001/content/browser/do...
content/browser/download/rate_estimator.h:48: };
did you intentionally allowed copy and assign of this class?

[tfarina, 2013-05-29 16:35:21]

https://chromiumcodereview.appspot.com/14697023/diff/51001/content/browser/do...
File content/browser/download/rate_estimator.cc (right):

https://chromiumcodereview.appspot.com/14697023/diff/51001/content/browser/do...
content/browser/download/rate_estimator.cc:16: static const int
kDefaultBucketTimeSeconds = 1;
static not needed here. const has internal linkage, and thus that means it
wouldn't be even necessary to put it in unnamed namespace. static in C means the
symbol is local to this file scope.

https://chromiumcodereview.appspot.com/14697023/diff/51001/content/browser/do...
content/browser/download/rate_estimator.cc:36: DCHECK(bucket_time_.InSeconds() >
0);
DCHECK_GT?

https://chromiumcodereview.appspot.com/14697023/diff/51001/content/browser/do...
content/browser/download/rate_estimator.cc:41: }
clang-format would put } in the above line.

https://chromiumcodereview.appspot.com/14697023/diff/51001/content/browser/do...
content/browser/download/rate_estimator.cc:50: DCHECK(seconds_since_oldest >=
0);
DCHECK_GE

https://chromiumcodereview.appspot.com/14697023/diff/51001/content/browser/do...
content/browser/download/rate_estimator.cc:114: for (size_t i = 0; i <
history_.size(); ++i) {
no curlies for single-line statements.