Make the number of raster threads appear in chrome://gpu. (issue 519923002 by danakj@chromium.org)

[dan...@chromium.org]

Reviewers: piman (OOO),

Description:
Make the number of raster threads appear in chrome://gpu.

This add methods to compositor_util.cc to compute the number of
raster threads to be used in the renderer process. Then this number
is passed to the renderer process explicitly instead of just forwarding
a command line flag blindly.

If the renderer will use more than one thread, chrome://gpu will report
that "Multiple Raster Threads" is enabled, otherwise, it shows disabled.
If the --num-raster-threads command line argument is used to force more
than one thread, then it will show Force enabled.

There is no change in behaviour with this patch, it still uses one thread
unless forced otherwise on the command line.

BUG=237669

Please review this at https://codereview.chromium.org/519923002/

SVN Base: https://chromium.googlesource.com/chromium/src.git@master

Affected files (+74, -19 lines):
M content/browser/gpu/compositor_util.h
M content/browser/gpu/compositor_util.cc
M content/browser/renderer_host/render_process_host_impl.cc
M content/browser/resources/gpu/info_view.js
M content/renderer/render_thread_impl.cc


Index: content/browser/gpu/compositor_util.cc
diff --git a/content/browser/gpu/compositor_util.cc
b/content/browser/gpu/compositor_util.cc
index
15eb6fee2531e18885597e68d83c6c0bc54d610e..ab17459298b73b2662706192c8309ded7be30307
100644
--- a/content/browser/gpu/compositor_util.cc
+++ b/content/browser/gpu/compositor_util.cc
@@ -7,6 +7,7 @@
#include "base/command_line.h"
#include "base/logging.h"
#include "base/metrics/field_trial.h"
+#include "base/strings/string_number_conversions.h"
#include "build/build_config.h"
#include "cc/base/switches.h"
#include "content/browser/gpu/gpu_data_manager_impl.h"
@@ -27,6 +28,10 @@ const char* kGpuCompositingFeatureName
= "gpu_compositing";
const char* kWebGLFeatureName = "webgl";
const char* kRasterizationFeatureName = "rasterization";
const char* kThreadedRasterizationFeatureName = "threaded_rasterization";
+const char* kMultipleRasterThreadsFeatureName = "multiple_raster_threads";
+
+const int kMinRasterThreads = 1;
+const int kMaxRasterThreads = 64;

struct GpuFeatureInfo {
std::string name;
@@ -142,8 +147,14 @@ const GpuFeatureInfo GetGpuFeatureInfo(size_t index,
bool* eof) {
"Threaded rasterization has not been enabled or"
" is not supported by the current system.",
false
- }
-
+ },
+ {
+ kMultipleRasterThreadsFeatureName,
+ false,
+ NumberOfRendererRasterThreads() == 1,
+ "Raster is using a single thread.",
+ false
+ },
};
DCHECK(index < arraysize(kGpuFeatureInfo));
*eof = (index == arraysize(kGpuFeatureInfo) - 1);
@@ -200,6 +211,36 @@ bool IsImplSidePaintingEnabled() {
return true;
}

