Chromium Code Reviews
Help | Chromium Project | Sign in
(9)

Issue 2729523006: Remove the |max_threads| argument from CreateAndSetSimpleTaskScheduler(). (Closed)

Can't Edit
Can't Publish+Mail
Start Review
Created:
1 month, 3 weeks ago by fdoray
Modified:
1 month ago
CC:
chromium-reviews, ozone-reviews_chromium.org, cbentzel+watch_chromium.org, sadrul, vmpstr+watch_chromium.org, robliao+watch_chromium.org, chromoting-reviews_chromium.org, net-reviews_chromium.org, gab+watch_chromium.org, kalyank, fdoray+watch_chromium.org, pauljensen
Target Ref:
refs/heads/master
Project:
chromium
Visibility:
Public.

Description

Remove the |max_threads| argument from CreateAndSetSimpleTaskScheduler(). We want to change TaskSchedulerImpl so that it always has 4 thread pools. This is incompatible with a CreateAndSetSimpleTaskScheduler() function that takes a global maximum number of threads as argument. Therefore, this CL removes that argument. Since existing callers didn't have comments to justify how the value of this argument was chosen, they shouldn't care too much about this change. This CL also adds a |name| argument to CreateAndSetSimpleTaskScheduler() that is used to label threads and histograms associated with the created TaskScheduler. BUG=690706 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.android:android_cronet_tester Review-Url: https://codereview.chromium.org/2729523006 Cr-Commit-Position: refs/heads/master@{#458450} Committed: https://chromium.googlesource.com/chromium/src/+/dafc3415b9ca6afe4c08690891e6331280ee7bbb

Patch Set 1 #

Patch Set 2 : rebase #

Patch Set 3 : self-review #

Total comments: 10

Patch Set 4 : CR robliao #8 #

Patch Set 5 : rebase #

Total comments: 2

Patch Set 6 : CR robliao #11 - ContentChild #

Total comments: 2

Patch Set 7 : rebase #

Patch Set 8 : fix build errors #

Patch Set 9 : fix build error #

Patch Set 10 : fix build error #

Unified diffs Side-by-side diffs Delta from patch set Stats (+30 lines, -42 lines) Patch
M base/task_scheduler/task_scheduler.h View 1 2 3 4 5 6 7 2 chunks +8 lines, -4 lines 0 comments Download
M base/task_scheduler/task_scheduler.cc View 1 2 3 4 5 6 7 2 chunks +10 lines, -3 lines 0 comments Download
M chrome/app/mash/mash_runner.cc View 1 2 3 4 2 chunks +1 line, -3 lines 0 comments Download
M chrome/service/service_process.cc View 1 2 3 1 chunk +1 line, -2 lines 0 comments Download
M components/cronet/ios/cronet_environment.mm View 1 2 3 2 chunks +1 line, -3 lines 0 comments Download
M content/child/child_process.cc View 1 2 3 4 5 6 7 1 chunk +1 line, -2 lines 0 comments Download
M remoting/client/chromoting_client_runtime.cc View 1 2 3 4 5 6 1 chunk +1 line, -6 lines 0 comments Download
M remoting/client/ios/app_runtime.cc View 1 2 3 1 chunk +1 line, -3 lines 0 comments Download
M remoting/host/it2me/it2me_native_messaging_host_main.cc View 1 chunk +1 line, -3 lines 0 comments Download
M remoting/host/remoting_me2me_host.cc View 1 chunk +1 line, -3 lines 0 comments Download
M remoting/host/setup/me2me_native_messaging_host_main.cc View 1 chunk +1 line, -3 lines 0 comments Download
M remoting/host/setup/start_host_main.cc View 1 2 3 4 5 6 7 8 1 chunk +1 line, -2 lines 0 comments Download
M remoting/test/chromoting_test_driver.cc View 1 2 3 4 5 6 7 8 9 1 chunk +1 line, -2 lines 0 comments Download
M ui/ozone/demo/ozone_demo.cc View 1 2 3 4 1 chunk +1 line, -3 lines 0 comments Download
Commit queue not available (can’t edit this change).

Messages

