Chromium Code Reviews
chromiumcodereview-hr@appspot.gserviceaccount.com (chromiumcodereview-hr) | Please choose your nickname with Settings | Help | Chromium Project | Gerrit Changes | Sign out
(119)

Issue 1705943002: TaskScheduler [5/9] Task Tracker (Closed)

Created:
4 years, 10 months ago by fdoray
Modified:
4 years, 8 months ago
CC:
chromium-reviews, vmpstr+watch_chromium.org
Base URL:
https://chromium.googlesource.com/chromium/src.git@s_3_pq
Target Ref:
refs/pending/heads/master
Project:
chromium
Visibility:
Public.

Description

TaskScheduler [5/9] Task Tracker This change is a subset of https://codereview.chromium.org/1698183005/ All tasks go through the scheduler's TaskTracker when they are posted and when they are executed. The TaskTracker enforces shutdown semantics and takes care of tracing and profiling. We have an alternate implementation which uses atomic operations to reduce lock contention. However, we prefer to check this simple implementation in first and bring the atomic operations in an incremental CL. BUG=553459 Committed: https://crrev.com/0fc7a6669c12c2ef2d1ccbd496232b7c65f6c81c Cr-Commit-Position: refs/heads/master@{#383749}

Patch Set 1 #

Patch Set 2 : rebase #

Total comments: 25

Patch Set 3 : ShutdownManager -> TaskTracker #

Patch Set 4 : rebase #

Patch Set 5 : self review #

Total comments: 1

Patch Set 6 : fix PostTask comment #

Total comments: 43

Patch Set 7 : address comments gab+robliao 15-17 #

Patch Set 8 : self review #

Patch Set 9 : self review #

Patch Set 10 : expand tests #

Total comments: 28

Patch Set 11 : CR gab #19 #

Total comments: 25

Patch Set 12 : CR from gab #23 #

Patch Set 13 : self review #

Total comments: 16

Patch Set 14 : CR gab/robliao #26-27 #

Patch Set 15 : CR gab/robliao #26-27 #

Patch Set 16 : self review #

Total comments: 16

Patch Set 17 : self review #

Patch Set 18 : self review #

Total comments: 2

Patch Set 19 : CR gab/asvitkine #31-35 #

Total comments: 18

Patch Set 20 : CR gab/danakj #38-39 #

Patch Set 21 : CR gab/danakj #38-39 #

Total comments: 2

Patch Set 22 : CR gab #41 (fix a comment) #

Total comments: 3

Patch Set 23 : fix build errors #

Patch Set 24 : fix build errors #

Patch Set 25 : CR robliao #61 #

Unified diffs Side-by-side diffs Delta from patch set Stats (+654 lines, -6 lines) Patch
M base/BUILD.gn View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 20 21 22 2 chunks +3 lines, -0 lines 0 comments Download
M base/base.gyp View 1 2 3 4 5 6 7 8 9 10 11 1 chunk +1 line, -0 lines 0 comments Download
M base/base.gypi View 1 2 3 4 5 6 7 8 9 10 11 1 chunk +2 lines, -0 lines 0 comments Download
A base/task_scheduler/task_tracker.h View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 20 21 22 23 1 chunk +95 lines, -0 lines 0 comments Download
A base/task_scheduler/task_tracker.cc View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 20 21 22 23 1 chunk +183 lines, -0 lines 0 comments Download
A base/task_scheduler/task_tracker_unittest.cc View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 20 21 22 1 chunk +332 lines, -0 lines 0 comments Download
M base/task_scheduler/task_traits.h View 1 2 3 4 5 6 7 8 9 10 11 1 chunk +8 lines, -2 lines 0 comments Download
M base/task_scheduler/task_traits.cc View 1 2 3 4 5 6 7 8 9 10 11 1 chunk +21 lines, -4 lines 0 comments Download
M tools/metrics/histograms/histograms.xml View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 20 21 22 1 chunk +9 lines, -0 lines 0 comments Download

Dependent Patchsets:

Messages

