Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2744793003:
Move the logic to determine how much data can be written to another function (Closed)
DescriptionMove the logic to determine how much data can be written to another function
The logic to calculate how much data to write is getting bigger,
move to a separate function.
This CL also adds a check if the target location is already written.
If so, it will request the current stream to terminate.
Unit test will pass once https://codereview.chromium.org/2737033002/ lands
BUG=644352
Review-Url: https://codereview.chromium.org/2744793003
Cr-Commit-Position: refs/heads/master@{#457839}
Committed: https://chromium.googlesource.com/chromium/src/+/457f1988af83ecf544aa76a8ee8126f6ac980c41
Patch Set 1 #
Total comments: 4
Patch Set 2 : comments #
Total comments: 9
Patch Set 3 : addressing comments #
Messages
Total messages: 29 (18 generated)
qinmin@chromium.org changed reviewers: + dtrainor@chromium.org, xingliu@chromium.org
PTAL
The CQ bit was checked by xingliu@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...
lgtm with nits% https://codereview.chromium.org/2744793003/diff/1/content/browser/download/do... File content/browser/download/download_file_impl.cc (right): https://codereview.chromium.org/2744793003/diff/1/content/browser/download/do... content/browser/download/download_file_impl.cc:171: if (source_stream->length() != DownloadSaveInfo::kLengthFullContent && nit%: Maybe add a comment for this block. Write a partial buffer if the source stream exceeds length limit. https://codereview.chromium.org/2744793003/diff/1/content/browser/download/do... File content/browser/download/download_file_impl.h (right): https://codereview.chromium.org/2744793003/diff/1/content/browser/download/do... content/browser/download/download_file_impl.h:181: // if the target location is already written by another worker, the current nit%: We actually check this only before the first disk IO of the stream. Maybe explicitly address this here. This sgtm since we can avoid iterate the received slice vector on every buffer read. A half open request will stop when it hit a received slice when another CL checked in to set the length of previous source streams, right?
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 #2 (id:20001) has been deleted
https://codereview.chromium.org/2744793003/diff/1/content/browser/download/do... File content/browser/download/download_file_impl.cc (right): https://codereview.chromium.org/2744793003/diff/1/content/browser/download/do... content/browser/download/download_file_impl.cc:171: if (source_stream->length() != DownloadSaveInfo::kLengthFullContent && On 2017/03/11 00:11:46, xingliu wrote: > nit%: Maybe add a comment for this block. > Write a partial buffer if the source stream exceeds length limit. Done. https://codereview.chromium.org/2744793003/diff/1/content/browser/download/do... File content/browser/download/download_file_impl.h (right): https://codereview.chromium.org/2744793003/diff/1/content/browser/download/do... content/browser/download/download_file_impl.h:181: // if the target location is already written by another worker, the current On 2017/03/11 00:11:46, xingliu wrote: > nit%: We actually check this only before the first disk IO of the stream. Maybe > explicitly address this here. > > This sgtm since we can avoid iterate the received slice vector on every buffer > read. > > A half open request will stop when it hit a received slice when another CL > checked in to set the length of previous source streams, right? Done. Yes, a half open request will stop when a later source stream starts writing the first chunk. There are 2 scenarios we need to handle. Suppose there are stream A, B, and C, all of them are half open, and are ordered by the offset. When B starts writing, it will set the length limit on A, thus causing it to stop early. that's addressed in another CL. Now B continues to write and it surpasses the offset of C. If C starts writing at this moment, it should immediately stop. That's addressed in this CL.
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...
On 2017/03/14 23:24:51, qinmin wrote: > https://codereview.chromium.org/2744793003/diff/1/content/browser/download/do... > File content/browser/download/download_file_impl.cc (right): > > https://codereview.chromium.org/2744793003/diff/1/content/browser/download/do... > content/browser/download/download_file_impl.cc:171: if (source_stream->length() > != DownloadSaveInfo::kLengthFullContent && > On 2017/03/11 00:11:46, xingliu wrote: > > nit%: Maybe add a comment for this block. > > Write a partial buffer if the source stream exceeds length limit. > > Done. > > https://codereview.chromium.org/2744793003/diff/1/content/browser/download/do... > File content/browser/download/download_file_impl.h (right): > > https://codereview.chromium.org/2744793003/diff/1/content/browser/download/do... > content/browser/download/download_file_impl.h:181: // if the target location is > already written by another worker, the current > On 2017/03/11 00:11:46, xingliu wrote: > > nit%: We actually check this only before the first disk IO of the stream. > Maybe > > explicitly address this here. > > > > This sgtm since we can avoid iterate the received slice vector on every buffer > > read. > > > > A half open request will stop when it hit a received slice when another CL > > checked in to set the length of previous source streams, right? > > Done. > Yes, a half open request will stop when a later source stream starts writing the > first chunk. > There are 2 scenarios we need to handle. Suppose there are stream A, B, and C, > all of them are half open, and are ordered by the offset. > When B starts writing, it will set the length limit on A, thus causing it to > stop early. that's addressed in another CL. > Now B continues to write and it surpasses the offset of C. If C starts writing > at this moment, it should immediately stop. That's addressed in this CL. sgtm.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
https://codereview.chromium.org/2744793003/diff/40001/content/browser/downloa... File content/browser/download/download_file_impl.cc (right): https://codereview.chromium.org/2744793003/diff/40001/content/browser/downloa... content/browser/download/download_file_impl.cc:161: size_t bytes_read, bytes_available_to_write more clear? https://codereview.chromium.org/2744793003/diff/40001/content/browser/downloa... content/browser/download/download_file_impl.cc:163: // If a new slice find that its target position has already been written, find -> finds https://codereview.chromium.org/2744793003/diff/40001/content/browser/downloa... content/browser/download/download_file_impl.cc:176: if (source_stream->length() != DownloadSaveInfo::kLengthFullContent && Is source_stream->length() DownloadSaveInfo::kLengthFullContent for both the first and last slice? If it's the first slice and we get a chunk of data that spans the boundary of the start of the second slice, we won't stop and will bypass this block and return false to keep reading right? It'll kind of be okay because it'll break during the next read, but still seems incorrect. https://codereview.chromium.org/2744793003/diff/40001/content/browser/downloa... content/browser/download/download_file_impl.cc:344: source_stream, incoming_data_size, &bytes_to_write); Just for sanity sake, DCHECK_GE(incoming_data_size, bytes_to_write)?
https://codereview.chromium.org/2744793003/diff/40001/content/browser/downloa... File content/browser/download/download_file_impl.cc (right): https://codereview.chromium.org/2744793003/diff/40001/content/browser/downloa... content/browser/download/download_file_impl.cc:161: size_t bytes_read, On 2017/03/15 22:35:42, David Trainor-ping if over 24h wrote: > bytes_available_to_write more clear? Done. https://codereview.chromium.org/2744793003/diff/40001/content/browser/downloa... content/browser/download/download_file_impl.cc:163: // If a new slice find that its target position has already been written, On 2017/03/15 22:35:42, David Trainor-ping if over 24h wrote: > find -> finds Done. https://codereview.chromium.org/2744793003/diff/40001/content/browser/downloa... content/browser/download/download_file_impl.cc:176: if (source_stream->length() != DownloadSaveInfo::kLengthFullContent && On 2017/03/15 22:35:42, David Trainor-ping if over 24h wrote: > Is source_stream->length() DownloadSaveInfo::kLengthFullContent for both the > first and last slice? If it's the first slice and we get a chunk of data that > spans the boundary of the start of the second slice, we won't stop and will > bypass this block and return false to keep reading right? > > It'll kind of be okay because it'll break during the next read, but still seems > incorrect. No, that won't happen. If the second slice starts to write, DownloadFileImpl::AddNewSlice() will change the length of the first slice. So it will never span across the boundary. https://codereview.chromium.org/2744793003/diff/40001/content/browser/downloa... content/browser/download/download_file_impl.cc:344: source_stream, incoming_data_size, &bytes_to_write); On 2017/03/15 22:35:42, David Trainor-ping if over 24h wrote: > Just for sanity sake, DCHECK_GE(incoming_data_size, bytes_to_write)? Done.
ping, dtrainor@, is this change good to go now?
lgtm https://codereview.chromium.org/2744793003/diff/40001/content/browser/downloa... File content/browser/download/download_file_impl.cc (right): https://codereview.chromium.org/2744793003/diff/40001/content/browser/downloa... content/browser/download/download_file_impl.cc:176: if (source_stream->length() != DownloadSaveInfo::kLengthFullContent && On 2017/03/16 00:15:49, qinmin wrote: > On 2017/03/15 22:35:42, David Trainor-ping if over 24h wrote: > > Is source_stream->length() DownloadSaveInfo::kLengthFullContent for both the > > first and last slice? If it's the first slice and we get a chunk of data that > > spans the boundary of the start of the second slice, we won't stop and will > > bypass this block and return false to keep reading right? > > > > It'll kind of be okay because it'll break during the next read, but still > seems > > incorrect. > > No, that won't happen. If the second slice starts to write, > DownloadFileImpl::AddNewSlice() will change the length of the first slice. So it > will never span across the boundary. Thanks for the explanation. Sounds good!
The CQ bit was checked by qinmin@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from xingliu@chromium.org Link to the patchset: https://codereview.chromium.org/2744793003/#ps60001 (title: "addressing comments")
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 qinmin@chromium.org
Patchset #3 (id:60001) has been deleted
The CQ bit was checked by qinmin@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from xingliu@chromium.org, dtrainor@chromium.org Link to the patchset: https://codereview.chromium.org/2744793003/#ps80001 (title: "addressing 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": 80001, "attempt_start_ts": 1489771647466920,
"parent_rev": "e13fdde8bc0fbc7a85c7caa21fe2ff766ced836a", "commit_rev":
"457f1988af83ecf544aa76a8ee8126f6ac980c41"}
Message was sent while issue was closed.
Description was changed from ========== Move the logic to determine how much data can be written to another function The logic to calculate how much data to write is getting bigger, move to a separate function. This CL also adds a check if the target location is already written. If so, it will request the current stream to terminate. Unit test will pass once https://codereview.chromium.org/2737033002/ lands BUG=644352 ========== to ========== Move the logic to determine how much data can be written to another function The logic to calculate how much data to write is getting bigger, move to a separate function. This CL also adds a check if the target location is already written. If so, it will request the current stream to terminate. Unit test will pass once https://codereview.chromium.org/2737033002/ lands BUG=644352 Review-Url: https://codereview.chromium.org/2744793003 Cr-Commit-Position: refs/heads/master@{#457839} Committed: https://chromium.googlesource.com/chromium/src/+/457f1988af83ecf544aa76a8ee81... ==========
Message was sent while issue was closed.
Committed patchset #3 (id:80001) as https://chromium.googlesource.com/chromium/src/+/457f1988af83ecf544aa76a8ee81... |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
