Render process count should not be forced from java unconditionally

Render process count should not be forced from java unconditionally

content::SetContentCommandLineFlags should enforce a maxiumum value from java.
The problem is that it overrides the value even if the default would be less.
Instead we should let RenderProcessHost decide it based on the available physical
memory and only override if the default value is too high.

Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=249117

[bulach, 2014-01-15 15:56:09]

+aberent is a better reviewer for this..

[kbalazs, 2014-01-23 21:10:54]

ping?

[aberent, 2014-01-24 17:42:51]

On 2014/01/23 21:10:54, kbalazs wrote:
> ping?

Sorry, been travelling for the last week (and still am). Will try to look at it
today, but Tuesday is more likely.

[aberent, 2014-01-24 21:29:08]

https://codereview.chromium.org/138313004/diff/1/content/browser/android/cont...
File content/browser/android/content_startup_flags.cc (right):

https://codereview.chromium.org/138313004/diff/1/content/browser/android/cont...
content/browser/android/content_startup_flags.cc:39: if
(command_line_renderer_limit != -1) {
What happens if command_line_render_limit is set but max_render_process_count <=
0?
It seems that that setting the render limit on the command line shouldn't
override single process mode, so that should execute the next block, but with
this code it won't.
Also what happens if command_line_render_limit is 0 (or < -1)?

https://codereview.chromium.org/138313004/diff/1/content/browser/android/cont...
content/browser/android/content_startup_flags.cc:49: int default_maximum =
RenderProcessHost::GetMaxRendererProcessCount();
If I am reading this right then GetMaxRendererProcessCount() isn't called if the
process count is set on the command line, so the available physical memory etc.
is ignored. This seems to contradict the description of the CL. Is this what you
intend?

https://codereview.chromium.org/138313004/diff/1/content/browser/android/cont...
content/browser/android/content_startup_flags.cc:52:
content::RenderProcessHost::SetMaxRendererProcessCount(
Why content::RenderProcessHost here, but simply RenderProcessHost on lines 42
and 49?

[kbalazs, 2014-01-29 16:55:43]

https://codereview.chromium.org/138313004/diff/1/content/browser/android/cont...
File content/browser/android/content_startup_flags.cc (right):

https://codereview.chromium.org/138313004/diff/1/content/browser/android/cont...
content/browser/android/content_startup_flags.cc:39: if
(command_line_renderer_limit != -1) {
On 2014/01/24 21:29:09, aberent wrote:
> What happens if command_line_render_limit is set but max_render_process_count
<=
> 0?
> It seems that that setting the render limit on the command line shouldn't
> override single process mode, so that should execute the next block, but with
> this code it won't.
> Also what happens if command_line_render_limit is 0 (or < -1)?

I think the command line should override everything if it's >= 1. Good point
about 0 or < -1, I will turn the condition into |command_line_renderer_limit >
0|.

https://codereview.chromium.org/138313004/diff/1/content/browser/android/cont...
content/browser/android/content_startup_flags.cc:49: int default_maximum =
RenderProcessHost::GetMaxRendererProcessCount();
On 2014/01/24 21:29:09, aberent wrote:
> If I am reading this right then GetMaxRendererProcessCount() isn't called if
the
> process count is set on the command line, so the available physical memory
etc.
> is ignored. This seems to contradict the description of the CL. Is this what
you
> intend?

Yes, this was my intent. This is consistent with this:
https://code.google.com/p/chromium/codesearch#chromium/src/content/browser/br....

https://codereview.chromium.org/138313004/diff/1/content/browser/android/cont...
content/browser/android/content_startup_flags.cc:52:
content::RenderProcessHost::SetMaxRendererProcessCount(
On 2014/01/24 21:29:09, aberent wrote:
> Why content::RenderProcessHost here, but simply RenderProcessHost on lines 42
> and 49?

by mistake

[kbalazs, 2014-02-03 17:51:52]

Incorporated comments in patch 2. PTAL :)

[aberent, 2014-02-03 18:17:40]

lgtm

[kbalazs, 2014-02-03 18:31:35]

@bulach: could you lgtm as well? I guess I need an owner. Thanks

[bulach, 2014-02-04 01:42:08]

lgtm, thanks!

[kbalazs, 2014-02-04 01:48:59]

The CQ bit was checked by b.kelemen@samsung.com

[commit-bot: I haz the power, 2014-02-04 01:49:50]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/b.kelemen@samsung.com/138313004/90001

[commit-bot: I haz the power, 2014-02-04 02:20:36]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2014-02-04 02:20:37]

Retried try job too often on win_rel for step(s) app_list_unittests,
ash_unittests, aura_unittests, base_unittests, browser_tests,
cacheinvalidation_unittests, cc_unittests, check_deps, check_deps2git,
chrome_elf_unittests, chromedriver_unittests, components_unittests,
compositor_unittests, content_browsertests, content_unittests, crypto_unittests,
events_unittests, google_apis_unittests, gpu_unittests,
installer_util_unittests, interactive_ui_tests, ipc_tests, jingle_unittests,
media_unittests, mini_installer_test, nacl_integration, net_unittests,
ppapi_unittests, printing_unittests, remoting_unittests, sql_unittests,
sync_integration_tests, sync_unit_tests, telemetry_perf_unittests,
telemetry_unittests, unit_tests, views_unittests
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win_rel&nu...

[kbalazs, 2014-02-04 16:35:05]

The CQ bit was checked by b.kelemen@samsung.com

[commit-bot: I haz the power, 2014-02-04 16:35:20]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/b.kelemen@samsung.com/138313004/370001

[commit-bot: I haz the power, 2014-02-04 17:44:23]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2014-02-04 17:44:24]

Retried try job too often on linux_rel for step(s) sync_integration_tests
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=linux_rel&...

[kbalazs, 2014-02-04 18:15:10]

The CQ bit was checked by b.kelemen@samsung.com

[commit-bot: I haz the power, 2014-02-04 18:20:32]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/b.kelemen@samsung.com/138313004/370001

[commit-bot: I haz the power, 2014-02-04 20:36:54]

Retried try job too often on linux_rel for step(s) base_unittests,
browser_tests, interactive_ui_tests, net_unittests, sync_integration_tests,
unit_tests
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=linux_rel&...

[commit-bot: I haz the power, 2014-02-04 20:38:22]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/b.kelemen@samsung.com/138313004/370001

[commit-bot: I haz the power, 2014-02-05 00:52:24]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/b.kelemen@samsung.com/138313004/370001

[Paweł Hajdan Jr., 2014-02-05 18:51:35]

The CQ bit was unchecked by phajdan.jr@chromium.org

[Paweł Hajdan Jr., 2014-02-05 19:17:14]

The CQ bit was checked by phajdan.jr@chromium.org

[commit-bot: I haz the power, 2014-02-05 19:26:45]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/b.kelemen@samsung.com/138313004/370001

[commit-bot: I haz the power, 2014-02-05 21:39:16]

Change committed as 249117

[kbalazs, 2014-02-05 21:45:15]

On 2014/02/05 21:39:16, I haz the power (commit-bot) wrote:
> Change committed as 249117

Thanks for nudging the commit bot! :)