Add a utility function to calculate the next slice to download

Add a utility function to calculate the next slice to download

For parallel downloading, the received slices may not be next to each other.
This CL adds a helper function to calculate the next slice to download.
This new slice will fill up the first hole in the received slice vector.

BUG=644352
Review-Url: https://codereview.chromium.org/2728673003
Cr-Commit-Position: refs/heads/master@{#454194}
Committed: https://chromium.googlesource.com/chromium/src/+/41a4abfd6e8d1cfa7bbf3024ac21565c5b66e1b4

[qinmin, 2017-03-01 21:03:46]

qinmin@chromium.org changed reviewers:
+ boliu@chromium.org, dtrainor@chromium.org, xingliu@chromium.org

[qinmin, 2017-03-01 21:03:46]

PTAL

+boliu@ for content/browser/BUILD.gn
+xingliu@ for code review

[qinmin, 2017-03-01 21:05:07]

Patchset #1 (id:1) has been deleted

[xingliu, 2017-03-01 22:28:00]

The CQ bit was checked by xingliu@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-03-01 22:28:53]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-03-01 22:52:27]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-03-01 22:52:28]

Dry run: Try jobs failed on following builders:
  android_compile_dbg on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_comp...)

[xingliu, 2017-03-01 23:03:59]

https://codereview.chromium.org/2728673003/diff/20001/content/browser/downloa...
File content/browser/download/download_item_impl.cc (right):

https://codereview.chromium.org/2728673003/diff/20001/content/browser/downloa...
content/browser/download/download_item_impl.cc:1948: if (received_slices_.size()
> 1) {
Does FindNextSliceToDownload assume the slices are downloaded one by one?

https://codereview.chromium.org/2728673003/diff/20001/content/browser/downloa...
File content/browser/download/parallel_download_utils.cc (right):

https://codereview.chromium.org/2728673003/diff/20001/content/browser/downloa...
content/browser/download/parallel_download_utils.cc:19: if (next ==
received_slices.end()) {
Is it possible that the vector only contains one element such as [10,50]? where
we need to download the slice [0,10].

If it's impossible then we're safe.

https://codereview.chromium.org/2728673003/diff/20001/content/browser/downloa...
content/browser/download/parallel_download_utils.cc:25:
DCHECK_LT(remaining_bytes, 0);
Maybe DCHECK_GE(remaining_bytes, 0) or DCHECK_GT()?

If the vector contains [0,50], [50,100] that 2 slices are fully downloaded, then
remaining_bytes will be 0. The return value is correct. Can we add unit test for
this.

Since each slice maps to one db record, we probably won't merge them.

[qinmin, 2017-03-01 23:25:28]

https://codereview.chromium.org/2728673003/diff/20001/content/browser/downloa...
File content/browser/download/download_item_impl.cc (right):

https://codereview.chromium.org/2728673003/diff/20001/content/browser/downloa...
content/browser/download/download_item_impl.cc:1948: if (received_slices_.size()
> 1) {
On 2017/03/01 23:03:59, xingliu wrote:
> Does FindNextSliceToDownload assume the slices are downloaded one by one?

No,there is no such assumptions. For download resumption, however, all the
workers should have been paused, so we can pick up the first gap to start.

https://codereview.chromium.org/2728673003/diff/20001/content/browser/downloa...
File content/browser/download/parallel_download_utils.cc (right):

https://codereview.chromium.org/2728673003/diff/20001/content/browser/downloa...
content/browser/download/parallel_download_utils.cc:19: if (next ==
received_slices.end()) {
On 2017/03/01 23:03:59, xingliu wrote:
> Is it possible that the vector only contains one element such as [10,50]?
where
> we need to download the slice [0,10].
> 
> If it's impossible then we're safe.

hmm... good catch, this is actually possible. The main request can fail while
workers can download content successfully, thus causing a hole at the beginning.

https://codereview.chromium.org/2728673003/diff/20001/content/browser/downloa...
content/browser/download/parallel_download_utils.cc:25:
DCHECK_LT(remaining_bytes, 0);
Yes, this should be GE. Somehow I messed up the DCHECK when I was thinking
whether there is a -1 in calculating the offset. The test should already cover
the case you mentioned below.

we can actually merge the slices. In DownloadFileImpl::SendUpdate(), for
example, we can actually merge the slices and report it to DownloadItem.

On 2017/03/01 23:03:59, xingliu wrote:
> Maybe DCHECK_GE(remaining_bytes, 0) or DCHECK_GT()?
> 
> If the vector contains [0,50], [50,100] that 2 slices are fully downloaded,
then
> remaining_bytes will be 0. The return value is correct. Can we add unit test
for
> this.
> 
> Since each slice maps to one db record, we probably won't merge them.
>

[qinmin, 2017-03-01 23:25:45]

The CQ bit was checked by qinmin@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-03-01 23:26:16]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-03-01 23:38:31]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-03-01 23:38:31]

Dry run: Try jobs failed on following builders:
  cast_shell_android on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/cast_shell_a...)

