Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2342373002:
Initialize the Browser Task Scheduler Between Field Trials and Metrics (Closed)
DescriptionInitialize the Browser Task Scheduler Between Field Trials and Metrics
This allows the Browser Task Scheduler to check variations before
metrics initializes and runs items on the blocking pool.
BUG=636518
Committed: https://crrev.com/3b73138b1a9173ec22856a2381c159fd6b68a1ba
Cr-Commit-Position: refs/heads/master@{#419549}
Patch Set 1 #
Total comments: 7
Patch Set 2 : CR Feedback #
Total comments: 8
Patch Set 3 : CR Feedback #
Total comments: 3
Depends on Patchset: Dependent Patchsets: Messages
Total messages: 26 (13 generated)
robliao@chromium.org changed reviewers: + fdoray@chromium.org, gab@chromium.org
The CQ bit was checked by robliao@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.
lgtm w/ comments https://codereview.chromium.org/2342373002/diff/1/chrome/browser/chrome_brows... File chrome/browser/chrome_browser_main.cc (right): https://codereview.chromium.org/2342373002/diff/1/chrome/browser/chrome_brows... chrome/browser/chrome_browser_main.cc:1339: // Note: It could also be the first thing in CreateThreads() but being in This Note is not so true if SetupMetrics() is in PreCreateThreads() and uses the blocking pool. https://codereview.chromium.org/2342373002/diff/1/chrome/browser/chrome_brows... chrome/browser/chrome_browser_main.cc:2193: // The TaskScheduler was initialized before browser process PreCreateThreads. "before the call to PreCreateThreads() on |browser_process_|" It took me a few seconds to understand what you meant by "browser process".
https://codereview.chromium.org/2342373002/diff/1/chrome/browser/chrome_brows... File chrome/browser/chrome_browser_main.cc (left): https://codereview.chromium.org/2342373002/diff/1/chrome/browser/chrome_brows... chrome/browser/chrome_browser_main.cc:1339: // This needs to be the last thing in PreCreateThreads() because the I think we do want this to remain the last thing. Can we instead move SetupMetrics() to PreMainMessageLoopRun() or something with all the other services? I want the invariant that no threads are created until the very end of PreCreateThreads() to eventually be true.
https://codereview.chromium.org/2342373002/diff/1/chrome/browser/chrome_brows... File chrome/browser/chrome_browser_main.cc (left): https://codereview.chromium.org/2342373002/diff/1/chrome/browser/chrome_brows... chrome/browser/chrome_browser_main.cc:1339: // This needs to be the last thing in PreCreateThreads() because the On 2016/09/16 19:57:04, gab wrote: > I think we do want this to remain the last thing. Can we instead move > SetupMetrics() to PreMainMessageLoopRun() or something with all the other > services? > > I want the invariant that no threads are created until the very end of > PreCreateThreads() to eventually be true. This will have to stay here since browser_process_->PreCreateThreads initializes the IOThread, which needs the metrics service. The nice part of this is that it is the absolute earliest time we can init. The possible drawback as you point out is that we create threads before the end of PreCreateThreads. One way to address this is to have a two-phase startup, init the structures here, buffer, and then start the threads at CreateThreads. I'm not so sure I'm thrilled about that structure, but it's something to think about. https://codereview.chromium.org/2342373002/diff/1/chrome/browser/chrome_brows... File chrome/browser/chrome_browser_main.cc (right): https://codereview.chromium.org/2342373002/diff/1/chrome/browser/chrome_brows... chrome/browser/chrome_browser_main.cc:1339: // Note: It could also be the first thing in CreateThreads() but being in On 2016/09/16 18:20:28, fdoray wrote: > This Note is not so true if SetupMetrics() is in PreCreateThreads() and uses the > blocking pool. Indeed. Removed. https://codereview.chromium.org/2342373002/diff/1/chrome/browser/chrome_brows... chrome/browser/chrome_browser_main.cc:2193: // The TaskScheduler was initialized before browser process PreCreateThreads. On 2016/09/16 18:20:28, fdoray wrote: > "before the call to PreCreateThreads() on |browser_process_|" > > It took me a few seconds to understand what you meant by "browser process". Done.
lgtm % comments, thanks! https://codereview.chromium.org/2342373002/diff/1/chrome/browser/chrome_brows... File chrome/browser/chrome_browser_main.cc (left): https://codereview.chromium.org/2342373002/diff/1/chrome/browser/chrome_brows... chrome/browser/chrome_browser_main.cc:1339: // This needs to be the last thing in PreCreateThreads() because the On 2016/09/16 21:35:40, robliao wrote: > On 2016/09/16 19:57:04, gab wrote: > > I think we do want this to remain the last thing. Can we instead move > > SetupMetrics() to PreMainMessageLoopRun() or something with all the other > > services? > > > > I want the invariant that no threads are created until the very end of > > PreCreateThreads() to eventually be true. > > This will have to stay here since browser_process_->PreCreateThreads initializes > the IOThread, which needs the metrics service. > > The nice part of this is that it is the absolute earliest time we can init. The > possible drawback as you point out is that we create threads before the end of > PreCreateThreads. One way to address this is to have a two-phase startup, init > the structures here, buffer, and then start the threads at CreateThreads. I'm > not so sure I'm thrilled about that structure, but it's something to think > about. Agreed I'm also not thrilled about buffering either just for the sake of making PreCreateThreads() correct. https://codereview.chromium.org/2342373002/diff/20001/chrome/browser/chrome_b... File chrome/browser/chrome_browser_main.cc (right): https://codereview.chromium.org/2342373002/diff/20001/chrome/browser/chrome_b... chrome/browser/chrome_browser_main.cc:1345: // ChromeOS needs ResourceBundle::InitSharedInstance to be called before this. Add: // This instantiates the IOThread and must be after SetupMetrics() (**explain why if it makes to here**?). https://codereview.chromium.org/2342373002/diff/20001/chrome/browser/chrome_b... chrome/browser/chrome_browser_main.cc:1348: device::GeolocationProvider::SetGeolocationDelegate( Can we move this before SetupFieldTrials() and then add a big comment before SetupFieldTrials() like: // ***IMPORTANT***: do not add anything after this comment unless you know specifically what you are doing. Some of the calls below end up implicitly creating threads and as such new calls typically either belong before them or in a later startup phase. https://codereview.chromium.org/2342373002/diff/20001/chrome/browser/chrome_b... chrome/browser/chrome_browser_main.cc:2190: // |browser_process_|. To maintain scoping symmetry, perform the shutdown s/before the PreCreateThreads on |browser_process_|/before invoking |browser_process->PreCreateThreads()|/ ? https://codereview.chromium.org/2342373002/diff/20001/chrome/browser/chrome_b... chrome/browser/chrome_browser_main.cc:2191: // after browser process PostDestroyThreads. Similarly: "after invoking |browser_process_->PostDestroyThreads()|" ?
robliao@chromium.org changed reviewers: + brettw@chromium.org
brettw: Please review this change. Thanks! https://codereview.chromium.org/2342373002/diff/20001/chrome/browser/chrome_b... File chrome/browser/chrome_browser_main.cc (right): https://codereview.chromium.org/2342373002/diff/20001/chrome/browser/chrome_b... chrome/browser/chrome_browser_main.cc:1345: // ChromeOS needs ResourceBundle::InitSharedInstance to be called before this. On 2016/09/19 17:51:32, gab wrote: > Add: > > // This instantiates the IOThread and must be after SetupMetrics() (**explain > why if it makes to here**?). Done. What do you mean mean about the "explain why if it makes it to here". I think we've hammered the issue home with the large comment on MaybeInitializeTaskScheduler. https://codereview.chromium.org/2342373002/diff/20001/chrome/browser/chrome_b... chrome/browser/chrome_browser_main.cc:1348: device::GeolocationProvider::SetGeolocationDelegate( On 2016/09/19 17:51:32, gab wrote: > Can we move this before SetupFieldTrials() and then add a big comment before > SetupFieldTrials() like: > > // ***IMPORTANT***: do not add anything after this comment unless you know > specifically what you are doing. Some of the calls below end up implicitly > creating threads and as such new calls typically either belong before them or in > a later startup phase. Looks safe to move (default constructor, global set but not dereferenced during this init phase). https://codereview.chromium.org/2342373002/diff/20001/chrome/browser/chrome_b... chrome/browser/chrome_browser_main.cc:2190: // |browser_process_|. To maintain scoping symmetry, perform the shutdown On 2016/09/19 17:51:32, gab wrote: > s/before the PreCreateThreads on |browser_process_|/before invoking > |browser_process->PreCreateThreads()|/ ? Done. https://codereview.chromium.org/2342373002/diff/20001/chrome/browser/chrome_b... chrome/browser/chrome_browser_main.cc:2191: // after browser process PostDestroyThreads. On 2016/09/19 17:51:32, gab wrote: > Similarly: "after invoking |browser_process_->PostDestroyThreads()|" ? Done.
The CQ bit was checked by robliao@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...
lgtm
lgtm++ https://codereview.chromium.org/2342373002/diff/40001/chrome/browser/chrome_b... File chrome/browser/chrome_browser_main.cc (right): https://codereview.chromium.org/2342373002/diff/40001/chrome/browser/chrome_b... chrome/browser/chrome_browser_main.cc:1336: // Now the command line has been mutated based on about:flags, we can While you're here: s/Now the/Now that the/
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
https://codereview.chromium.org/2342373002/diff/40001/chrome/browser/chrome_b... File chrome/browser/chrome_browser_main.cc (right): https://codereview.chromium.org/2342373002/diff/40001/chrome/browser/chrome_b... chrome/browser/chrome_browser_main.cc:1336: // Now the command line has been mutated based on about:flags, we can On 2016/09/19 20:05:20, gab wrote: > While you're here: s/Now the/Now that the/ I'll do this in a next change to make reverts easier in case something happens.
https://codereview.chromium.org/2342373002/diff/40001/chrome/browser/chrome_b... File chrome/browser/chrome_browser_main.cc (right): https://codereview.chromium.org/2342373002/diff/40001/chrome/browser/chrome_b... chrome/browser/chrome_browser_main.cc:1336: // Now the command line has been mutated based on about:flags, we can On 2016/09/19 20:08:34, robliao wrote: > On 2016/09/19 20:05:20, gab wrote: > > While you're here: s/Now the/Now that the/ > > I'll do this in a next change to make reverts easier in case something happens. https://codereview.chromium.org/2350943002/ tracks this change.
The CQ bit was checked by robliao@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from fdoray@chromium.org Link to the patchset: https://codereview.chromium.org/2342373002/#ps40001 (title: "CR Feedback")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Message was sent while issue was closed.
Committed patchset #3 (id:40001)
Message was sent while issue was closed.
Description was changed from ========== Initialize the Browser Task Scheduler Between Field Trials and Metrics This allows the Browser Task Scheduler to check variations before metrics initializes and runs items on the blocking pool. BUG=636518 ========== to ========== Initialize the Browser Task Scheduler Between Field Trials and Metrics This allows the Browser Task Scheduler to check variations before metrics initializes and runs items on the blocking pool. BUG=636518 Committed: https://crrev.com/3b73138b1a9173ec22856a2381c159fd6b68a1ba Cr-Commit-Position: refs/heads/master@{#419549} ==========
Message was sent while issue was closed.
Patchset 3 (id:??) landed as https://crrev.com/3b73138b1a9173ec22856a2381c159fd6b68a1ba Cr-Commit-Position: refs/heads/master@{#419549} |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
