Adding a command line flag to specify renderer process limit

Adding a command line flag to specify renderer process limit

BUG=111498
TEST=Run chrome with --renderer-process-limit=X where X is the maximum number of renderer processes.


Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=121744

[Charlie Reis, 2012-02-03 23:05:34]

Great-- I'm eager to try this out.  A few comments inline.

https://chromiumcodereview.appspot.com/9328011/diff/1/content/browser/browser...
File content/browser/browser_main_loop.cc (right):

https://chromiumcodereview.appspot.com/9328011/diff/1/content/browser/browser...
content/browser/browser_main_loop.cc:277: if (base::StringToInt(limit_string,
&process_limit)) {
We should use StringToSizeT, right?  No negatives allowed, and that's what
SetMax is expecting.

https://chromiumcodereview.appspot.com/9328011/diff/1/content/browser/rendere...
File content/browser/renderer_host/render_process_host_impl.cc (right):

https://chromiumcodereview.appspot.com/9328011/diff/1/content/browser/rendere...
content/browser/renderer_host/render_process_host_impl.cc:210: size_t
max_renderer_count_override;
This should probably become a static on RenderProcessHost, now that it's not in
a local namespace.

https://chromiumcodereview.appspot.com/9328011/diff/1/content/public/browser/...
File content/public/browser/render_process_host.h (right):

https://chromiumcodereview.appspot.com/9328011/diff/1/content/public/browser/...
content/public/browser/render_process_host.h:230: // renderer processes.
We should either guarantee that it's only called at startup, or add a comment
here saying that.  We don't expect it to change at runtime.

https://chromiumcodereview.appspot.com/9328011/diff/1/content/public/browser/...
content/public/browser/render_process_host.h:235: static size_t
GetMaxRendererProcessCount();
This isn't strictly needed here, but I know we'll need it in your next CL, so I
think it's fine to add it now.

https://chromiumcodereview.appspot.com/9328011/diff/1/content/public/common/c...
File content/public/common/content_switches.cc (right):

https://chromiumcodereview.appspot.com/9328011/diff/1/content/public/common/c...
content/public/common/content_switches.cc:464: // Sets the limit of number of
renderer processes to use.
We should add a warning here that high values may lead to instability or high
memory use (at least until we verify otherwise).

[nasko, 2012-02-07 00:42:05]

I've updated the patch, addressing the comments you have. Unit test is still to
be added.

https://chromiumcodereview.appspot.com/9328011/diff/1/content/browser/browser...
File content/browser/browser_main_loop.cc (right):

https://chromiumcodereview.appspot.com/9328011/diff/1/content/browser/browser...
content/browser/browser_main_loop.cc:277: if (base::StringToInt(limit_string,
&process_limit)) {
On 2012/02/03 23:05:34, creis wrote:
> We should use StringToSizeT, right?  No negatives allowed, and that's what
> SetMax is expecting.

Done.

https://chromiumcodereview.appspot.com/9328011/diff/1/content/browser/rendere...
File content/browser/renderer_host/render_process_host_impl.cc (right):

https://chromiumcodereview.appspot.com/9328011/diff/1/content/browser/rendere...
content/browser/renderer_host/render_process_host_impl.cc:210: size_t
max_renderer_count_override;
On 2012/02/03 23:05:34, creis wrote:
> This should probably become a static on RenderProcessHost, now that it's not
in
> a local namespace.

Done.

https://chromiumcodereview.appspot.com/9328011/diff/1/content/public/browser/...
File content/public/browser/render_process_host.h (right):

https://chromiumcodereview.appspot.com/9328011/diff/1/content/public/browser/...
content/public/browser/render_process_host.h:230: // renderer processes.
On 2012/02/03 23:05:34, creis wrote:
> We should either guarantee that it's only called at startup, or add a comment
> here saying that.  We don't expect it to change at runtime.

Done.

https://chromiumcodereview.appspot.com/9328011/diff/1/content/public/browser/...
content/public/browser/render_process_host.h:235: static size_t
GetMaxRendererProcessCount();
On 2012/02/03 23:05:34, creis wrote:
> This isn't strictly needed here, but I know we'll need it in your next CL, so
I
> think it's fine to add it now.

Done.

https://chromiumcodereview.appspot.com/9328011/diff/1/content/public/common/c...
File content/public/common/content_switches.cc (right):

https://chromiumcodereview.appspot.com/9328011/diff/1/content/public/common/c...
content/public/common/content_switches.cc:464: // Sets the limit of number of
renderer processes to use.
On 2012/02/03 23:05:34, creis wrote:
> We should add a warning here that high values may lead to instability or high
> memory use (at least until we verify otherwise).

Done.

[Charlie Reis, 2012-02-07 17:36:36]

Looks good, once we have the unit test.  Don't forget to CC the watchlist folks
and add an owner to review when you're ready.

https://chromiumcodereview.appspot.com/9328011/diff/4001/content/public/brows...
File content/public/browser/render_process_host.h (right):

https://chromiumcodereview.appspot.com/9328011/diff/4001/content/public/brows...
content/public/browser/render_process_host.h:229: // also used to allow command
line parameter to configure the max number of
Nit: "...allow a command..."

https://chromiumcodereview.appspot.com/9328011/diff/4001/content/public/commo...
File content/public/common/content_switches.cc (right):

https://chromiumcodereview.appspot.com/9328011/diff/4001/content/public/commo...
content/public/common/content_switches.cc:464: // Overwrites the
default/calculated limit to the number of renderer processes.
Nit: Overrides

[nasko, 2012-02-07 19:01:22]

I've addressed the initial feedback from Charlie and added an unit test. Ready
for review.

https://chromiumcodereview.appspot.com/9328011/diff/4001/content/public/brows...
File content/public/browser/render_process_host.h (right):

https://chromiumcodereview.appspot.com/9328011/diff/4001/content/public/brows...
content/public/browser/render_process_host.h:229: // also used to allow command
line parameter to configure the max number of
On 2012/02/07 17:36:36, creis wrote:
> Nit: "...allow a command..."

Done.

https://chromiumcodereview.appspot.com/9328011/diff/4001/content/public/commo...
File content/public/common/content_switches.cc (right):

https://chromiumcodereview.appspot.com/9328011/diff/4001/content/public/commo...
content/public/common/content_switches.cc:464: // Overwrites the
default/calculated limit to the number of renderer processes.
On 2012/02/07 17:36:36, creis wrote:
> Nit: Overrides

Done.

[Charlie Reis, 2012-02-07 19:29:20]

LGTM.  John, can you review for owner's approval?

https://chromiumcodereview.appspot.com/9328011/diff/4002/content/browser/rend...
File content/browser/renderer_host/render_process_host_browsertest.cc (right):

https://chromiumcodereview.appspot.com/9328011/diff/4002/content/browser/rend...
content/browser/renderer_host/render_process_host_browsertest.cc:154: void
RenderProcessHostTest::TestProcessOverflow() {
Let's move this function near the top, above the actual tests.

[jam, 2012-02-07 20:27:29]

http://codereview.chromium.org/9328011/diff/4002/content/public/browser/rende...
File content/public/browser/render_process_host.h (right):

http://codereview.chromium.org/9328011/diff/4002/content/public/browser/rende...
content/public/browser/render_process_host.h:241: static size_t
max_renderer_count_override_;
for the content api we avoid putting statics in the interface. instead keep the
static function as above, but just have it use a static in the
render_process_host_impl.cc file

[nasko, 2012-02-07 21:29:50]

I've made the changes to fix the issues brought up.
Thanks

http://codereview.chromium.org/9328011/diff/4002/content/browser/renderer_hos...
File content/browser/renderer_host/render_process_host_browsertest.cc (right):

http://codereview.chromium.org/9328011/diff/4002/content/browser/renderer_hos...
content/browser/renderer_host/render_process_host_browsertest.cc:154: void
RenderProcessHostTest::TestProcessOverflow() {
On 2012/02/07 19:29:20, creis wrote:
> Let's move this function near the top, above the actual tests.

Done.

http://codereview.chromium.org/9328011/diff/4002/content/public/browser/rende...
File content/public/browser/render_process_host.h (right):

http://codereview.chromium.org/9328011/diff/4002/content/public/browser/rende...
content/public/browser/render_process_host.h:241: static size_t
max_renderer_count_override_;
On 2012/02/07 20:27:29, John Abd-El-Malek wrote:
> for the content api we avoid putting statics in the interface. instead keep
the
> static function as above, but just have it use a static in the
> render_process_host_impl.cc file

Done.

[nasko, 2012-02-09 17:08:18]

John, can you review the changes to address the feedback?

[jam, 2012-02-10 18:02:11]

lgtm with nit

http://codereview.chromium.org/9328011/diff/10004/content/browser/renderer_ho...
File content/browser/renderer_host/render_process_host_impl.cc (right):

http://codereview.chromium.org/9328011/diff/10004/content/browser/renderer_ho...
content/browser/renderer_host/render_process_host_impl.cc:212: static size_t
max_renderer_count_override = 0;
nit: globals should be "g_max_renderer_count_override" to differentiate them
from local variables

[nasko, 2012-02-11 00:49:19]

I added the prefix to the variable. I also had to move the body of the test
method from the header to the .cc file due to a compile error on OSX.
Unfortunately I also had to sync my tree.

http://codereview.chromium.org/9328011/diff/10004/content/browser/renderer_ho...
File content/browser/renderer_host/render_process_host_impl.cc (right):

http://codereview.chromium.org/9328011/diff/10004/content/browser/renderer_ho...
content/browser/renderer_host/render_process_host_impl.cc:212: static size_t
max_renderer_count_override = 0;
On 2012/02/10 18:02:12, John Abd-El-Malek wrote:
> nit: globals should be "g_max_renderer_count_override" to differentiate them
> from local variables

Done.

[commit-bot: I haz the power, 2012-02-13 17:45:19]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/nasko@chromium.org/9328011/22001

[commit-bot: I haz the power, 2012-02-13 17:45:25]

Presubmit check for 9328011-22001 failed and returned exit status 1.

Running presubmit commit checks ...

** Presubmit ERRORS **
Missing LGTM from an OWNER for:
chrome/browser/ui/webui/ntp/new_tab_ui_browsertest.cc

Presubmit checks took 1.6s to calculate.

Was the presubmit check useful? Please send feedback & hate mail to
maruel@chromium.org!

[nasko, 2012-02-13 17:48:30]

Including jhawkins@chromium.org for owners approval for
chrome/browser/ui/webui/ntp/new_tab_ui_browsertest.cc.

[James Hawkins, 2012-02-13 20:47:10]

lgtm

[commit-bot: I haz the power, 2012-02-13 20:52:15]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/nasko@chromium.org/9328011/22001

[commit-bot: I haz the power, 2012-02-13 22:39:36]

Change committed as 121744