[qinmin, 2017-03-01 23:41:17]

https://codereview.chromium.org/2728673003/diff/20001/content/browser/downloa...
File content/browser/download/parallel_download_utils.cc (right):

https://codereview.chromium.org/2728673003/diff/20001/content/browser/downloa...
content/browser/download/parallel_download_utils.cc:25:
DCHECK_LT(remaining_bytes, 0);
On 2017/03/01 23:25:28, qinmin wrote:
> Yes, this should be GE. Somehow I messed up the DCHECK when I was thinking
> whether there is a -1 in calculating the offset. The test should already cover
> the case you mentioned below.
> 
> we can actually merge the slices. In DownloadFileImpl::SendUpdate(), for
> example, we can actually merge the slices and report it to DownloadItem.
> 
> On 2017/03/01 23:03:59, xingliu wrote:
> > Maybe DCHECK_GE(remaining_bytes, 0) or DCHECK_GT()?
> > 
> > If the vector contains [0,50], [50,100] that 2 slices are fully downloaded,
> then
> > remaining_bytes will be 0. The return value is correct. Can we add unit test
> for
> > this.
> > 
> > Since each slice maps to one db record, we probably won't merge them.
> > 
> 

well, merging the slices requires 2 updates to the db. One with an empty vector,
and the other with a new vector, so it will require DownloadItem to update its
observers 2 times, which may not be very desirable. When download finishes, the
slice vector is cleared, so the slice information will be removed from db

[qinmin, 2017-03-01 23:43:00]

Patchset #2 (id:20002) has been deleted

[qinmin, 2017-03-01 23:43:14]

The CQ bit was checked by qinmin@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-03-01 23:44:21]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[qinmin, 2017-03-01 23:45:09]

Patchset #2 (id:50001) has been deleted

[qinmin, 2017-03-01 23:45:24]

The CQ bit was checked by qinmin@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-03-01 23:45:56]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-03-01 23:59:21]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-03-01 23:59:22]

Dry run: Try jobs failed on following builders:
  cast_shell_android on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/cast_shell_a...)
  cast_shell_linux on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/cast_shell_linu...)

[qinmin, 2017-03-02 00:14:11]

Patchset #2 (id:70001) has been deleted

[qinmin, 2017-03-02 00:14:18]

The CQ bit was checked by qinmin@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-03-02 00:14:58]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[xingliu, 2017-03-02 00:23:51]

https://codereview.chromium.org/2728673003/diff/20001/content/browser/downloa...
File content/browser/download/download_item_impl.cc (right):

