cc: Add experimental --num-raster-threads option.

cc: Add experimental --num-raster-threads option.

Makes it possible to specify the number of worker threads used to
rasterize content.
    
BUG=154637
TEST=manual
TBR=sky@chromium.org

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

[nduca, 2012-11-30 07:17:28]

What about a command line flag, for now? Eg. --num-raster-threads=4?

I think that we'll probably need to experiment with it for a bit before we're
sure we want to use num cpus.

My guess, yet tbd, is that its max(1, numSysThreads - k)... for k=2 or 3.
Measurements of m18 on Nexus 7 suggest k=3.

[reveman, 2012-11-30 07:44:33]

On 2012/11/30 07:17:28, nduca wrote:
> What about a command line flag, for now? Eg. --num-raster-threads=4?
> 
> I think that we'll probably need to experiment with it for a bit before we're
> sure we want to use num cpus.
> 
> My guess, yet tbd, is that its max(1, numSysThreads - k)... for k=2 or 3.
> Measurements of m18 on Nexus 7 suggest k=3.

--num-raster-threads=X sounds good, I'll make this CL do that.

[reveman, 2012-11-30 08:56:40]

Added --num-raster-threads. Ptal.

[nduca, 2012-11-30 09:05:32]

Look at my last change to cc_switches, you're gonna need to modify something in
content to pass the flag down to the render process on some platforms... dont
land till you do that, but then lgtm

[commit-bot: I haz the power, 2012-11-30 10:09:25]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/reveman@chromium.org/11421171/3001

[commit-bot: I haz the power, 2012-11-30 10:09:27]

Failed to apply patch for cc/tile_manager.cc:
While running patch -p1 --forward --force --no-backup-if-mismatch;
  patching file cc/tile_manager.cc
  Hunk #1 FAILED at 7.
  Hunk #2 succeeded at 33 with fuzz 1 (offset 2 lines).
  Hunk #3 succeeded at 65 (offset 2 lines).
  1 out of 3 hunks FAILED -- saving rejects to file cc/tile_manager.cc.rej

Patch:       cc/tile_manager.cc
Index: cc/tile_manager.cc
diff --git a/cc/tile_manager.cc b/cc/tile_manager.cc
index
75deebe77f563a0b24f85aada10f30534458311d..c57e889ef5db1cd4d19518292734af7a771f55e2
100644
--- a/cc/tile_manager.cc
+++ b/cc/tile_manager.cc
@@ -7,11 +7,14 @@
 #include <algorithm>
 
 #include "base/bind.h"
+#include "base/command_line.h"
 #include "base/debug/trace_event.h"
 #include "base/logging.h"
+#include "base/string_number_conversions.h"
 #include "base/threading/sequenced_worker_pool.h"
 #include "cc/platform_color.h"
 #include "cc/resource_pool.h"
+#include "cc/switches.h"
 #include "cc/tile.h"
 #include "third_party/skia/include/core/SkDevice.h"
 
@@ -31,7 +34,8 @@ void RasterizeTile(cc::PicturePileImpl* picture_pile,
   picture_pile->Raster(&canvas, rect);
 }
 
-const int kMaxRasterThreads = 1;
+const int kMaxRasterThreads = 64;
+const int kDefaultNumberOfRasterThreads = 1;
 
 const char* kRasterThreadNamePrefix = "CompositorRaster";
 
@@ -62,9 +66,23 @@ TileManager::TileManager(
       resource_pool_(ResourcePool::Create(resource_provider,
                                           Renderer::ImplPool)),
       manage_tiles_pending_(false),
-      pending_raster_tasks_(0),
-      worker_pool_(new base::SequencedWorkerPool(kMaxRasterThreads,
-                                                 kRasterThreadNamePrefix)) {
+      pending_raster_tasks_(0) {
+  size_t worker_threads = kDefaultNumberOfRasterThreads;
+  if (CommandLine::ForCurrentProcess()->HasSwitch(
+          cc::switches::kNumRasterThreads)) {
+    std::string num_raster_threads =
+        CommandLine::ForCurrentProcess()->GetSwitchValueASCII(
+            cc::switches::kNumRasterThreads);
+    int num_threads;
+    if (base::StringToInt(num_raster_threads, &num_threads) &&
+        num_threads > 0 && num_threads <= kMaxRasterThreads) {
+      worker_threads = num_threads;
+    } else {
+      LOG(WARNING) << "Bad number of raster threads: " << num_raster_threads;
+    }
+  }
+  worker_pool_ = new base::SequencedWorkerPool(worker_threads,
+                                               kRasterThreadNamePrefix);
 }
 
 TileManager::~TileManager() {

[commit-bot: I haz the power, 2012-11-30 16:59:25]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/reveman@chromium.org/11421171/8001

[commit-bot: I haz the power, 2012-11-30 16:59:28]

Presubmit check for 11421171-8001 failed and returned exit status 1.


Running presubmit commit checks ...

** Presubmit ERRORS **
Missing LGTM from an OWNER for files in these directories:
    content/browser/renderer_host

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

[reveman, 2012-11-30 17:11:10]

content/browser owner lgtm please.

[nduca, 2012-12-01 04:37:16]

I'd tbr this to sky, David. We just landed an identical one like this yesterday.
Or find another reviewer.

[commit-bot: I haz the power, 2012-12-01 04:52:36]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/reveman@chromium.org/11421171/8001

[commit-bot: I haz the power, 2012-12-01 07:04:45]

Change committed as 170655

[enne (OOO), 2012-12-03 17:47:42]

https://codereview.chromium.org/11421171/diff/8001/cc/tile_manager.cc
File cc/tile_manager.cc (right):

https://codereview.chromium.org/11421171/diff/8001/cc/tile_manager.cc#newcode73
cc/tile_manager.cc:73: if (CommandLine::ForCurrentProcess()->HasSwitch(
Given that ubercompositor may want to set properties differently in the future,
I'd really like if new settings could go through a LayerTreeSettings indirection
so that we weren't checking global command line state.

[reveman, 2012-12-03 20:50:08]

On 2012/12/03 17:47:42, enne wrote:
> https://codereview.chromium.org/11421171/diff/8001/cc/tile_manager.cc
> File cc/tile_manager.cc (right):
> 
>
https://codereview.chromium.org/11421171/diff/8001/cc/tile_manager.cc#newcode73
> cc/tile_manager.cc:73: if (CommandLine::ForCurrentProcess()->HasSwitch(
> Given that ubercompositor may want to set properties differently in the
future,
> I'd really like if new settings could go through a LayerTreeSettings
indirection
> so that we weren't checking global command line state.

ok, makes sense. I'll fix this asap.