Total messages: 68 (19 generated)
fdoray
4 years, 10 months ago (2016-02-17 20:12:02 UTC) #1
gab
1st pass, would like you two's input on my mega meta comment about the API ...
4 years, 10 months ago (2016-02-23 22:28:23 UTC) #4
robliao
Just responding to the large comment. https://codereview.chromium.org/1705943002/diff/20001/base/task_scheduler/shutdown_manager.h File base/task_scheduler/shutdown_manager.h (right): https://codereview.chromium.org/1705943002/diff/20001/base/task_scheduler/shutdown_manager.h#newcode34 base/task_scheduler/shutdown_manager.h:34: bool ShouldPostTask(TaskShutdownBehavior shutdown_behavior); ...
4 years, 10 months ago (2016-02-23 22:47:02 UTC) #5
robliao
One nit and waiting for the resolution of the API. https://codereview.chromium.org/1705943002/diff/20001/base/task_scheduler/shutdown_manager.cc File base/task_scheduler/shutdown_manager.cc (right): https://codereview.chromium.org/1705943002/diff/20001/base/task_scheduler/shutdown_manager.cc#newcode57 ...
4 years, 10 months ago (2016-02-24 01:22:45 UTC) #6
fdoray
I just replied to the big comment. https://codereview.chromium.org/1705943002/diff/20001/base/task_scheduler/shutdown_manager.h File base/task_scheduler/shutdown_manager.h (right): https://codereview.chromium.org/1705943002/diff/20001/base/task_scheduler/shutdown_manager.h#newcode34 base/task_scheduler/shutdown_manager.h:34: bool ShouldPostTask(TaskShutdownBehavior ...
4 years, 10 months ago (2016-02-24 17:01:06 UTC) #7
gab
On 2016/02/24 17:01:06, fdoray wrote: > I just replied to the big comment. > > ...
4 years, 10 months ago (2016-02-24 23:35:41 UTC) #8
fdoray
Can you review this CL again? ShutdownManager is now TaskTracker. https://codereview.chromium.org/1705943002/diff/20001/base/task_scheduler/shutdown_manager.cc File base/task_scheduler/shutdown_manager.cc (right): https://codereview.chromium.org/1705943002/diff/20001/base/task_scheduler/shutdown_manager.cc#newcode24 ...
4 years, 10 months ago (2016-02-26 15:53:28 UTC) #11
robliao
https://codereview.chromium.org/1705943002/diff/80001/base/task_scheduler/task_tracker.h File base/task_scheduler/task_tracker.h (right): https://codereview.chromium.org/1705943002/diff/80001/base/task_scheduler/task_tracker.h#newcode38 base/task_scheduler/task_tracker.h:38: void PostTask(const Callback<void(scoped_ptr<Task>)>& post_task_continuation, A part of the previous ...
4 years, 10 months ago (2016-02-26 22:16:50 UTC) #12
fdoray
I liked the TaskTracker { PostTask(), RunTask() } because it could have been a good ...
4 years, 9 months ago (2016-02-29 23:08:24 UTC) #13
robliao
On 2016/02/29 23:08:24, fdoray wrote: > I liked the TaskTracker { PostTask(), RunTask() } because ...
4 years, 9 months ago (2016-03-01 01:01:58 UTC) #14
gab
https://codereview.chromium.org/1705943002/diff/100001/base/task_scheduler/task_tracker.cc File base/task_scheduler/task_tracker.cc (right): https://codereview.chromium.org/1705943002/diff/100001/base/task_scheduler/task_tracker.cc#newcode50 base/task_scheduler/task_tracker.cc:50: task_annotator.DidQueueTask(kQueueFunctionName, *task.get()); Not your fault but noticing that "DidQueueTask" ...
4 years, 9 months ago (2016-03-01 22:18:47 UTC) #15
robliao
https://codereview.chromium.org/1705943002/diff/100001/base/task_scheduler/task_tracker.cc File base/task_scheduler/task_tracker.cc (right): https://codereview.chromium.org/1705943002/diff/100001/base/task_scheduler/task_tracker.cc#newcode69 base/task_scheduler/task_tracker.cc:69: bool TaskTracker::RequestPostTask(TaskShutdownBehavior shutdown_behavior) { Now that these are private, ...
4 years, 9 months ago (2016-03-01 22:29:42 UTC) #16
robliao
https://codereview.chromium.org/1705943002/diff/100001/base/task_scheduler/task_tracker_unittest.cc File base/task_scheduler/task_tracker_unittest.cc (right): https://codereview.chromium.org/1705943002/diff/100001/base/task_scheduler/task_tracker_unittest.cc#newcode376 base/task_scheduler/task_tracker_unittest.cc:376: } On 2016/03/01 22:18:47, gab wrote: > To avoid ...
4 years, 9 months ago (2016-03-01 22:36:42 UTC) #17
fdoray
https://codereview.chromium.org/1705943002/diff/100001/base/task_scheduler/task_tracker.cc File base/task_scheduler/task_tracker.cc (right): https://codereview.chromium.org/1705943002/diff/100001/base/task_scheduler/task_tracker.cc#newcode50 base/task_scheduler/task_tracker.cc:50: task_annotator.DidQueueTask(kQueueFunctionName, *task.get()); On 2016/03/01 22:18:47, gab wrote: > Not ...
4 years, 9 months ago (2016-03-02 00:38:41 UTC) #18
gab
Looked mostly at diff since last round. Will make a full pass after this. Thanks! ...
4 years, 9 months ago (2016-03-09 21:53:26 UTC) #19
robliao
https://codereview.chromium.org/1705943002/diff/180001/base/task_scheduler/task_tracker_unittest.cc File base/task_scheduler/task_tracker_unittest.cc (right): https://codereview.chromium.org/1705943002/diff/180001/base/task_scheduler/task_tracker_unittest.cc#newcode135 base/task_scheduler/task_tracker_unittest.cc:135: TEST_P(TaskSchedulerTaskTrackerTest, PostAndRunBeforeShutdown) { If the parametrized tests are like ...
4 years, 9 months ago (2016-03-09 22:31:47 UTC) #20
fdoray
https://codereview.chromium.org/1705943002/diff/180001/base/task_scheduler/task_tracker.cc File base/task_scheduler/task_tracker.cc (right): https://codereview.chromium.org/1705943002/diff/180001/base/task_scheduler/task_tracker.cc#newcode101 base/task_scheduler/task_tracker.cc:101: // SKIP_ON_SHUTDOWN tasks block shutdown when they are running. ...
4 years, 9 months ago (2016-03-15 17:28:09 UTC) #22
gab
lvgtm :-), mostly nits below https://codereview.chromium.org/1705943002/diff/200001/base/task_scheduler/task_tracker.cc File base/task_scheduler/task_tracker.cc (right): https://codereview.chromium.org/1705943002/diff/200001/base/task_scheduler/task_tracker.cc#newcode18 base/task_scheduler/task_tracker.cc:18: : cv_(lock_.CreateConditionVariable()), Since we ...
4 years, 9 months ago (2016-03-17 01:42:13 UTC) #23
fdoray
gab@: All done. https://codereview.chromium.org/1705943002/diff/200001/base/task_scheduler/task_tracker.cc File base/task_scheduler/task_tracker.cc (right): https://codereview.chromium.org/1705943002/diff/200001/base/task_scheduler/task_tracker.cc#newcode18 base/task_scheduler/task_tracker.cc:18: : cv_(lock_.CreateConditionVariable()), On 2016/03/17 01:42:12, gab ...
4 years, 9 months ago (2016-03-17 20:12:16 UTC) #25
robliao
https://codereview.chromium.org/1705943002/diff/240001/base/task_scheduler/task_tracker.cc File base/task_scheduler/task_tracker.cc (right): https://codereview.chromium.org/1705943002/diff/240001/base/task_scheduler/task_tracker.cc#newcode81 base/task_scheduler/task_tracker.cc:81: // TODO(fdoray): Add a UMA histogram with the number ...
4 years, 9 months ago (2016-03-17 23:34:21 UTC) #26
gab
lgtm :-) w/ nits https://codereview.chromium.org/1705943002/diff/200001/base/task_scheduler/task_traits.h File base/task_scheduler/task_traits.h (right): https://codereview.chromium.org/1705943002/diff/200001/base/task_scheduler/task_traits.h#newcode139 base/task_scheduler/task_traits.h:139: // Stream operator so TaskPriority ...
4 years, 9 months ago (2016-03-18 18:48:17 UTC) #27
fdoray
https://codereview.chromium.org/1705943002/diff/240001/base/task_scheduler/task_tracker.cc File base/task_scheduler/task_tracker.cc (right): https://codereview.chromium.org/1705943002/diff/240001/base/task_scheduler/task_tracker.cc#newcode81 base/task_scheduler/task_tracker.cc:81: // TODO(fdoray): Add a UMA histogram with the number ...
4 years, 9 months ago (2016-03-18 20:35:39 UTC) #28
gab
https://codereview.chromium.org/1705943002/diff/240001/base/task_scheduler/task_tracker_unittest.cc File base/task_scheduler/task_tracker_unittest.cc (right): https://codereview.chromium.org/1705943002/diff/240001/base/task_scheduler/task_tracker_unittest.cc#newcode239 base/task_scheduler/task_tracker_unittest.cc:239: // It is not possible to test running after ...
4 years, 9 months ago (2016-03-21 17:42:30 UTC) #31
fdoray
Dana: Can you review this CL? Alexei: Can you review histograms.xml + lines 43-45 of ...
4 years, 9 months ago (2016-03-21 17:42:34 UTC) #33
fdoray
Dana: I just received comments from Gab. You can wait until they are addressed to ...
4 years, 9 months ago (2016-03-21 17:44:39 UTC) #34
Alexei Svitkine (slow)
lgtm https://codereview.chromium.org/1705943002/diff/340001/tools/metrics/histograms/histograms.xml File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/1705943002/diff/340001/tools/metrics/histograms/histograms.xml#newcode53146 tools/metrics/histograms/histograms.xml:53146: + units="Tasks"> Nit: No need to capitalize units.
4 years, 9 months ago (2016-03-21 18:14:35 UTC) #35
fdoray
gab@: Can you take another look? https://codereview.chromium.org/1705943002/diff/240001/base/task_scheduler/task_tracker_unittest.cc File base/task_scheduler/task_tracker_unittest.cc (right): https://codereview.chromium.org/1705943002/diff/240001/base/task_scheduler/task_tracker_unittest.cc#newcode239 base/task_scheduler/task_tracker_unittest.cc:239: // It is ...
4 years, 9 months ago (2016-03-21 19:08:08 UTC) #36
Alexei Svitkine (slow)
https://codereview.chromium.org/1705943002/diff/300001/tools/metrics/histograms/histograms.xml File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/1705943002/diff/300001/tools/metrics/histograms/histograms.xml#newcode53009 tools/metrics/histograms/histograms.xml:53009: + units="Tasks"> On 2016/03/21 19:08:08, fdoray wrote: > On ...
4 years, 9 months ago (2016-03-21 19:35:35 UTC) #37
gab
https://codereview.chromium.org/1705943002/diff/300001/base/task_scheduler/task_tracker.cc File base/task_scheduler/task_tracker.cc (right): https://codereview.chromium.org/1705943002/diff/300001/base/task_scheduler/task_tracker.cc#newcode39 base/task_scheduler/task_tracker.cc:39: num_tasks_blocking_shutdown_copy = num_tasks_blocking_shutdown_; On 2016/03/21 19:08:07, fdoray wrote: > ...
4 years, 9 months ago (2016-03-21 19:53:25 UTC) #38
danakj
https://codereview.chromium.org/1705943002/diff/360001/base/task_scheduler/task_tracker.cc File base/task_scheduler/task_tracker.cc (right): https://codereview.chromium.org/1705943002/diff/360001/base/task_scheduler/task_tracker.cc#newcode36 base/task_scheduler/task_tracker.cc:36: DCHECK(!shutdown_completed_ && !shutdown_cv_) Don't use && in DCHECK. Write ...
4 years, 9 months ago (2016-03-22 18:56:56 UTC) #39
fdoray
danakj@: Can you take another look? Thanks. https://codereview.chromium.org/1705943002/diff/360001/base/task_scheduler/task_tracker.cc File base/task_scheduler/task_tracker.cc (right): https://codereview.chromium.org/1705943002/diff/360001/base/task_scheduler/task_tracker.cc#newcode36 base/task_scheduler/task_tracker.cc:36: DCHECK(!shutdown_completed_ && ...
4 years, 9 months ago (2016-03-22 20:19:43 UTC) #40
gab
lgtm++ for PS21, thanks! https://codereview.chromium.org/1705943002/diff/360001/base/task_scheduler/task_tracker.cc File base/task_scheduler/task_tracker.cc (right): https://codereview.chromium.org/1705943002/diff/360001/base/task_scheduler/task_tracker.cc#newcode103 base/task_scheduler/task_tracker.cc:103: DCHECK_NE(TaskShutdownBehavior::BLOCK_SHUTDOWN, shutdown_behavior); On 2016/03/22 20:19:43, ...
4 years, 9 months ago (2016-03-24 13:00:39 UTC) #41
fdoray
gab@: Done. danakj@: Could you take another look? Pending question: Should we change TaskTracker::PostTask(Closure&, scoped_ptr<Task>) ...
4 years, 9 months ago (2016-03-24 14:23:51 UTC) #42
gab
https://codereview.chromium.org/1705943002/diff/360001/base/task_scheduler/task_tracker.h File base/task_scheduler/task_tracker.h (right): https://codereview.chromium.org/1705943002/diff/360001/base/task_scheduler/task_tracker.h#newcode39 base/task_scheduler/task_tracker.h:39: void PostTask(const Callback<void(scoped_ptr<Task>)>& post_task_callback, On 2016/03/22 20:19:43, fdoray wrote: ...
4 years, 9 months ago (2016-03-24 14:50:12 UTC) #43
robliao
https://codereview.chromium.org/1705943002/diff/360001/base/task_scheduler/task_tracker.h File base/task_scheduler/task_tracker.h (right): https://codereview.chromium.org/1705943002/diff/360001/base/task_scheduler/task_tracker.h#newcode39 base/task_scheduler/task_tracker.h:39: void PostTask(const Callback<void(scoped_ptr<Task>)>& post_task_callback, On 2016/03/24 14:50:12, gab (OOO ...
4 years, 9 months ago (2016-03-24 15:46:04 UTC) #44
danakj
On 2016/03/24 14:50:12, gab (OOO until Apr. 5) wrote: > https://codereview.chromium.org/1705943002/diff/360001/base/task_scheduler/task_tracker.h > File base/task_scheduler/task_tracker.h (right): ...
4 years, 8 months ago (2016-03-28 20:09:58 UTC) #45
danakj
LGTM
4 years, 8 months ago (2016-03-28 20:10:06 UTC) #46
robliao
https://codereview.chromium.org/1705943002/diff/420001/base/task_scheduler/test_utils.h File base/task_scheduler/test_utils.h (right): https://codereview.chromium.org/1705943002/diff/420001/base/task_scheduler/test_utils.h#newcode16 base/task_scheduler/test_utils.h:16: #define EXPECT_DCHECK_DEATH(statement, regex) statement Intentional change? Didn't see the ...
4 years, 8 months ago (2016-03-29 00:42:03 UTC) #47
commit-bot: I haz the power
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1705943002/420001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1705943002/420001
4 years, 8 months ago (2016-03-29 00:42:35 UTC) #49
commit-bot: I haz the power
Dry run: Try jobs failed on following builders: ios_dbg_simulator_gn on tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios_dbg_simulator_gn/builds/10070) ios_rel_device_ninja on ...
4 years, 8 months ago (2016-03-29 00:47:09 UTC) #51
fdoray
https://codereview.chromium.org/1705943002/diff/420001/base/task_scheduler/test_utils.h File base/task_scheduler/test_utils.h (right): https://codereview.chromium.org/1705943002/diff/420001/base/task_scheduler/test_utils.h#newcode16 base/task_scheduler/test_utils.h:16: #define EXPECT_DCHECK_DEATH(statement, regex) statement On 2016/03/29 00:42:03, robliao wrote: ...
4 years, 8 months ago (2016-03-29 11:31:49 UTC) #52
commit-bot: I haz the power
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1705943002/440001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1705943002/440001
4 years, 8 months ago (2016-03-29 11:32:17 UTC) #54
commit-bot: I haz the power
Dry run: Try jobs failed on following builders: win_chromium_x64_rel_ng on tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_rel_ng/builds/188514)
4 years, 8 months ago (2016-03-29 12:29:25 UTC) #56
commit-bot: I haz the power
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1705943002/460001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1705943002/460001
4 years, 8 months ago (2016-03-29 12:35:26 UTC) #58
commit-bot: I haz the power
Dry run: This issue passed the CQ dry run.
4 years, 8 months ago (2016-03-29 14:05:24 UTC) #60
robliao
lgtm + 1 nit. https://codereview.chromium.org/1705943002/diff/420001/base/task_scheduler/test_utils.h File base/task_scheduler/test_utils.h (right): https://codereview.chromium.org/1705943002/diff/420001/base/task_scheduler/test_utils.h#newcode16 base/task_scheduler/test_utils.h:16: #define EXPECT_DCHECK_DEATH(statement, regex) statement On ...
4 years, 8 months ago (2016-03-29 15:28:10 UTC) #61
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1705943002/480001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1705943002/480001
4 years, 8 months ago (2016-03-29 16:00:10 UTC) #64
commit-bot: I haz the power
Committed patchset #25 (id:480001)
4 years, 8 months ago (2016-03-29 17:13:07 UTC) #66
commit-bot: I haz the power
4 years, 8 months ago (2016-03-29 17:14:08 UTC) #68
Message was sent while issue was closed.
Patchset 25 (id:??) landed as
https://crrev.com/0fc7a6669c12c2ef2d1ccbd496232b7c65f6c81c
Cr-Commit-Position: refs/heads/master@{#383749}

Powered by Google App Engine
This is Rietveld 408576698