Chromium Code Reviews
Help | Chromium Project | Gerrit Changes | Sign in
(13)

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

Can't Edit
Can't Publish+Mail
Start Review
Created:
3 months, 4 weeks ago by fdoray
Modified:
3 months, 1 week 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
3 months, 2 weeks 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 ...
3 months, 2 weeks 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, ...
3 months, 2 weeks 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 ...
3 months, 2 weeks 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 ...
3 months, 2 weeks 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 ...
3 months, 2 weeks 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 ...
3 months, 2 weeks ago (2017-03-15 15:40:13 UTC) #16
jam
lgtm
3 months, 2 weeks ago (2017-03-15 17:07:51 UTC) #17
kapishnikov
LGTM for Cronet
3 months, 2 weeks ago (2017-03-15 18:28:02 UTC) #18
gab (slow Tue-Wed)
lgtm
3 months, 2 weeks ago (2017-03-15 19:34:13 UTC) #19
sky OOO
mash_runner LGTM - fdoray, as you changed chrome/service/service_process.cc I'm assuming you know the change to ...
3 months, 2 weeks 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"); ...
3 months, 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: > ...
3 months, 1 week 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 ...
3 months, 1 week 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 ...
3 months, 1 week 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
3 months, 1 week 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, ...
3 months, 1 week 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
3 months, 1 week 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, ...
3 months, 1 week 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
3 months, 1 week 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)
3 months, 1 week 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
3 months, 1 week 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)
3 months, 1 week 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
3 months, 1 week 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, ...
3 months, 1 week 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
3 months, 1 week 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)
3 months, 1 week 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
3 months, 1 week ago (2017-03-21 16:06:57 UTC) #57
commit-bot: I haz the power
3 months, 1 week 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 23e94e589