+int NumberOfRendererRasterThreads() {
+ int num_raster_threads = 1;
+
+ int force_num_raster_threads = ForceNumberOfRendererRasterThreads();
+ if (force_num_raster_threads)
+ num_raster_threads = force_num_raster_threads;
+
+ return num_raster_threads;
+}
+
+int ForceNumberOfRendererRasterThreads() {
+ const base::CommandLine& command_line =
+ *base::CommandLine::ForCurrentProcess();
+
+ if (!command_line.HasSwitch(switches::kNumRasterThreads))
+ return 0;
+ std::string string_value =
+ command_line.GetSwitchValueASCII(switches::kNumRasterThreads);
+ int force_num_raster_threads;
+ if (base::StringToInt(string_value, &force_num_raster_threads) &&
+ force_num_raster_threads >= kMinRasterThreads &&
+ force_num_raster_threads <= kMaxRasterThreads) {
+ return force_num_raster_threads;
+ } else {
+ LOG(WARNING) << "Failed to parse switch " <<
+ switches::kNumRasterThreads << ": " << string_value;
+ return 0;
+ }
+}
+
bool IsGpuRasterizationEnabled() {
const base::CommandLine& command_line =
*base::CommandLine::ForCurrentProcess();
@@ -252,9 +293,9 @@ base::Value* GetFeatureStatus() {
} else if (gpu_feature_info.blocked ||
gpu_access_blocked) {
status = "unavailable";
- if (gpu_feature_info.fallback_to_software) {
+ if (gpu_feature_info.fallback_to_software)
status += "_software";
- } else
+ else
status += "_off";
} else {
status = "enabled";
@@ -265,7 +306,12 @@ base::Value* GetFeatureStatus() {
if (IsForceGpuRasterizationEnabled())
status += "_force";
}
- if (gpu_feature_info.name == kThreadedRasterizationFeatureName)
+ if (gpu_feature_info.name == kMultipleRasterThreadsFeatureName) {
+ if (ForceNumberOfRendererRasterThreads() > 0)
+ status += "_force";
+ }
+ if (gpu_feature_info.name == kThreadedRasterizationFeatureName ||
+ gpu_feature_info.name == kMultipleRasterThreadsFeatureName)
status += "_on";
}
if (gpu_feature_info.name == kWebGLFeatureName &&
Index: content/browser/gpu/compositor_util.h
diff --git a/content/browser/gpu/compositor_util.h
b/content/browser/gpu/compositor_util.h
index
3ce396e384b14bd115d2eedc22c95afe990147b5..149863c211426addc32877016b750e266f7a75a8
100644
--- a/content/browser/gpu/compositor_util.h
+++ b/content/browser/gpu/compositor_util.h
@@ -30,6 +30,13 @@ CONTENT_EXPORT bool IsGpuRasterizationEnabled();
// Returns true if force-gpu-rasterization is on (via flags) for the
renderer.
CONTENT_EXPORT bool IsForceGpuRasterizationEnabled();

+// Returns the number of raster threads to use for compositing.
+CONTENT_EXPORT int NumberOfRendererRasterThreads();
+
+// Returns the number of raster threads to use for compositing that are
forced
+// by the command line.
+CONTENT_EXPORT int ForceNumberOfRendererRasterThreads();
+
CONTENT_EXPORT base::Value* GetFeatureStatus();
CONTENT_EXPORT base::Value* GetProblems();
CONTENT_EXPORT base::Value* GetDriverBugWorkarounds();
Index: content/browser/renderer_host/render_process_host_impl.cc
diff --git a/content/browser/renderer_host/render_process_host_impl.cc
b/content/browser/renderer_host/render_process_host_impl.cc
index
0b95dda213f4478e756b8df802d049288b5f7398..8b80576124d26f3ebc00a0c562551506a0610271
100644
--- a/content/browser/renderer_host/render_process_host_impl.cc
+++ b/content/browser/renderer_host/render_process_host_impl.cc
@@ -1020,8 +1020,12 @@ static void
AppendCompositorCommandLineFlags(base::CommandLine* command_line) {
if (IsDelegatedRendererEnabled())
command_line->AppendSwitch(switches::kEnableDelegatedRenderer);

- if (IsImplSidePaintingEnabled())
+ if (IsImplSidePaintingEnabled()) {
command_line->AppendSwitch(switches::kEnableImplSidePainting);
+ command_line->AppendSwitchASCII(
+ switches::kNumRasterThreads,
+ base::IntToString(NumberOfRendererRasterThreads()));
+ }

if (content::IsGpuRasterizationEnabled())
command_line->AppendSwitch(switches::kEnableGpuRasterization);
@@ -1179,7 +1183,6 @@ void
RenderProcessHostImpl::PropagateBrowserCommandLineToRenderer(
switches::kMemoryMetrics,
switches::kNoReferrers,
switches::kNoSandbox,
- switches::kNumRasterThreads,
switches::kPpapiInProcess,
switches::kProfilerTiming,
switches::kReduceSecurityForTesting,
Index: content/browser/resources/gpu/info_view.js
diff --git a/content/browser/resources/gpu/info_view.js
b/content/browser/resources/gpu/info_view.js
index
39042cadbeffb7c44e8501278a3a31664723e07b..e4d1795cf99174c27ae01fd3b74e4ae20ae2d628
100644
--- a/content/browser/resources/gpu/info_view.js
+++ b/content/browser/resources/gpu/info_view.js
@@ -94,6 +94,7 @@ cr.define('gpu', function() {
'panel_fitting': 'Panel Fitting',
'rasterization': 'Rasterization',
'threaded_rasterization': 'Threaded Rasterization',
+ 'multiple_raster_threads': 'Multiple Raster Threads',
};

var statusMap = {
@@ -136,7 +137,11 @@ cr.define('gpu', function() {
'enabled_on': {
'label': 'Enabled',
'class': 'feature-green'
- }
+ },
+ 'enabled_force_on': {
+ 'label': 'Force enabled',
+ 'class': 'feature-green'
+ },
};

// GPU info, basic
Index: content/renderer/render_thread_impl.cc
diff --git a/content/renderer/render_thread_impl.cc
b/content/renderer/render_thread_impl.cc
index
d6aa2743bc5c0e553da66e7b70ee3934740beb6f..e21440c55baa8ceff5d1d84530f6c4e142f536be
100644
--- a/content/renderer/render_thread_impl.cc
+++ b/content/renderer/render_thread_impl.cc
@@ -173,8 +173,6 @@ const int64 kInitialIdleHandlerDelayMs = 1000;
const int64 kShortIdleHandlerDelayMs = 1000;
const int64 kLongIdleHandlerDelayMs = 30*1000;
const int kIdleCPUUsageThresholdInPercents = 3;
-const int kMinRasterThreads = 1;
-const int kMaxRasterThreads = 64;

// Maximum allocation size allowed for image scaling filters that
// require pre-scaling. Skia will fallback to a filter that doesn't
@@ -556,18 +554,14 @@ void RenderThreadImpl::Init() {
// it doesn't have to be the same thread RenderThreadImpl is created on.
allocate_gpu_memory_buffer_thread_checker_.DetachFromThread();

- if (command_line.HasSwitch(switches::kNumRasterThreads)) {
+ if (is_impl_side_painting_enabled_) {
int num_raster_threads;
std::string string_value =
command_line.GetSwitchValueASCII(switches::kNumRasterThreads);
- if (base::StringToInt(string_value, &num_raster_threads) &&
- num_raster_threads >= kMinRasterThreads &&
- num_raster_threads <= kMaxRasterThreads) {
- cc::RasterWorkerPool::SetNumRasterThreads(num_raster_threads);
- } else {
- LOG(WARNING) << "Failed to parse switch " <<
- switches::kNumRasterThreads << ": " << string_value;
- }
+ bool ok = base::StringToInt(string_value, &num_raster_threads);
+ DCHECK(ok) << string_value;
+ DCHECK_GT(num_raster_threads, 0);
+ cc::RasterWorkerPool::SetNumRasterThreads(num_raster_threads);
}

service_registry()->AddService<RenderFrameSetup>(

[dan...@chromium.org]

https://codereview.chromium.org/519923002/

[dan...@chromium.org]

+kbr for content/browser stuff
+jamesr for content/renderer stuff

https://codereview.chromium.org/519923002/

[jam...@chromium.org]

lgtm




https://codereview.chromium.org/519923002/diff/1/content/renderer/render_thread_impl.cc
File content/renderer/render_thread_impl.cc (right):

https://codereview.chromium.org/519923002/diff/1/content/renderer/render_thread_impl.cc#newcode558
content/renderer/render_thread_impl.cc:558: int num_raster_threads;
initialize this var, please. if the StringToInt call fails and we aren't
in a debug build this will call SetNumRasterThreads() with whatever's
sitting on the stack here which'll probably lead to a difficult to
understand crash later

https://codereview.chromium.org/519923002/diff/1/content/renderer/render_thread_impl.cc#newcode562
content/renderer/render_thread_impl.cc:562: DCHECK(ok) << string_value;
if you give this var a better name the error message will be slightly
easier to parse when this trips. DCHECK(num_raster_threads_parsed) or
something?

https://codereview.chromium.org/519923002/

[dan...@chromium.org]

https://codereview.chromium.org/519923002/diff/1/content/renderer/render_thread_impl.cc
File content/renderer/render_thread_impl.cc (right):

https://codereview.chromium.org/519923002/diff/1/content/renderer/render_thread_impl.cc#newcode558
content/renderer/render_thread_impl.cc:558: int num_raster_threads;

On 2014/08/29 19:50:39, jamesr wrote:
> initialize this var, please. if the StringToInt call fails and we
aren't in a
> debug build this will call SetNumRasterThreads() with whatever's
sitting on the
> stack here which'll probably lead to a difficult to understand crash
later

Done.

https://codereview.chromium.org/519923002/diff/1/content/renderer/render_thread_impl.cc#newcode562
content/renderer/render_thread_impl.cc:562: DCHECK(ok) << string_value;

On 2014/08/29 19:50:38, jamesr wrote:
> if you give this var a better name the error message will be slightly
easier to
> parse when this trips. DCHECK(num_raster_threads_parsed) or something?

Done.

https://codereview.chromium.org/519923002/

[k...@chromium.org]

LGTM if this has been tested. One question.



https://codereview.chromium.org/519923002/diff/20001/content/browser/renderer_host/render_process_host_impl.cc
File content/browser/renderer_host/render_process_host_impl.cc (left):

https://codereview.chromium.org/519923002/diff/20001/content/browser/renderer_host/render_process_host_impl.cc#oldcode1182
content/browser/renderer_host/render_process_host_impl.cc:1182:
switches::kNumRasterThreads,
Why is this being removed from this list? Isn't it needed here to get
this argument propagated down to the renderer?

https://codereview.chromium.org/519923002/

[dan...@chromium.org]

https://codereview.chromium.org/519923002/diff/20001/content/browser/renderer_host/render_process_host_impl.cc
File content/browser/renderer_host/render_process_host_impl.cc (left):

https://codereview.chromium.org/519923002/diff/20001/content/browser/renderer_host/render_process_host_impl.cc#oldcode1182
content/browser/renderer_host/render_process_host_impl.cc:1182:
switches::kNumRasterThreads,

On 2014/08/29 22:41:05, Ken Russell wrote:
> Why is this being removed from this list? Isn't it needed here to get
this
> argument propagated down to the renderer?

because the other code adds it to the renderer command line explicitly,
we don't just forward it along blindly. this list is flags to just copy
from the browser command line.

https://codereview.chromium.org/519923002/

[commi...@chromium.org]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/dan...@chromium.org/519923002/20001


https://codereview.chromium.org/519923002/