https://codereview.chromium.org/2728673003/diff/20001/content/browser/downloa...
content/browser/download/download_item_impl.cc:1948: if (received_slices_.size()
> 1) {
On 2017/03/01 23:25:27, qinmin wrote:
> On 2017/03/01 23:03:59, xingliu wrote:
> > Does FindNextSliceToDownload assume the slices are downloaded one by one?
> 
> No,there is no such assumptions. For download resumption, however, all the
> workers should have been paused, so we can pick up the first gap to start.

So this function mainly serves for sending the first resumption request(not a
range request since we need the first response headers). That sounds good to me.

https://codereview.chromium.org/2728673003/diff/20001/content/browser/downloa...
content/browser/download/download_item_impl.cc:1948: if (received_slices_.size()
> 1) {
On 2017/03/01 23:25:27, qinmin wrote:
> On 2017/03/01 23:03:59, xingliu wrote:
> > Does FindNextSliceToDownload assume the slices are downloaded one by one?
> 
> No,there is no such assumptions. For download resumption, however, all the
> workers should have been paused, so we can pick up the first gap to start.

Following up the size of received_slices, if the size is 1, and we have one
slice [10, 20], then the logic falls into set_offset(GetReceivedBytes()), which
is probably wrong.

Also for normal download, if the size of received_slices is guaranteed to be 0,
then maybe check greater than 0 is ok.

https://codereview.chromium.org/2728673003/diff/20001/content/browser/downloa...
File content/browser/download/parallel_download_utils.cc (right):

https://codereview.chromium.org/2728673003/diff/20001/content/browser/downloa...
content/browser/download/parallel_download_utils.cc:25:
DCHECK_LT(remaining_bytes, 0);
On 2017/03/01 23:41:16, qinmin wrote:
> On 2017/03/01 23:25:28, qinmin wrote:
> > Yes, this should be GE. Somehow I messed up the DCHECK when I was thinking
> > whether there is a -1 in calculating the offset. The test should already
cover
> > the case you mentioned below.
> > 
> > we can actually merge the slices. In DownloadFileImpl::SendUpdate(), for
> > example, we can actually merge the slices and report it to DownloadItem.
> > 
> > On 2017/03/01 23:03:59, xingliu wrote:
> > > Maybe DCHECK_GE(remaining_bytes, 0) or DCHECK_GT()?
> > > 
> > > If the vector contains [0,50], [50,100] that 2 slices are fully
downloaded,
> > then
> > > remaining_bytes will be 0. The return value is correct. Can we add unit
test
> > for
> > > this.
> > > 
> > > Since each slice maps to one db record, we probably won't merge them.
> > > 
> > 
> 
> well, merging the slices requires 2 updates to the db. One with an empty
vector,
> and the other with a new vector, so it will require DownloadItem to update its
> observers 2 times, which may not be very desirable. When download finishes,
the
> slice vector is cleared, so the slice information will be removed from db

Agree, probably no need to merge. Also easier to debug and understand.

[commit-bot: I haz the power, 2017-03-02 00:47:50]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-03-02 00:47:51]

Dry run: Try jobs failed on following builders:
  linux_chromium_chromeos_ozone_rel_ng on master.tryserver.chromium.linux
(JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[qinmin, 2017-03-02 00:48:42]

The CQ bit was checked by qinmin@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-03-02 00:49:17]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[qinmin, 2017-03-02 01:00:39]

https://codereview.chromium.org/2728673003/diff/20001/content/browser/downloa...
File content/browser/download/download_item_impl.cc (right):

https://codereview.chromium.org/2728673003/diff/20001/content/browser/downloa...
content/browser/download/download_item_impl.cc:1948: if (received_slices_.size()
> 1) {
On 2017/03/02 00:23:51, xingliu wrote:
> On 2017/03/01 23:25:27, qinmin wrote:
> > On 2017/03/01 23:03:59, xingliu wrote:
> > > Does FindNextSliceToDownload assume the slices are downloaded one by one?
> > 
> > No,there is no such assumptions. For download resumption, however, all the
> > workers should have been paused, so we can pick up the first gap to start.
> 
> Following up the size of received_slices, if the size is 1, and we have one
> slice [10, 20], then the logic falls into set_offset(GetReceivedBytes()),
which
> is probably wrong.
> 
> Also for normal download, if the size of received_slices is guaranteed to be
0,
> then maybe check greater than 0 is ok.

The case you described(main request returns no slice, while one worker returns a
slice) will never happen. This is because the main request will create the
download file before DownloadItem::start() is called. So at least a [0, 0] slice
will be recorded in the received_slices_ when DownloadItem::start() is called.
That's why I put a slice size > 1 here.  Anyway, changed this to > 0 as it
shouldn't matter with the final result.

[xingliu, 2017-03-02 01:20:54]

lgtm

[commit-bot: I haz the power, 2017-03-02 02:20:00]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-03-02 02:20:01]

Dry run: This issue passed the CQ dry run.

[boliu, 2017-03-02 02:24:29]

content/browser/BUILD.gn rs lgtm

[qinmin, 2017-03-02 06:16:02]

The CQ bit was checked by qinmin@chromium.org

[commit-bot: I haz the power, 2017-03-02 06:16:51]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-03-02 06:21:35]

CQ is committing da patch.

Bot data: {"patchset_id": 110001, "attempt_start_ts": 1488435362216150,
"parent_rev": "a33c8dcc7f6cc69abb60a092cdbcc2d5e57b7ed7", "commit_rev":
"41a4abfd6e8d1cfa7bbf3024ac21565c5b66e1b4"}

[commit-bot: I haz the power, 2017-03-02 06:22:23]

Description was changed from

==========
Add a utility function to calculate the next slice to download

For parallel downloading, the received slices may not be next to each other.
This CL adds a helper function to calculate the next slice to download.
This new slice will fill up the first hole in the received slice vector.

BUG=644352
==========

to

==========
Add a utility function to calculate the next slice to download

For parallel downloading, the received slices may not be next to each other.
This CL adds a helper function to calculate the next slice to download.
This new slice will fill up the first hole in the received slice vector.

BUG=644352

Review-Url: https://codereview.chromium.org/2728673003
Cr-Commit-Position: refs/heads/master@{#454194}
Committed:
https://chromium.googlesource.com/chromium/src/+/41a4abfd6e8d1cfa7bbf3024ac21...
==========

[commit-bot: I haz the power, 2017-03-02 06:22:25]

Committed patchset #3 (id:110001) as
https://chromium.googlesource.com/chromium/src/+/41a4abfd6e8d1cfa7bbf3024ac21...