Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2888743002:
Switch temporary_file_stream to the TaskScheduler API. (Closed)
|
Created:
3 years, 7 months ago by Sébastien Marchand Modified:
3 years, 6 months ago CC:
chromium-reviews, loading-reviews_chromium.org, jam, darin-cc_chromium.org, Randy Smith (Not in Mondays) Target Ref:
refs/heads/master Project:
chromium Visibility:
Public. |
DescriptionSwitch temporary_file_stream to the TaskScheduler API.
BUG=689520
Review-Url: https://codereview.chromium.org/2888743002
Cr-Commit-Position: refs/heads/master@{#478652}
Committed: https://chromium.googlesource.com/chromium/src/+/4ee99e970e98366c707549fa52a1e2c4ac6b3d4d
Patch Set 1 #
Total comments: 2
Patch Set 2 : Address Francois' comments. #
Total comments: 2
Patch Set 3 : Add base::TaskShutdownBehavior::BLOCK_SHUTDOWN to the task traits #
Total comments: 8
Patch Set 4 : Switch to USER_VISIBLE priority #
Total comments: 1
Messages
Total messages: 46 (24 generated)
The CQ bit was checked by sebmarchand@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: win_chromium_rel_ng on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)
sebmarchand@chromium.org changed reviewers: + fdoray@chromium.org
PTAL https://codereview.chromium.org/2888743002/diff/1/content/browser/loader/temp... File content/browser/loader/temporary_file_stream_unittest.cc (right): https://codereview.chromium.org/2888743002/diff/1/content/browser/loader/temp... content/browser/loader/temporary_file_stream_unittest.cc:115: base::RunLoop().RunUntilIdle(); I'm not sure if this RunUntilIdle is still needed?
lgtm https://codereview.chromium.org/2888743002/diff/1/content/browser/loader/temp... File content/browser/loader/temporary_file_stream.cc (right): https://codereview.chromium.org/2888743002/diff/1/content/browser/loader/temp... content/browser/loader/temporary_file_stream.cc:66: base::RetainedRef(task_runner))); s/base::RetainedRef(task_runner)/std::move(task_runner)/ RetainedRef is only useful when you want to bind a ref-counted object and pass it as a raw pointer to the callback. Here, when the callback is invoked, a raw pointer is extracted from the scoped_refptr and it is converted back to a scoped_refptr because this is what DidCreateTemporaryFile takes as argument. std::move the TaskRunner avoids a ref-count bump and works since |task_runner| is not used after this line.
sebmarchand@chromium.org changed reviewers: + mmenke@chromium.org
+mmenke@ for owner review/approval.
https://codereview.chromium.org/2888743002/diff/20001/content/browser/loader/... File content/browser/loader/temporary_file_stream.cc (right): https://codereview.chromium.org/2888743002/diff/20001/content/browser/loader/... content/browser/loader/temporary_file_stream.cc:58: {base::MayBlock(), base::TaskPriority::BACKGROUND}); Should we use BLOCK_SHUTDOWN to try and make sure the temp file is destroyed? Temp files are created in a temp directory, but some OSes aren't very good about cleaning those up.
The CQ bit was checked by sebmarchand@chromium.org to run a CQ dry run
https://codereview.chromium.org/2888743002/diff/20001/content/browser/loader/... File content/browser/loader/temporary_file_stream.cc (right): https://codereview.chromium.org/2888743002/diff/20001/content/browser/loader/... content/browser/loader/temporary_file_stream.cc:58: {base::MayBlock(), base::TaskPriority::BACKGROUND}); On 2017/05/17 18:43:49, mmenke wrote: > Should we use BLOCK_SHUTDOWN to try and make sure the temp file is destroyed? > Temp files are created in a temp directory, but some OSes aren't very good about > cleaning those up. Ha good point, yeah adding base::TaskShutdownBehavior::BLOCK_SHUTDOWN makes sense here.
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
On 2017/05/17 18:51:35, Sébastien Marchand wrote: > https://codereview.chromium.org/2888743002/diff/20001/content/browser/loader/... > File content/browser/loader/temporary_file_stream.cc (right): > > https://codereview.chromium.org/2888743002/diff/20001/content/browser/loader/... > content/browser/loader/temporary_file_stream.cc:58: {base::MayBlock(), > base::TaskPriority::BACKGROUND}); > On 2017/05/17 18:43:49, mmenke wrote: > > Should we use BLOCK_SHUTDOWN to try and make sure the temp file is destroyed? > > Temp files are created in a temp directory, but some OSes aren't very good > about > > cleaning those up. > > Ha good point, yeah adding base::TaskShutdownBehavior::BLOCK_SHUTDOWN makes > sense here. LGTM!
Thanks for the quick review!
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
FYI this is blocked by crbug.com/724086
The CQ bit was checked by sebmarchand@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...
gab@chromium.org changed reviewers: + gab@chromium.org
You can forgo all the comments about raw ptrs being wrong, it was like that already... lgtm % question about BACKGROUND (OWNERS?) Note to owners of this code (I know it's probably old, but reminder just in case): ref ptrs should be passed explicitly (shouldn't eventually land in a scoped_refptr that grabs a ref when the caller merely passed a raw ptr): https://www.chromium.org/developers/smart-pointer-guidelines Cheers, Gab https://codereview.chromium.org/2888743002/diff/40001/content/browser/loader/... File content/browser/loader/temporary_file_stream.cc (right): https://codereview.chromium.org/2888743002/diff/40001/content/browser/loader/... content/browser/loader/temporary_file_stream.cc:42: task_runner.get()); optional: This is wrong, ShareableFileReference::GetOrCreate() should take a scoped_refptr (it passes it to something that does take a ref, passing raw ptrs should only be for things that use the pointer synchronously without taking a ref. If you update that call's semantics then this should be std::move(task_runner). https://codereview.chromium.org/2888743002/diff/40001/content/browser/loader/... content/browser/loader/temporary_file_stream.cc:47: callback.Run(error_code, std::move(file_stream), deletable_file.get()); Oh gosh... all of this code is so loosy-goosy with refcounting... |deletable_file| also eventually ends in a refcount and should be passed as such (but that feels way too far out of reach for this CL) https://codereview.chromium.org/2888743002/diff/40001/content/browser/loader/... content/browser/loader/temporary_file_stream.cc:58: {base::MayBlock(), base::TaskPriority::BACKGROUND, The API in the header file doesn't mention anything about this not having to respond in a reasonable time. Are owners explicitly okay with this? Otherwise we can also avoid specifying priority so that the caller's priority is inherited. https://codereview.chromium.org/2888743002/diff/40001/content/browser/loader/... content/browser/loader/temporary_file_stream.cc:62: new base::FileProxy(task_runner.get())); ditto (though you can't std::move here because you need it below; still should not hide the fact that FileProxy is taking a real ref) https://codereview.chromium.org/2888743002/diff/40001/content/browser/loader/... File content/browser/loader/temporary_file_stream_unittest.cc (right): https://codereview.chromium.org/2888743002/diff/40001/content/browser/loader/... content/browser/loader/temporary_file_stream_unittest.cc:116: content::RunAllBlockingPoolTasksUntilIdle(); This call includes RunLoop::RunUntilIdle so you can remove 115. Or even, did you just need that because of crbug.com/724086? If so, it's fixed now.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: win_chromium_rel_ng on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)
On 2017/05/19 00:10:52, gab wrote: > You can forgo all the comments about raw ptrs being wrong, it was like that > already... > > lgtm % question about BACKGROUND (OWNERS?) > > Note to owners of this code (I know it's probably old, but reminder just in > case): ref ptrs should be passed explicitly (shouldn't eventually land in a > scoped_refptr that grabs a ref when the caller merely passed a raw ptr): > https://www.chromium.org/developers/smart-pointer-guidelines > > Cheers, > Gab > > https://codereview.chromium.org/2888743002/diff/40001/content/browser/loader/... > File content/browser/loader/temporary_file_stream.cc (right): > > https://codereview.chromium.org/2888743002/diff/40001/content/browser/loader/... > content/browser/loader/temporary_file_stream.cc:42: task_runner.get()); > optional: This is wrong, ShareableFileReference::GetOrCreate() should take a > scoped_refptr (it passes it to something that does take a ref, passing raw ptrs > should only be for things that use the pointer synchronously without taking a > ref. > > If you update that call's semantics then this should be std::move(task_runner). As an owner, this is the kind of refactoring that doesn't seem worth the effort. Code is eventually going away, anyways (Within next year or two), but even without that, new code should certainly stick to new guidelines, but updating all old code doesn't seem worth the effort.
sebmarchand@chromium.org changed reviewers: + robliao@chromium.org
https://codereview.chromium.org/2888743002/diff/40001/content/browser/loader/... File content/browser/loader/temporary_file_stream.cc (right): https://codereview.chromium.org/2888743002/diff/40001/content/browser/loader/... content/browser/loader/temporary_file_stream.cc:59: base::TaskShutdownBehavior::BLOCK_SHUTDOWN}); +robliao@ (as gab@ and fdoray@ are ooo today): Could you clarify how using these flags differs from the old behavior? What would be the most equivalent to the good old FILE thread? https://codereview.chromium.org/2888743002/diff/40001/content/browser/loader/... File content/browser/loader/temporary_file_stream_unittest.cc (right): https://codereview.chromium.org/2888743002/diff/40001/content/browser/loader/... content/browser/loader/temporary_file_stream_unittest.cc:116: content::RunAllBlockingPoolTasksUntilIdle(); On 2017/05/19 00:10:52, gab wrote: > This call includes RunLoop::RunUntilIdle so you can remove 115. > > Or even, did you just need that because of crbug.com/724086? If so, it's fixed > now. No, I really need to wait for all the tasks to run before checking that the temporary file has been deleted.
https://codereview.chromium.org/2888743002/diff/40001/content/browser/loader/... File content/browser/loader/temporary_file_stream.cc (right): https://codereview.chromium.org/2888743002/diff/40001/content/browser/loader/... content/browser/loader/temporary_file_stream.cc:59: base::TaskShutdownBehavior::BLOCK_SHUTDOWN}); On 2017/05/22 16:55:21, Sébastien Marchand wrote: > +robliao@ (as gab@ and fdoray@ are ooo today): Could you clarify how using these > flags differs from the old behavior? What would be the most equivalent to the > good old FILE thread? Good ol' FILE thread is: MayBlock(), TaskPriority::USER_VISIBLE, TaskShutdownBehaviour::BLOCK_SHUTDOWN TaskPriority::USER_VISIBLE is really because some things do depend on responsiveness despite running on the FILE thread so by greatest common denominator that's what it's redirected as, but hopefully many things can move to BACKGROUND (i.e. will run as soon as the kernel isn't swamped).
On 2017/05/23 15:35:17, gab wrote: > https://codereview.chromium.org/2888743002/diff/40001/content/browser/loader/... > File content/browser/loader/temporary_file_stream.cc (right): > > https://codereview.chromium.org/2888743002/diff/40001/content/browser/loader/... > content/browser/loader/temporary_file_stream.cc:59: > base::TaskShutdownBehavior::BLOCK_SHUTDOWN}); > On 2017/05/22 16:55:21, Sébastien Marchand wrote: > > +robliao@ (as gab@ and fdoray@ are ooo today): Could you clarify how using > these > > flags differs from the old behavior? What would be the most equivalent to the > > good old FILE thread? > > Good ol' FILE thread is: > MayBlock(), TaskPriority::USER_VISIBLE, TaskShutdownBehaviour::BLOCK_SHUTDOWN > > TaskPriority::USER_VISIBLE is really because some things do depend on > responsiveness despite running on the FILE thread so by greatest common > denominator that's what it's redirected as, but hopefully many things can move > to BACKGROUND (i.e. will run as soon as the kernel isn't swamped). So this is one the path for blob URLs (Or at least some blob URLs), which could well be user visible, depending on their user on a website. So should this be USER_VISIBLE?
On 2017/05/23 15:37:04, mmenke wrote: > On 2017/05/23 15:35:17, gab wrote: > > > https://codereview.chromium.org/2888743002/diff/40001/content/browser/loader/... > > File content/browser/loader/temporary_file_stream.cc (right): > > > > > https://codereview.chromium.org/2888743002/diff/40001/content/browser/loader/... > > content/browser/loader/temporary_file_stream.cc:59: > > base::TaskShutdownBehavior::BLOCK_SHUTDOWN}); > > On 2017/05/22 16:55:21, Sébastien Marchand wrote: > > > +robliao@ (as gab@ and fdoray@ are ooo today): Could you clarify how using > > these > > > flags differs from the old behavior? What would be the most equivalent to > the > > > good old FILE thread? > > > > Good ol' FILE thread is: > > MayBlock(), TaskPriority::USER_VISIBLE, TaskShutdownBehaviour::BLOCK_SHUTDOWN > > > > TaskPriority::USER_VISIBLE is really because some things do depend on > > responsiveness despite running on the FILE thread so by greatest common > > denominator that's what it's redirected as, but hopefully many things can move > > to BACKGROUND (i.e. will run as soon as the kernel isn't swamped). > > So this is one the path for blob URLs (Or at least some blob URLs), which could > well be user visible, depending on their user on a website. So should this be > USER_VISIBLE? Feels like it, see https://cs.chromium.org/chromium/src/base/task_scheduler/task_traits.h?type=c... for detailed documentation.
The CQ bit was checked by sebmarchand@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...
k, I've switched the priority to USER_VISIBLE
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
The trybots are happy with this approach, PTAL.
lgtm % question for OWNERS https://codereview.chromium.org/2888743002/diff/60001/content/browser/loader/... File content/browser/loader/temporary_file_stream.cc (right): https://codereview.chromium.org/2888743002/diff/60001/content/browser/loader/... content/browser/loader/temporary_file_stream.cc:59: base::TaskShutdownBehavior::BLOCK_SHUTDOWN}); Does this need to block shutdown?
On 2017/05/23 22:12:37, gab wrote: > lgtm % question for OWNERS > > https://codereview.chromium.org/2888743002/diff/60001/content/browser/loader/... > File content/browser/loader/temporary_file_stream.cc (right): > > https://codereview.chromium.org/2888743002/diff/60001/content/browser/loader/... > content/browser/loader/temporary_file_stream.cc:59: > base::TaskShutdownBehavior::BLOCK_SHUTDOWN}); > Does this need to block shutdown? Per my earlier comment, some OSes do a really bad job of deleting temp files - on Windows, for instance, my temp directory regularly ends up being dozens of gigabytes.
[Sébastien Marchand]: Ping! Are you planning on landing this?
Sure!
The CQ bit was checked by sebmarchand@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from fdoray@chromium.org, mmenke@chromium.org Link to the patchset: https://codereview.chromium.org/2888743002/#ps60001 (title: "Switch to USER_VISIBLE priority")
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": 60001, "attempt_start_ts": 1497280212442620,
"parent_rev": "99c49baaf56fc1d3c0fac377f975a737686e7ae0", "commit_rev":
"4ee99e970e98366c707549fa52a1e2c4ac6b3d4d"}
Message was sent while issue was closed.
Description was changed from ========== Switch temporary_file_stream to the TaskScheduler API. BUG=689520 ========== to ========== Switch temporary_file_stream to the TaskScheduler API. BUG=689520 Review-Url: https://codereview.chromium.org/2888743002 Cr-Commit-Position: refs/heads/master@{#478652} Committed: https://chromium.googlesource.com/chromium/src/+/4ee99e970e98366c707549fa52a1... ==========
Message was sent while issue was closed.
Committed patchset #4 (id:60001) as https://chromium.googlesource.com/chromium/src/+/4ee99e970e98366c707549fa52a1... |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
