Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2955953002:
[Cleanup] Migrate ExternalMetrics to use the Task Scheduler. (Closed)
|
Created:
3 years, 5 months ago by Ilya Sherman Modified:
3 years, 5 months ago CC:
chromium-reviews, alokp+watch_chromium.org, lcwu+watch_chromium.org, asvitkine+watch_chromium.org, halliwell+watch_chromium.org Target Ref:
refs/heads/master Project:
chromium Visibility:
Public. |
Description[Cleanup] Migrate ExternalMetrics to use the Task Scheduler.
BUG=667892
TEST=none
R=gfhuang@chromium.org
Review-Url: https://codereview.chromium.org/2955953002
Cr-Commit-Position: refs/heads/master@{#482998}
Committed: https://chromium.googlesource.com/chromium/src/+/b6580ce6bda3382b3954c26776c206f08c7d46c8
Patch Set 1 #
Total comments: 8
Patch Set 2 : Use a custom sequenced task runner #
Messages
Total messages: 21 (10 generated)
Hi Gaofeng, please take a careful look. I think that I've understood the threading constraints correctly, but they seem a bit subtle, so please double-check me! Useful documents: [1] and [2]. Thanks! [1] https://chromium.googlesource.com/chromium/src/+/master/docs/threading_and_ta... [2] https://chromium.googlesource.com/chromium/src/+/master/docs/task_scheduler_m...
https://codereview.chromium.org/2955953002/diff/1/chromecast/browser/metrics/... File chromecast/browser/metrics/external_metrics.cc (right): https://codereview.chromium.org/2955953002/diff/1/chromecast/browser/metrics/... chromecast/browser/metrics/external_metrics.cc:68: } should we add a sequence_checker? https://codereview.chromium.org/2955953002/diff/1/chromecast/browser/metrics/... chromecast/browser/metrics/external_metrics.cc:72: Are above 2 changes safe? Now the object could be destroyed outside of the sequence (in main thread I believe). https://codereview.chromium.org/2955953002/diff/1/chromecast/browser/metrics/... chromecast/browser/metrics/external_metrics.cc:85: weak_factory_.GetWeakPtr()), nit. BindOnce
https://codereview.chromium.org/2955953002/diff/1/chromecast/browser/metrics/... File chromecast/browser/metrics/external_metrics.cc (right): https://codereview.chromium.org/2955953002/diff/1/chromecast/browser/metrics/... chromecast/browser/metrics/external_metrics.cc:87: } I think another issue is here. because ScheduleCollection() starts a loop of CollectEvents(). This (manual) triggered ProcessExternalEvents() needs to be run in the same sequence. otherwise, 2 threads might enter CollectEvents() at the same time.
The CQ bit was checked by isherman@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...
https://codereview.chromium.org/2955953002/diff/1/chromecast/browser/metrics/... File chromecast/browser/metrics/external_metrics.cc (right): https://codereview.chromium.org/2955953002/diff/1/chromecast/browser/metrics/... chromecast/browser/metrics/external_metrics.cc:68: } On 2017/06/27 00:34:54, gfhuang wrote: > should we add a sequence_checker? Done. https://codereview.chromium.org/2955953002/diff/1/chromecast/browser/metrics/... chromecast/browser/metrics/external_metrics.cc:72: On 2017/06/27 00:34:54, gfhuang wrote: > Are above 2 changes safe? > > Now the object could be destroyed outside of the sequence (in main thread I > believe). Sorry, I missed that |this| was actually used within CollectEvents, so I oversimplified. Thanks for calling this out; I've restored the previous behavior now. https://codereview.chromium.org/2955953002/diff/1/chromecast/browser/metrics/... chromecast/browser/metrics/external_metrics.cc:85: weak_factory_.GetWeakPtr()), On 2017/06/27 00:34:54, gfhuang wrote: > nit. BindOnce Done. https://codereview.chromium.org/2955953002/diff/1/chromecast/browser/metrics/... chromecast/browser/metrics/external_metrics.cc:87: } On 2017/06/27 00:55:01, gfhuang wrote: > I think another issue is here. > > because ScheduleCollection() starts a loop of CollectEvents(). > This (manual) triggered ProcessExternalEvents() needs to be run in the same > sequence. > > otherwise, 2 threads might enter CollectEvents() at the same time. Yes, that's a good point, thanks! I got much too carried away trying to simplify yesterday. Done.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
On 2017/06/27 19:17:16, Ilya Sherman wrote: > https://codereview.chromium.org/2955953002/diff/1/chromecast/browser/metrics/... > File chromecast/browser/metrics/external_metrics.cc (right): > > https://codereview.chromium.org/2955953002/diff/1/chromecast/browser/metrics/... > chromecast/browser/metrics/external_metrics.cc:68: } > On 2017/06/27 00:34:54, gfhuang wrote: > > should we add a sequence_checker? > > Done. > > https://codereview.chromium.org/2955953002/diff/1/chromecast/browser/metrics/... > chromecast/browser/metrics/external_metrics.cc:72: > On 2017/06/27 00:34:54, gfhuang wrote: > > Are above 2 changes safe? > > > > Now the object could be destroyed outside of the sequence (in main thread I > > believe). > > Sorry, I missed that |this| was actually used within CollectEvents, so I > oversimplified. Thanks for calling this out; I've restored the previous > behavior now. > > https://codereview.chromium.org/2955953002/diff/1/chromecast/browser/metrics/... > chromecast/browser/metrics/external_metrics.cc:85: weak_factory_.GetWeakPtr()), > On 2017/06/27 00:34:54, gfhuang wrote: > > nit. BindOnce > > Done. > > https://codereview.chromium.org/2955953002/diff/1/chromecast/browser/metrics/... > chromecast/browser/metrics/external_metrics.cc:87: } > On 2017/06/27 00:55:01, gfhuang wrote: > > I think another issue is here. > > > > because ScheduleCollection() starts a loop of CollectEvents(). > > This (manual) triggered ProcessExternalEvents() needs to be run in the same > > sequence. > > > > otherwise, 2 threads might enter CollectEvents() at the same time. > > Yes, that's a good point, thanks! I got much too carried away trying to > simplify yesterday. Done. lgtm. thx
The CQ bit was checked by isherman@chromium.org
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
No L-G-T-M from a valid reviewer yet. CQ run can only be started once the patch has received an L-G-T-M from a full committer. Even if an L-G-T-M may have been provided, it was from a non-committer,_not_ a full super star committer. Committers are members of the group "project-chromium-committers". Note that this has nothing to do with OWNERS files.
isherman@chromium.org changed reviewers: + asvitkine@chromium.org
+Alexei for review-by-committe...r
lgtm
The CQ bit was checked by isherman@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": 1498664037978150,
"parent_rev": "554c9722c5a73ec099fa6055983e8d88d87db384", "commit_rev":
"b6580ce6bda3382b3954c26776c206f08c7d46c8"}
Message was sent while issue was closed.
Description was changed from ========== [Cleanup] Migrate ExternalMetrics to use the Task Scheduler. BUG=667892 TEST=none R=gfhuang@chromium.org ========== to ========== [Cleanup] Migrate ExternalMetrics to use the Task Scheduler. BUG=667892 TEST=none R=gfhuang@chromium.org Review-Url: https://codereview.chromium.org/2955953002 Cr-Commit-Position: refs/heads/master@{#482998} Committed: https://chromium.googlesource.com/chromium/src/+/b6580ce6bda3382b3954c26776c2... ==========
Message was sent while issue was closed.
Committed patchset #2 (id:20001) as https://chromium.googlesource.com/chromium/src/+/b6580ce6bda3382b3954c26776c2... |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
