Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2677913002:
Use TaskScheduler instead of blocking pool in private_api_mount.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_mount.cc.
The blocking pool is being deprecated in favor of TaskScheduler.
BUG=667892
R=hirono@chromium.org
Review-Url: https://codereview.chromium.org/2677913002
Cr-Commit-Position: refs/heads/master@{#450579}
Committed: https://chromium.googlesource.com/chromium/src/+/16ffd0f8b3aa07845ccfcd134e159320f6cec16d
Patch Set 1 #Patch Set 2 : upload #Patch Set 3 : upload #
Total comments: 1
Patch Set 4 : USER_BLOCKING #Messages
Total messages: 18 (11 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: Try jobs failed on following builders: chromeos_amd64-generic_chromium_compile_only_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromeos_amd64-...)
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/2677913002/diff/40001/chrome/browser/chromeos... File chrome/browser/chromeos/extensions/file_manager/private_api_mount.cc (right): https://codereview.chromium.org/2677913002/diff/40001/chrome/browser/chromeos... chrome/browser/chromeos/extensions/file_manager/private_api_mount.cc:105: base::TaskPriority::BACKGROUND), This is blocking user operation to open an archive file. https://cs.chromium.org/chromium/src/ui/file_manager/file_manager/background/...
PTAnL
lgtm, thanks!
The CQ bit was checked by hirono@chromium.org
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": 1487134711389280,
"parent_rev": "8111f4d820ce77d9cfc6c1d47db3f35159d8f2e1", "commit_rev":
"16ffd0f8b3aa07845ccfcd134e159320f6cec16d"}
Message was sent while issue was closed.
Description was changed from ========== Use TaskScheduler instead of blocking pool in private_api_mount.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_mount.cc. The blocking pool is being deprecated in favor of TaskScheduler. BUG=667892 R=hirono@chromium.org Review-Url: https://codereview.chromium.org/2677913002 Cr-Commit-Position: refs/heads/master@{#450579} Committed: https://chromium.googlesource.com/chromium/src/+/16ffd0f8b3aa07845ccfcd134e15... ==========
Message was sent while issue was closed.
Committed patchset #4 (id:60001) as https://chromium.googlesource.com/chromium/src/+/16ffd0f8b3aa07845ccfcd134e15... |
