Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2676543004:
Use TaskScheduler instead of blocking pool in private_api_file_system.cc. (Closed)
|
Created:
3 years, 10 months ago by fdoray Modified:
3 years, 10 months ago Reviewers:
hirono CC:
chromium-reviews, extensions-reviews_chromium.org, yamaguchi+watch_chromium.org, oka+watch_chromium.org, rginda+watch_chromium.org, oshima+watch_chromium.org, fukino+watch_chromium.org, chromium-apps-reviews_chromium.org, davemoore+watch_chromium.org Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionUse TaskScheduler instead of blocking pool in private_api_file_system.cc.
The blocking pool is being deprecated in favor of TaskScheduler.
BUG=667892
R=hirono@chromium.org
Review-Url: https://codereview.chromium.org/2676543004
Cr-Commit-Position: refs/heads/master@{#451000}
Committed: https://chromium.googlesource.com/chromium/src/+/2400bf6994e29c7646b33508950c1343f0138658
Patch Set 1 #Patch Set 2 : upload #
Total comments: 4
Patch Set 3 : USER_VISIBLE and merge #
Total comments: 6
Patch Set 4 : update priorities #Messages
Total messages: 15 (8 generated)
Hi! I'm a Python script responsible for migrating tasks from the blocking pool to TaskScheduler. In this CL, I used these traits to post to TaskScheduler a task that was previously posted to the blocking pool: .WithPriority(base::TaskPriority::BACKGROUND) User won't notice if the task takes an arbitrarily long time to complete. Making your task BACKGROUND allows non-BACKGROUND tasks to run faster :) *No* .WithShutdownBehavior() .MayBlock() The task may block. This includes but is not limited to tasks that wait on synchronous file I/O operations: read or write a file from disk, interact with a pipe or a socket, rename or delete a file, enumerate files in a directory, etc. This trait isn't required for the mere use of locks. *No* .WithBaseSyncPrimitives() The task is not allowed to use these functions: - base::WaitableEvent::Wait - base::ConditionVariable::Wait - base::PlatformThread::Join - base::PlatformThread::Sleep - base::Process::WaitForExit - base::Process::WaitForExitWithTimeout If you think this is correct, please LGTM and CQ this CL. Otherwise, tell me which traits I should have used and I'll automatically upload a new patch. You can find documentation at https://cs.chromium.org/chromium/src/base/task_scheduler/task_traits.h WithPriority() [default/BACKGROUND/USER_VISIBLE/USER_BLOCKING]: WithShutdownBehavior() [default/CONTINUE_ON_SHUTDOWN/SKIP_ON_SHUTDOWN/BLOCK_SHUTDOWN]: MayBlock() [yes/no]: WithBaseSyncPrimitives() [yes/no]: Comments for a human: [A human will read this] FAQ: What should I do if the CQ dry run didn't pass? You can ignore this CL. A human will take a look and get back to you. Why are you doing this? Browser threads, the blocking pool and base::WorkerPool are being deprecated in favor of TaskScheduler. Design doc: https://docs.google.com/document/d/1S2AAeoo1xa_vsLbDYBsDHCqhrkfiMgoIPlyRi6kxa... What is the default priority? A task posted to TaskScheduler without an explicit priority inherits its priority from the calling context (e.g. a task posted to TaskScheduler from a BACKGROUND task without an explicit .WithPriority() will have a BACKGROUND priority). What is the default shutdown behavior? It is not documented on purpose. If your task shouldn't be skipped on shutdown (e.g. a task that persists user data on disk), it should have an explicit BLOCK_SHUTDOWN shutdown behavior. If it's important not to wait on your task on shutdown (e.g. it takes a long time to run and could cause a shutdown hang), it should have an explicit CONTINUE_ON_SHUTDOWN shutdown behavior.
The CQ bit was checked by fdoray@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: This issue passed the CQ dry run.
https://codereview.chromium.org/2676543004/diff/20001/chrome/browser/chromeos... File chrome/browser/chromeos/extensions/file_manager/private_api_file_system.cc (right): https://codereview.chromium.org/2676543004/diff/20001/chrome/browser/chromeos... chrome/browser/chromeos/extensions/file_manager/private_api_file_system.cc:76: void GetSizeStatsBlocking(const base::FilePath& mount_path, Do we need to change the name from *OnBlockingPool to *Blocking? https://codereview.chromium.org/2676543004/diff/20001/chrome/browser/chromeos... chrome/browser/chromeos/extensions/file_manager/private_api_file_system.cc:526: base::TaskPriority::BACKGROUND), The function is used by JavaScript to update UI. So I think USER_VISIBLE is better.
PTAnL https://codereview.chromium.org/2676543004/diff/20001/chrome/browser/chromeos... File chrome/browser/chromeos/extensions/file_manager/private_api_file_system.cc (right): https://codereview.chromium.org/2676543004/diff/20001/chrome/browser/chromeos... chrome/browser/chromeos/extensions/file_manager/private_api_file_system.cc:76: void GetSizeStatsBlocking(const base::FilePath& mount_path, On 2017/02/13 04:01:49, hirono wrote: > Do we need to change the name from *OnBlockingPool to *Blocking? The task is no longer running in the blocking pool. Switched to *Async per feedback from other owners on other CLs. https://codereview.chromium.org/2676543004/diff/20001/chrome/browser/chromeos... chrome/browser/chromeos/extensions/file_manager/private_api_file_system.cc:526: base::TaskPriority::BACKGROUND), On 2017/02/13 04:01:50, hirono wrote: > The function is used by JavaScript to update UI. So I think USER_VISIBLE is > better. Done.
lgtm after updating the priorities. https://codereview.chromium.org/2676543004/diff/40001/chrome/browser/chromeos... File chrome/browser/chromeos/extensions/file_manager/private_api_file_system.cc (right): https://codereview.chromium.org/2676543004/diff/40001/chrome/browser/chromeos... chrome/browser/chromeos/extensions/file_manager/private_api_file_system.cc:452: base::TaskPriority::BACKGROUND), USER_VISIBLE https://codereview.chromium.org/2676543004/diff/40001/chrome/browser/chromeos... chrome/browser/chromeos/extensions/file_manager/private_api_file_system.cc:528: base::TaskPriority::USER_VISIBLE), Sorry this was actually not only updating UI. This blocks user renaming operation. So it should be USER_BLOCKING. https://codereview.chromium.org/2676543004/diff/40001/chrome/browser/chromeos... chrome/browser/chromeos/extensions/file_manager/private_api_file_system.cc:996: base::TaskPriority::BACKGROUND), USER_VISIBLE
https://codereview.chromium.org/2676543004/diff/40001/chrome/browser/chromeos... File chrome/browser/chromeos/extensions/file_manager/private_api_file_system.cc (right): https://codereview.chromium.org/2676543004/diff/40001/chrome/browser/chromeos... chrome/browser/chromeos/extensions/file_manager/private_api_file_system.cc:452: base::TaskPriority::BACKGROUND), On 2017/02/15 04:40:50, hirono wrote: > USER_VISIBLE Done. https://codereview.chromium.org/2676543004/diff/40001/chrome/browser/chromeos... chrome/browser/chromeos/extensions/file_manager/private_api_file_system.cc:528: base::TaskPriority::USER_VISIBLE), On 2017/02/15 04:40:50, hirono wrote: > Sorry this was actually not only updating UI. This blocks user renaming > operation. So it should be USER_BLOCKING. Done. https://codereview.chromium.org/2676543004/diff/40001/chrome/browser/chromeos... chrome/browser/chromeos/extensions/file_manager/private_api_file_system.cc:996: base::TaskPriority::BACKGROUND), On 2017/02/15 04:40:50, hirono wrote: > USER_VISIBLE Done.
The CQ bit was checked by fdoray@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from hirono@chromium.org Link to the patchset: https://codereview.chromium.org/2676543004/#ps60001 (title: "update priorities")
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": 1487263196185530,
"parent_rev": "2c9cf1f8d2082e8210e2bec04d9580873fa629a4", "commit_rev":
"2400bf6994e29c7646b33508950c1343f0138658"}
Message was sent while issue was closed.
Description was changed from ========== Use TaskScheduler instead of blocking pool in private_api_file_system.cc. The blocking pool is being deprecated in favor of TaskScheduler. BUG=667892 R=hirono@chromium.org ========== to ========== Use TaskScheduler instead of blocking pool in private_api_file_system.cc. The blocking pool is being deprecated in favor of TaskScheduler. BUG=667892 R=hirono@chromium.org Review-Url: https://codereview.chromium.org/2676543004 Cr-Commit-Position: refs/heads/master@{#451000} Committed: https://chromium.googlesource.com/chromium/src/+/2400bf6994e29c7646b33508950c... ==========
Message was sent while issue was closed.
Committed patchset #4 (id:60001) as https://chromium.googlesource.com/chromium/src/+/2400bf6994e29c7646b33508950c... |