Total messages: 60 (31 generated)
fdoray
PTAL
1 month, 1 week ago (2017-03-13 22:09:15 UTC) #4
robliao
Looks generally good. Some comments and questions. https://codereview.chromium.org/2729523006/diff/40001/base/task_scheduler/task_scheduler.cc File base/task_scheduler/task_scheduler.cc (right): https://codereview.chromium.org/2729523006/diff/40001/base/task_scheduler/task_scheduler.cc#newcode28 base/task_scheduler/task_scheduler.cc:28: constexpr int ...
1 month, 1 week ago (2017-03-13 23:56:05 UTC) #8
fdoray
PTAnL https://codereview.chromium.org/2729523006/diff/40001/base/task_scheduler/task_scheduler.cc File base/task_scheduler/task_scheduler.cc (right): https://codereview.chromium.org/2729523006/diff/40001/base/task_scheduler/task_scheduler.cc#newcode28 base/task_scheduler/task_scheduler.cc:28: constexpr int kMinNumThreads = 2; On 2017/03/13 23:56:05, ...
1 month, 1 week ago (2017-03-14 15:08:22 UTC) #10
robliao
lgtm + nit. Thanks! https://codereview.chromium.org/2729523006/diff/80001/content/child/child_process.cc File content/child/child_process.cc (right): https://codereview.chromium.org/2729523006/diff/80001/content/child/child_process.cc#newcode59 content/child/child_process.cc:59: base::TaskScheduler::CreateAndSetSimpleTaskScheduler("Child"); Nit: ContentChildProcess might be ...
1 month, 1 week ago (2017-03-14 20:06:09 UTC) #11
fdoray
https://codereview.chromium.org/2729523006/diff/80001/content/child/child_process.cc File content/child/child_process.cc (right): https://codereview.chromium.org/2729523006/diff/80001/content/child/child_process.cc#newcode59 content/child/child_process.cc:59: base::TaskScheduler::CreateAndSetSimpleTaskScheduler("Child"); On 2017/03/14 20:06:08, robliao wrote: > Nit: ContentChildProcess ...
1 month, 1 week ago (2017-03-15 14:24:19 UTC) #12
fdoray
pauljensen@chromium.org: Please review changes in components/cronet/ sky@chromium.org: Please review changes in chrome/ sergeyu@chromium.org: Please review ...
1 month, 1 week ago (2017-03-15 15:16:26 UTC) #14
pauljensen
Andrei (who is familiar with Cronet iOS and an OWNER) and Lily (who wrote the ...
1 month, 1 week ago (2017-03-15 15:40:13 UTC) #16
jam
lgtm
1 month, 1 week ago (2017-03-15 17:07:51 UTC) #17
kapishnikov
LGTM for Cronet
1 month, 1 week ago (2017-03-15 18:28:02 UTC) #18
gab
lgtm
1 month, 1 week ago (2017-03-15 19:34:13 UTC) #19
sky
mash_runner LGTM - fdoray, as you changed chrome/service/service_process.cc I'm assuming you know the change to ...
1 month, 1 week ago (2017-03-15 19:53:05 UTC) #20
nicholss
There will be a merge conflict, heads up. https://codereview.chromium.org/2729523006/diff/100001/remoting/client/jni/chromoting_jni_runtime.cc File remoting/client/jni/chromoting_jni_runtime.cc (right): https://codereview.chromium.org/2729523006/diff/100001/remoting/client/jni/chromoting_jni_runtime.cc#newcode46 remoting/client/jni/chromoting_jni_runtime.cc:46: base::TaskScheduler::CreateAndSetSimpleTaskScheduler("RemotingJava"); ...
1 month, 1 week ago (2017-03-17 17:20:55 UTC) #22
fdoray
ping sergeyu@ https://codereview.chromium.org/2729523006/diff/100001/remoting/client/jni/chromoting_jni_runtime.cc File remoting/client/jni/chromoting_jni_runtime.cc (right): https://codereview.chromium.org/2729523006/diff/100001/remoting/client/jni/chromoting_jni_runtime.cc#newcode46 remoting/client/jni/chromoting_jni_runtime.cc:46: base::TaskScheduler::CreateAndSetSimpleTaskScheduler("RemotingJava"); On 2017/03/17 17:20:54, nicholss wrote: > ...
1 month ago (2017-03-20 16:58:41 UTC) #24
Sergey Ulanov
As far as I understand there can't be more than one TaskScheduler per process and ...
1 month ago (2017-03-20 17:45:20 UTC) #25
fdoray
On 2017/03/20 17:45:20, Sergey Ulanov wrote: > As far as I understand there can't be ...
1 month ago (2017-03-20 18:03:33 UTC) #28
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.org/2729523006/120001
1 month ago (2017-03-20 18:04:31 UTC) #29
commit-bot: I haz the power
Try jobs failed on following builders: cast_shell_android on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/cast_shell_android/builds/230836) mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED, ...
1 month ago (2017-03-20 18:20:41 UTC) #31
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.org/2729523006/140001
1 month ago (2017-03-20 19:08:32 UTC) #35
commit-bot: I haz the power
Try jobs failed on following builders: android_clang_dbg_recipe on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_clang_dbg_recipe/builds/231657) android_cronet_tester on master.tryserver.chromium.android (JOB_FAILED, ...
1 month ago (2017-03-20 19:20:29 UTC) #37
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.org/2729523006/140001
1 month ago (2017-03-20 19:39:11 UTC) #39
commit-bot: I haz the power
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_chromeos_rel_ng/builds/387089)
1 month ago (2017-03-20 20:04:59 UTC) #41
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.org/2729523006/140001
1 month ago (2017-03-20 20:12:03 UTC) #43
commit-bot: I haz the power
Try jobs failed on following builders: linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_rel_ng/builds/412413)
1 month ago (2017-03-20 20:28:00 UTC) #45
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.org/2729523006/160001
1 month ago (2017-03-20 20:40:51 UTC) #48
commit-bot: I haz the power
Try jobs failed on following builders: android_arm64_dbg_recipe on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_arm64_dbg_recipe/builds/231073) linux_chromium_compile_dbg_ng on master.tryserver.chromium.linux (JOB_FAILED, ...
1 month ago (2017-03-20 21:12:30 UTC) #50
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.org/2729523006/160001
1 month ago (2017-03-21 14:28:06 UTC) #52
commit-bot: I haz the power
Try jobs failed on following builders: linux_chromium_compile_dbg_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_compile_dbg_ng/builds/260201)
1 month ago (2017-03-21 14:49:06 UTC) #54
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.org/2729523006/180001
1 month ago (2017-03-21 16:06:57 UTC) #57
commit-bot: I haz the power
1 month ago (2017-03-21 17:20:15 UTC) #60
Message was sent while issue was closed.
Committed patchset #10 (id:180001) as
https://chromium.googlesource.com/chromium/src/+/dafc3415b9ca6afe4c08690891e6...
Sign in to reply to this message.

Powered by Google App Engine
RSS Feeds Recent Issues | This issue
This is Rietveld cc6ac46