Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2486603003:
Redirect HistoryService thread to TaskScheduler via a field trial. (Closed)
DescriptionRedirect HistoryService thread to TaskScheduler via a field trial.
When the "RedirectHistoryService" variation param of the BrowserScheduler
experiment is set, HistoryService doesn't create a dedicated thread to
run HistoryBackend operations. Instead, it forwards these operations to
a TaskScheduler sequence.
BUG=661143
Committed: https://crrev.com/0cab87a48e26580170d502dc54a195cecd5cf1ca
Cr-Commit-Position: refs/heads/master@{#433855}
Patch Set 1 #Patch Set 2 : self-review #
Total comments: 2
Patch Set 3 : CR brettw #17 (do not involve chrome_browser_main) #
Messages
Total messages: 36 (14 generated)
fdoray@chromium.org changed reviewers: + sky@chromium.org
PTAL
In the bug you mentioned: "- The history thread is currently joined when the HistoryService is destroyed, i.e. when the ProfileManager is destroyed. This is earlier than when BrowserThreads and TaskScheduler are shut down." That means after this change history tasks are going to be shutdown later. How do you know that isn't a problem?
gab@chromium.org changed reviewers: + gab@chromium.org
Re. shutdown, see suggestion below. https://codereview.chromium.org/2486603003/diff/20001/components/history/core... File components/history/core/browser/history_service.cc (right): https://codereview.chromium.org/2486603003/diff/20001/components/history/core... components/history/core/browser/history_service.cc:874: backend_task_runner_ = nullptr; To mimic identical shutdown behavior here while in redirection mode: Post a WaitableEvent::Signal() task to |backend_task_runner_| and synchronously block on it here? With a TODO to consider removing it in a follow-up (i.e. so that changing the shutdown order isn't convoluted with this change and can be reasoned about on its own).
On 2016/11/08 19:15:43, gab wrote: > Re. shutdown, see suggestion below. Other than that, lgtm. > > https://codereview.chromium.org/2486603003/diff/20001/components/history/core... > File components/history/core/browser/history_service.cc (right): > > https://codereview.chromium.org/2486603003/diff/20001/components/history/core... > components/history/core/browser/history_service.cc:874: backend_task_runner_ = > nullptr; > To mimic identical shutdown behavior here while in redirection mode: > > Post a WaitableEvent::Signal() task to |backend_task_runner_| and synchronously > block on it here? With a TODO to consider removing it in a follow-up (i.e. so > that changing the shutdown order isn't convoluted with this change and can be > reasoned about on its own).
robliao@chromium.org changed reviewers: + robliao@chromium.org
https://codereview.chromium.org/2486603003/diff/20001/chrome/browser/chrome_b... File chrome/browser/chrome_browser_main.cc (right): https://codereview.chromium.org/2486603003/diff/20001/chrome/browser/chrome_b... chrome/browser/chrome_browser_main.cc:342: // TODO(fdoray): Remove this when https://crbug.com/661143 concludes. Add this to ios_chrome_main_parts.mm. Side thought, can be done in a separate CL: Might be a good idea to centralize the redirection portion in components/task_scheduler_utils.
Description was changed from ========== Redirect HistoryService thread to TaskScheduler via a field trial. When the "RedirectHistoryService" variation param of the BrowserScheduler experiment is set, HistoryService doesn't create a dedicated thread to run HistoryBackend operations. Instead, it forwards these operations to a TaskScheduler sequence. BUG=661143 ========== to ========== Redirect HistoryService thread to TaskScheduler via a field trial. When the "RedirectHistoryService" variation param of the BrowserScheduler experiment is set, HistoryService doesn't create a dedicated thread to run HistoryBackend operations. Instead, it forwards these operations to a TaskScheduler sequence. BUG=661143 ==========
sky@chromium.org changed reviewers: + maxbogue@chromium.org, zea@chromium.org
On 2016/11/08 22:21:50, robliao wrote: > https://codereview.chromium.org/2486603003/diff/20001/chrome/browser/chrome_b... > File chrome/browser/chrome_browser_main.cc (right): > > https://codereview.chromium.org/2486603003/diff/20001/chrome/browser/chrome_b... > chrome/browser/chrome_browser_main.cc:342: // TODO(fdoray): Remove this when > https://crbug.com/661143 concludes. > Add this to ios_chrome_main_parts.mm. > > Side thought, can be done in a separate CL: Might be a good idea to centralize > the redirection portion in components/task_scheduler_utils. +zea/+maxbogue to see if there are issues with this around sync.
LGTM from a sync perspective
maxbogue@chromium.org changed reviewers: - maxbogue@chromium.org
On 2016/11/08 19:03:28, sky wrote: > In the bug you mentioned: > "- The history thread is currently joined when the HistoryService is destroyed, > i.e. when the ProfileManager is destroyed. This is earlier than when > BrowserThreads and TaskScheduler are shut down." > > That means after this change history tasks are going to be shutdown later. How > do you know that isn't a problem? sky@: I can use a WaitableEvent to make HistoryService::Cleanup() block until the HistoryBackend is destroyed. That way, history tasks wouldn't run later than when we join the history thread. Do I have your support to add HistoryService as a friend of ThreadRestrictions for that purpose? https://cs.chromium.org/chromium/src/base/threading/thread_restrictions.h?l=189
sky@chromium.org changed reviewers: + brettw@chromium.org
sky->brettw as he can hopefully better know if we need to make sure we have the exact same shutdown sequence.
Is there a way that we can structure them to avoid touching chrome_browser_main every time? I'm thinking of some kind of helper function that returns true or false given some kind of key, and looks up this particular field trial. Then this code could be done entirely inside the history service and without these extra statics.
On 2016/11/17 21:45:10, brettw (ping on IM after 24h) wrote: > Is there a way that we can structure them to avoid touching chrome_browser_main > every time? I'm thinking of some kind of helper function that returns true or > false given some kind of key, and looks up this particular field trial. Then > this code could be done entirely inside the history service and without these > extra statics. Sure, I could add such a function in components/task_scheduler_util/. Are you okay with the fact that with this CL, when redirection to TaskScheduler is enabled, HistoryBackend can outlive HistoryService::Cleanup()? Previously, HistoryBackend was always destroyed before this line returned https://cs.chromium.org/chromium/src/components/history/core/browser/history_... I could use a WaitableEvent to ensure that HistoryService::Cleanup() blocks until HistoryBackend is destroyed.
In the common case we just need to be sure that the databases are closed cleanly. I think this means we just need to ensure that the history tasks block shutdown. The other case I can think of is user switching. I don't know the answer since I've never used this and I don't know how the lifetimes of the HistoryService and the Profile relate to this. If when you switch users in Chrome it destroys the profile (and hence history service) and then you switch back quickly (maybe the disk is slow flushing), we wouldn't want to get a sharing error. If we don't delete the profile in this case, or all of the history services share the same sequenced task runner, we should be OK.
sky@ and brettw@: PTAL chrome_browser_main is no longer involved in this change.
Do you understand the profile switch issues I mentioned in the previous message?
On 2016/11/21 21:23:20, brettw (ping on IM after 24h) wrote: > Do you understand the profile switch issues I mentioned in the previous message? Profiles are only destroyed on browser shutdown (never on profile switch). gab@ can confirm. Also, history tasks redirected to Task Scheduler do block shutdown https://codereview.chromium.org/2486603003/diff/40001/components/history/core... When redirection to Task Scheduler is enabled, we wait for history tasks here https://cs.chromium.org/chromium/src/chrome/browser/chrome_browser_main.cc?sq... instead of in HistoryService::Cleanup().
Sounds good, LGTM
On 2016/11/21 21:34:56, brettw (ping on IM after 24h) wrote: > Sounds good, LGTM sky@: Any comments?
On 2016/11/21 21:31:17, fdoray wrote: > On 2016/11/21 21:23:20, brettw (ping on IM after 24h) wrote: > > Do you understand the profile switch issues I mentioned in the previous > message? > > Profiles are only destroyed on browser shutdown (never on profile switch). gab@ > can confirm. That is sad but true, closing the last window of a profile doesn't initialize any of it. Profile uninitialization only happens on shutdown. People tried to make early uninitialization possible but failed so it's unlikely to change anytime soon IMO. > > Also, history tasks redirected to Task Scheduler do block shutdown > https://codereview.chromium.org/2486603003/diff/40001/components/history/core... > When redirection to Task Scheduler is enabled, we wait for history tasks here > https://cs.chromium.org/chromium/src/chrome/browser/chrome_browser_main.cc?sq... > instead of in HistoryService::Cleanup().
Brett covers it all, so don't wait for me.
The CQ bit was checked by fdoray@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from gab@chromium.org, zea@chromium.org Link to the patchset: https://codereview.chromium.org/2486603003/#ps40001 (title: "CR brettw #17 (do not involve chrome_browser_main)")
The CQ bit was unchecked by fdoray@chromium.org
The CQ bit was checked by fdoray@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": 40001, "attempt_start_ts": 1479820345200730,
"parent_rev": "e5b186941afc4bfb7af6443e0dc9c9b0576ee236", "commit_rev":
"0e963572de03a32799a5e72fbcef7789bc14bbcf"}
Message was sent while issue was closed.
Description was changed from ========== Redirect HistoryService thread to TaskScheduler via a field trial. When the "RedirectHistoryService" variation param of the BrowserScheduler experiment is set, HistoryService doesn't create a dedicated thread to run HistoryBackend operations. Instead, it forwards these operations to a TaskScheduler sequence. BUG=661143 ========== to ========== Redirect HistoryService thread to TaskScheduler via a field trial. When the "RedirectHistoryService" variation param of the BrowserScheduler experiment is set, HistoryService doesn't create a dedicated thread to run HistoryBackend operations. Instead, it forwards these operations to a TaskScheduler sequence. BUG=661143 ==========
Message was sent while issue was closed.
Committed patchset #3 (id:40001)
Message was sent while issue was closed.
Description was changed from ========== Redirect HistoryService thread to TaskScheduler via a field trial. When the "RedirectHistoryService" variation param of the BrowserScheduler experiment is set, HistoryService doesn't create a dedicated thread to run HistoryBackend operations. Instead, it forwards these operations to a TaskScheduler sequence. BUG=661143 ========== to ========== Redirect HistoryService thread to TaskScheduler via a field trial. When the "RedirectHistoryService" variation param of the BrowserScheduler experiment is set, HistoryService doesn't create a dedicated thread to run HistoryBackend operations. Instead, it forwards these operations to a TaskScheduler sequence. BUG=661143 Committed: https://crrev.com/0cab87a48e26580170d502dc54a195cecd5cf1ca Cr-Commit-Position: refs/heads/master@{#433855} ==========
Message was sent while issue was closed.
Patchset 3 (id:??) landed as https://crrev.com/0cab87a48e26580170d502dc54a195cecd5cf1ca Cr-Commit-Position: refs/heads/master@{#433855} |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
