Shared Raster Worker Threads

cc/base/switches.cc

 // Copyright (c) 2012 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 #include "cc/base/switches.h"
 
 #include "base/command_line.h"
+#include "base/strings/string_number_conversions.h"
 
 namespace cc {
 namespace switches {
 
 // On platforms where checkerboards are used, prefer background colors instead
 // of checkerboards.
 const char kBackgroundColorInsteadOfCheckerboard[] =
     "background-color-instead-of-checkerboard";
 
 // Disables LCD text.
@@ skipping 184 matching lines @@
   const CommandLine& command_line = *CommandLine::ForCurrentProcess();
 
   if (command_line.HasSwitch(switches::kDisableMapImage))
     return false;
   else if (command_line.HasSwitch(switches::kEnableMapImage))
     return true;
 
   return false;
 }
 
+size_t GetNumRasterThreads() {

[reveman, 2013/12/22 15:49:25]

You need to remove LayerTreeSettings::num_raster_threads in
cc/trees/layer_tree_settings.h as number of raster threads is no longer a per
LTHI setting.

It's critical that all code the depend on this setting use the exact same value.
That's not the case at the moment as the code used here has some subtle
difference compared to the code used in RenderWidgetCompositor. We should also
not be duplicating the code. It should only live in once place as anything else
would be a nightmare to maintain.

I think the best is to remove LayerTreeSettings::num_raster_threads completely
and in resources/picture_pile.cc:PicturePile::Update change:
picture->CloneForDrawing(num_raster_threads_);
to
picture->CloneForDrawing(switches::GetNumRasterThreads());

However, this means that the GetNumRasterThreads() function is now on the
critical path so we'll need to use a static initializer to avoid a performance
regression. Basically just like IsGPURasterizationEnabled() above.

I would move the code here (after fixing my comments below) into a
CheckNumRasterThreads() function and change this function to:

static size_t num_raster_threads = CheckNumRasterThreads();
return num_raster_threads;

FYI, there's a subtle race in this code but the code above has the same problem
so I think it's better to be consistent and fix that in a follow up patch
instead.

+  const int kMinRasterThreads = 1;
+  const int kMaxRasterThreads = 64;
+  const int kDefaultNumRasterThreads = 1;
+
+  const CommandLine& command_line = *CommandLine::ForCurrentProcess();
+  if (command_line.HasSwitch(switches::kNumRasterThreads)) {
+    std::string string_value =
+        command_line.GetSwitchValueASCII(switches::kNumRasterThreads);
+    int num_threads;
+    base::StringToInt(string_value, &num_threads);
+    return std::max(kMinRasterThreads,
+      std::min(num_threads, kMaxRasterThreads));

[reveman, 2013/12/22 15:49:25]

This is different from the parsing of this value in RenderWidgetCompositor.
RenderWidgetCompositor doesn't clamp the value but instead print a warning and
use the default value if it's outside the valid range. It seems like a good idea
to keep doing that instead of clamping the value.

+  } else {

[reveman, 2013/12/22 15:49:25]

nit: style guide prefers no "else" if you return a value above.

+    return kDefaultNumRasterThreads;
+  }
+}
+
 }  // namespace switches
 }  // namespace cc