Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2566323002:
Defer Posting StoreMetricsClientInfo During PreCreateThreads (Closed)
DescriptionDefer Posting StoreMetricsClientInfo During PreCreateThreads
The BlockingPool isn't ready to go during PreCreateThreads. This CL
posts a task to UI's message loop to post the real task to call
StoreMetricsClientInfo.
BUG=670137
Committed: https://crrev.com/14cdd1233c48fdd5268dbdc4ad5219c638ee22d7
Cr-Commit-Position: refs/heads/master@{#438254}
Patch Set 1 #
Total comments: 2
Patch Set 2 : Update Comment #Messages
Total messages: 31 (17 generated)
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...
robliao@chromium.org changed reviewers: + fdoray@chromium.org, gab@chromium.org
Can this sort of thing be done internally by blocking pool's posttask? This way, we don't need to have the complexity on the call site. On Mon, Dec 12, 2016 at 6:17 PM, <robliao@chromium.org> wrote: > Reviewers: gab, fdoray > CL: https://codereview.chromium.org/2566323002/ > > Description: > Defer Posting StoreMetricsClientInfo During PreCreateThreads > > The BlockingPool isn't ready to go during PreCreateThreads. This CL > posts a task to UI's message loop to post the real task to call > StoreMetricsClientInfo. > > BUG=670137 > > Affected files (+12, -3 lines): > M chrome/browser/metrics/chrome_metrics_services_manager_client.cc > > > Index: chrome/browser/metrics/chrome_metrics_services_manager_client.cc > diff --git a/chrome/browser/metrics/chrome_metrics_services_manager_client.cc > b/chrome/browser/metrics/chrome_metrics_services_manager_client.cc > index 82a5a10ac877320c1b619cf3acae581465813315.. > 628d8ce881270f4263a65781184b0dd60d171275 100644 > --- a/chrome/browser/metrics/chrome_metrics_services_manager_client.cc > +++ b/chrome/browser/metrics/chrome_metrics_services_manager_client.cc > @@ -59,9 +59,18 @@ const base::Feature kMetricsReportingFeature{" > MetricsReporting", > // Posts |GoogleUpdateSettings::StoreMetricsClientInfo| on blocking pool > thread > // because it needs access to IO and cannot work from UI thread. > void PostStoreMetricsClientInfo(const metrics::ClientInfo& client_info) { > - content::BrowserThread::GetBlockingPool()->PostTask( > - FROM_HERE, > - base::Bind(&GoogleUpdateSettings::StoreMetricsClientInfo, client_info)); > + // The delayed post task allows this function to get called before the > + // blocking pool is ready to go. > + content::BrowserThread::PostTask( > + content::BrowserThread::UI, FROM_HERE, > + base::Bind( > + [](const metrics::ClientInfo& client_info) { > + content::BrowserThread::PostBlockingPoolTask( > + FROM_HERE, > + base::Bind(&GoogleUpdateSettings::StoreMetricsClientInfo, > + client_info)); > + }, > + client_info)); > } > > // Appends a group to the sampling controlling |trial|. The group will be > > > -- You received this message because you are subscribed to the Google Groups "Chromium-reviews" group. To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
On 2016/12/12 23:29:57, chromium-reviews wrote: > Can this sort of thing be done internally by blocking pool's posttask? This > way, we don't need to have the complexity on the call site. > > On Mon, Dec 12, 2016 at 6:17 PM, <mailto:robliao@chromium.org> wrote: > > > Reviewers: gab, fdoray > > CL: https://codereview.chromium.org/2566323002/ > > > > Description: > > Defer Posting StoreMetricsClientInfo During PreCreateThreads > > > > The BlockingPool isn't ready to go during PreCreateThreads. This CL > > posts a task to UI's message loop to post the real task to call > > StoreMetricsClientInfo. > > > > BUG=670137 > > > > Affected files (+12, -3 lines): > > M chrome/browser/metrics/chrome_metrics_services_manager_client.cc > > > > > > Index: chrome/browser/metrics/chrome_metrics_services_manager_client.cc > > diff --git a/chrome/browser/metrics/chrome_metrics_services_manager_client.cc > > b/chrome/browser/metrics/chrome_metrics_services_manager_client.cc > > index 82a5a10ac877320c1b619cf3acae581465813315.. > > 628d8ce881270f4263a65781184b0dd60d171275 100644 > > --- a/chrome/browser/metrics/chrome_metrics_services_manager_client.cc > > +++ b/chrome/browser/metrics/chrome_metrics_services_manager_client.cc > > @@ -59,9 +59,18 @@ const base::Feature kMetricsReportingFeature{" > > MetricsReporting", > > // Posts |GoogleUpdateSettings::StoreMetricsClientInfo| on blocking pool > > thread > > // because it needs access to IO and cannot work from UI thread. > > void PostStoreMetricsClientInfo(const metrics::ClientInfo& client_info) { > > - content::BrowserThread::GetBlockingPool()->PostTask( > > - FROM_HERE, > > - base::Bind(&GoogleUpdateSettings::StoreMetricsClientInfo, client_info)); > > + // The delayed post task allows this function to get called before the > > + // blocking pool is ready to go. > > + content::BrowserThread::PostTask( > > + content::BrowserThread::UI, FROM_HERE, > > + base::Bind( > > + [](const metrics::ClientInfo& client_info) { > > + content::BrowserThread::PostBlockingPoolTask( > > + FROM_HERE, > > + base::Bind(&GoogleUpdateSettings::StoreMetricsClientInfo, > > + client_info)); > > + }, > > + client_info)); > > } > > > > // Appends a group to the sampling controlling |trial|. The group will be > > > > > > > > -- > You received this message because you are subscribed to the Google Groups > "Chromium-reviews" group. > To unsubscribe from this group and stop receiving emails from it, send an email > to mailto:chromium-reviews+unsubscribe@chromium.org. Because this is one of, if not the only, remaining task being posted before the blocking pool is ready, our preference is to defer this call. New startup code going forward needs to be aware of the current startup phase.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_chromium_chromeos_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
On 2016/12/13 00:10:14, robliao wrote: > On 2016/12/12 23:29:57, chromium-reviews wrote: > > Can this sort of thing be done internally by blocking pool's posttask? This > > way, we don't need to have the complexity on the call site. > > > > On Mon, Dec 12, 2016 at 6:17 PM, <mailto:robliao@chromium.org> wrote: > > > > > Reviewers: gab, fdoray > > > CL: https://codereview.chromium.org/2566323002/ > > > > > > Description: > > > Defer Posting StoreMetricsClientInfo During PreCreateThreads > > > > > > The BlockingPool isn't ready to go during PreCreateThreads. This CL > > > posts a task to UI's message loop to post the real task to call > > > StoreMetricsClientInfo. > > > > > > BUG=670137 > > > > > > Affected files (+12, -3 lines): > > > M chrome/browser/metrics/chrome_metrics_services_manager_client.cc > > > > > > > > > Index: chrome/browser/metrics/chrome_metrics_services_manager_client.cc > > > diff --git > a/chrome/browser/metrics/chrome_metrics_services_manager_client.cc > > > b/chrome/browser/metrics/chrome_metrics_services_manager_client.cc > > > index 82a5a10ac877320c1b619cf3acae581465813315.. > > > 628d8ce881270f4263a65781184b0dd60d171275 100644 > > > --- a/chrome/browser/metrics/chrome_metrics_services_manager_client.cc > > > +++ b/chrome/browser/metrics/chrome_metrics_services_manager_client.cc > > > @@ -59,9 +59,18 @@ const base::Feature kMetricsReportingFeature{" > > > MetricsReporting", > > > // Posts |GoogleUpdateSettings::StoreMetricsClientInfo| on blocking pool > > > thread > > > // because it needs access to IO and cannot work from UI thread. > > > void PostStoreMetricsClientInfo(const metrics::ClientInfo& client_info) { > > > - content::BrowserThread::GetBlockingPool()->PostTask( > > > - FROM_HERE, > > > - base::Bind(&GoogleUpdateSettings::StoreMetricsClientInfo, client_info)); > > > + // The delayed post task allows this function to get called before the > > > + // blocking pool is ready to go. > > > + content::BrowserThread::PostTask( > > > + content::BrowserThread::UI, FROM_HERE, > > > + base::Bind( > > > + [](const metrics::ClientInfo& client_info) { > > > + content::BrowserThread::PostBlockingPoolTask( > > > + FROM_HERE, > > > + base::Bind(&GoogleUpdateSettings::StoreMetricsClientInfo, > > > + client_info)); > > > + }, > > > + client_info)); > > > } > > > > > > // Appends a group to the sampling controlling |trial|. The group will be > > > > > > > > > > > > > -- > > You received this message because you are subscribed to the Google Groups > > "Chromium-reviews" group. > > To unsubscribe from this group and stop receiving emails from it, send an > email > > to mailto:chromium-reviews+unsubscribe@chromium.org. > > Because this is one of, if not the only, remaining task being posted before the > blocking pool is ready, our preference is to defer this call. > New startup code going forward needs to be aware of the current startup phase. Exactly, the problem is that: yes the scheduler could queue tasks and only start running them when initialized but that prevents having A DCHECK when a task is posted and no TaskScheduler is ever initialized which could potentially result in subtle bugs say a component unknowingly posts a task from a process with no scheduler. Given the use case of requiring to post before CreateThreads is very minimal we prefer having such tasks post themselves through the main MessageLoop (when we know the scheduler will be alive).
asvitkine@chromium.org changed reviewers: + asvitkine@chromium.org
lgtm ... but I still worry that over time more places will need to do this and it will result in more complexity at call sites (i.e. it's surprising to me that metrics service is the only thing that runs into this).
On 2016/12/13 15:32:15, Alexei Svitkine (OO from 15th) wrote: > lgtm > > ... but I still worry that over time more places will need to do this and it > will result in more complexity at call sites (i.e. it's surprising to me that > metrics service is the only thing that runs into this). If that becomes the case we can add support for queuing tasks prior to scheduler initialization, but we'd rather not if we can avoid it.
https://codereview.chromium.org/2566323002/diff/1/chrome/browser/metrics/chro... File chrome/browser/metrics/chrome_metrics_services_manager_client.cc (right): https://codereview.chromium.org/2566323002/diff/1/chrome/browser/metrics/chro... chrome/browser/metrics/chrome_metrics_services_manager_client.cc:63: // blocking pool is ready to go. (this sentence is backwards?) // Posting the task from the main message loop ensures the blocking pool is initialized (which it isn't always at this point).
lgtm +1 to gab@'s comment
Patchset #2 (id:20001) has been deleted
https://codereview.chromium.org/2566323002/diff/1/chrome/browser/metrics/chro... File chrome/browser/metrics/chrome_metrics_services_manager_client.cc (right): https://codereview.chromium.org/2566323002/diff/1/chrome/browser/metrics/chro... chrome/browser/metrics/chrome_metrics_services_manager_client.cc:63: // blocking pool is ready to go. On 2016/12/13 15:58:39, gab wrote: > (this sentence is backwards?) > > // Posting the task from the main message loop ensures the blocking pool is > initialized (which it isn't always at this point). Clarified to // The message loop processes messages after the blocking pool is initialized. // Posting a task to the message loop to post a task to the blocking pool // ensures that the blocking pool is ready to accept tasks at that time.
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...
robliao@chromium.org changed reviewers: + brettw@chromium.org
brettw@chromium.org: Please review this CL. Thanks!
lgtm
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 robliao@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from asvitkine@chromium.org, fdoray@chromium.org Link to the patchset: https://codereview.chromium.org/2566323002/#ps40001 (title: "Update 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": 40001, "attempt_start_ts": 1481657073398940,
"parent_rev": "70f446d289bf9bfaa7c28336c3560d6d5049612d", "commit_rev":
"cfc7ceb5606f849e7027bffa581451a3ea1da950"}
Message was sent while issue was closed.
Description was changed from ========== Defer Posting StoreMetricsClientInfo During PreCreateThreads The BlockingPool isn't ready to go during PreCreateThreads. This CL posts a task to UI's message loop to post the real task to call StoreMetricsClientInfo. BUG=670137 ========== to ========== Defer Posting StoreMetricsClientInfo During PreCreateThreads The BlockingPool isn't ready to go during PreCreateThreads. This CL posts a task to UI's message loop to post the real task to call StoreMetricsClientInfo. BUG=670137 Review-Url: https://codereview.chromium.org/2566323002 ==========
Message was sent while issue was closed.
Committed patchset #2 (id:40001)
Message was sent while issue was closed.
Description was changed from ========== Defer Posting StoreMetricsClientInfo During PreCreateThreads The BlockingPool isn't ready to go during PreCreateThreads. This CL posts a task to UI's message loop to post the real task to call StoreMetricsClientInfo. BUG=670137 Review-Url: https://codereview.chromium.org/2566323002 ========== to ========== Defer Posting StoreMetricsClientInfo During PreCreateThreads The BlockingPool isn't ready to go during PreCreateThreads. This CL posts a task to UI's message loop to post the real task to call StoreMetricsClientInfo. BUG=670137 Committed: https://crrev.com/14cdd1233c48fdd5268dbdc4ad5219c638ee22d7 Cr-Commit-Position: refs/heads/master@{#438254} ==========
Message was sent while issue was closed.
Patchset 2 (id:??) landed as https://crrev.com/14cdd1233c48fdd5268dbdc4ad5219c638ee22d7 Cr-Commit-Position: refs/heads/master@{#438254} |
