Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2881873002:
[reland] Use TaskScheduler instead of SequencedWorkerPool in snapshot_cache.mm. (Closed)
|
Created:
3 years, 7 months ago by fdoray Modified:
3 years, 7 months ago Reviewers:
jif CC:
chromium-reviews, marq+watch_chromium.org, ios-reviews+chrome_chromium.org, noyau+watch_chromium.org, ios-reviews_chromium.org, pkl (ping after 24h if needed) Target Ref:
refs/heads/master Project:
chromium Visibility:
Public. |
Description[reland] Use TaskScheduler instead of SequencedWorkerPool in snapshot_cache.mm.
Reland with fix in test.
SequencedWorkerPool is being deprecated in favor of TaskScheduler.
BUG=667892
R=jif@chromium.org
Review-Url: https://codereview.chromium.org/2881873002
Cr-Original-Commit-Position: refs/heads/master@{#472052}
Committed: https://chromium.googlesource.com/chromium/src/+/aa5299ae23a57ca5dce00a11da3ac8e73849351f
Review-Url: https://codereview.chromium.org/2881873002
Cr-Commit-Position: refs/heads/master@{#475107}
Committed: https://chromium.googlesource.com/chromium/src/+/a11be0d57a4e4f214588bbd6fab9398824802b7f
Patch Set 1 #Patch Set 2 : USER_VISIBLE #Patch Set 3 : fix test error #Patch Set 4 : self-review #Patch Set 5 : sequence #Patch Set 6 : self-review #Patch Set 7 : self-review #Patch Set 8 : add comment #
Dependent Patchsets: Messages
Total messages: 48 (35 generated)
The CQ bit was checked by fdoray@chromium.org to run a CQ dry run
Please take a look. This CL was generated automatically. The base::MayBlock() trait was specified for all call sites and the base::TaskPriority::BACKGROUND trait was specified for all non-test call sites. That may not be appropriate for your use case. Please verify that appropriate traits were used https://cs.chromium.org/chromium/src/base/task_scheduler/task_traits.h If everything looks good, lgtm and CQ this CL. Otherwise, tell us what's wrong. Thanks. - FAQ - What if bots are red? Ignore this CL. A human will look at errors shortly. What if the shutdown behavior is not set explicitly (no base::TaskShutdownBehavior)? If shutdown behavior is important for a task, it should be set explicitly. It's not necessary to specify it if you're fine with either BLOCK_SHUTDOWN or SKIP_ON_SHUTDOWN. Note that the default shutdown behavior is BLOCK_SHUTDOWN in SequencedWorkerPool and SKIP_ON_SHUTDOWN in TaskScheduler. What if the task priority is not set explicitly (no base::TaskPriority)? When there is no explicit priority, the priority is inherited from the calling context (e.g. a task posted from a BACKGROUND task without an explicit priority will have a BACKGROUND priority).
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.
base::TaskPriority::BACKGROUND is fine for saving snapshots to disk lgtm
On 2017/05/15 08:06:59, jif wrote: > base::TaskPriority::BACKGROUND is fine for saving snapshots to disk > lgtm actually, not lgtm. We may need the snapshots quickly if the users switches from one tab to an other rapidly. Can we bump the priority to USER_VISIBLE?
The CQ bit was checked by fdoray@chromium.org to run a CQ dry run
PTAnL
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.
lgtm, thank you.
The CQ bit was checked by jif@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": 20001, "attempt_start_ts": 1494926718942320,
"parent_rev": "eb9298e3f4cfba215057d6a5fcc5cbaa14cb9947", "commit_rev":
"aa5299ae23a57ca5dce00a11da3ac8e73849351f"}
Message was sent while issue was closed.
Description was changed from ========== Use TaskScheduler instead of SequencedWorkerPool in snapshot_cache.mm. SequencedWorkerPool is being deprecated in favor of TaskScheduler. BUG=667892 R=jif@chromium.org ========== to ========== Use TaskScheduler instead of SequencedWorkerPool in snapshot_cache.mm. SequencedWorkerPool is being deprecated in favor of TaskScheduler. BUG=667892 R=jif@chromium.org Review-Url: https://codereview.chromium.org/2881873002 Cr-Commit-Position: refs/heads/master@{#472052} Committed: https://chromium.googlesource.com/chromium/src/+/aa5299ae23a57ca5dce00a11da3a... ==========
Message was sent while issue was closed.
Committed patchset #2 (id:20001) as https://chromium.googlesource.com/chromium/src/+/aa5299ae23a57ca5dce00a11da3a...
Message was sent while issue was closed.
A revert of this CL (patchset #2 id:20001) has been created in https://codereview.chromium.org/2889553002/ by kolos@chromium.org. The reason for reverting is: Probably this CL causes many failures of iOS bots. For example, https://build.chromium.org/p/chromium.mac/builders/ios-simulator/builds/17573.
Message was sent while issue was closed.
Description was changed from ========== Use TaskScheduler instead of SequencedWorkerPool in snapshot_cache.mm. SequencedWorkerPool is being deprecated in favor of TaskScheduler. BUG=667892 R=jif@chromium.org Review-Url: https://codereview.chromium.org/2881873002 Cr-Commit-Position: refs/heads/master@{#472052} Committed: https://chromium.googlesource.com/chromium/src/+/aa5299ae23a57ca5dce00a11da3a... ========== to ========== Use TaskScheduler instead of SequencedWorkerPool in snapshot_cache.mm. SequencedWorkerPool is being deprecated in favor of TaskScheduler. BUG=667892 R=jif@chromium.org Review-Url: https://codereview.chromium.org/2881873002 Cr-Commit-Position: refs/heads/master@{#472052} Committed: https://chromium.googlesource.com/chromium/src/+/aa5299ae23a57ca5dce00a11da3a... ==========
Description was changed from ========== Use TaskScheduler instead of SequencedWorkerPool in snapshot_cache.mm. SequencedWorkerPool is being deprecated in favor of TaskScheduler. BUG=667892 R=jif@chromium.org Review-Url: https://codereview.chromium.org/2881873002 Cr-Commit-Position: refs/heads/master@{#472052} Committed: https://chromium.googlesource.com/chromium/src/+/aa5299ae23a57ca5dce00a11da3a... ========== to ========== [reland] Use TaskScheduler instead of SequencedWorkerPool in snapshot_cache.mm. Reland with fix in test. SequencedWorkerPool is being deprecated in favor of TaskScheduler. BUG=667892 R=jif@chromium.org Review-Url: https://codereview.chromium.org/2881873002 Cr-Commit-Position: refs/heads/master@{#472052} Committed: https://chromium.googlesource.com/chromium/src/+/aa5299ae23a57ca5dce00a11da3a... ==========
The CQ bit was checked by fdoray@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from jif@chromium.org Link to the patchset: https://codereview.chromium.org/2881873002/#ps40001 (title: "fix test error")
The CQ bit was unchecked by fdoray@chromium.org
The CQ bit was checked by fdoray@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from jif@chromium.org Link to the patchset: https://codereview.chromium.org/2881873002/#ps60001 (title: "self-review")
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 fdoray@chromium.org
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: ios-device-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device-xcode-...) ios-simulator-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator-xco...)
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 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 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.
The CQ bit was checked by fdoray@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from jif@chromium.org Link to the patchset: https://codereview.chromium.org/2881873002/#ps140001 (title: "add comment")
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": 140001, "attempt_start_ts": 1495830294889620,
"parent_rev": "721f723a4dd263c357515574823e8f3a91ee5561", "commit_rev":
"a11be0d57a4e4f214588bbd6fab9398824802b7f"}
Message was sent while issue was closed.
Description was changed from ========== [reland] Use TaskScheduler instead of SequencedWorkerPool in snapshot_cache.mm. Reland with fix in test. SequencedWorkerPool is being deprecated in favor of TaskScheduler. BUG=667892 R=jif@chromium.org Review-Url: https://codereview.chromium.org/2881873002 Cr-Commit-Position: refs/heads/master@{#472052} Committed: https://chromium.googlesource.com/chromium/src/+/aa5299ae23a57ca5dce00a11da3a... ========== to ========== [reland] Use TaskScheduler instead of SequencedWorkerPool in snapshot_cache.mm. Reland with fix in test. SequencedWorkerPool is being deprecated in favor of TaskScheduler. BUG=667892 R=jif@chromium.org Review-Url: https://codereview.chromium.org/2881873002 Cr-Original-Commit-Position: refs/heads/master@{#472052} Committed: https://chromium.googlesource.com/chromium/src/+/aa5299ae23a57ca5dce00a11da3a... Review-Url: https://codereview.chromium.org/2881873002 Cr-Commit-Position: refs/heads/master@{#475107} Committed: https://chromium.googlesource.com/chromium/src/+/a11be0d57a4e4f214588bbd6fab9... ==========
Message was sent while issue was closed.
Committed patchset #8 (id:140001) as https://chromium.googlesource.com/chromium/src/+/a11be0d57a4e4f214588bbd6fab9...
Message was sent while issue was closed.
A revert of this CL (patchset #8 id:140001) has been created in https://codereview.chromium.org/2910183002/ by sdefresne@chromium.org. The reason for reverting is: This CL is incorrect. SnapshotCache is a singleton that is indirectly accessed by multiple unit tests (for example MainController unit tests). CreateSequencedTaskRunnerWithTraits() returns an object that reference TaskScheduler::GetInstance(). This TaskScheduler object is detroyed between each test, thus we have a singleton object pointing to de-allocated memory. SnapshotCache need to be changed to no longer be a singleton, this is tracked by https://bugs.chromium.org/p/chromium/issues/detail?id=727597.. |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
