Improve GPU software rendering list histograms.

content/browser/gpu/gpu_data_manager.cc

 // Copyright (c) 2011 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 "content/browser/gpu/gpu_data_manager.h"
 
 #if defined(OS_MACOSX)
 #include <CoreGraphics/CGDisplayConfiguration.h>
 #endif
 
 #include "base/command_line.h"
 #include "base/metrics/histogram.h"
 #include "base/stringprintf.h"
 #include "base/string_number_conversions.h"
+#include "base/sys_info.h"
+#include "base/values.h"
+#include "base/version.h"
 #include "content/browser/browser_thread.h"
 #include "content/browser/gpu/gpu_blacklist.h"
 #include "content/browser/gpu/gpu_process_host.h"
 #include "content/common/content_client.h"
 #include "content/common/content_switches.h"
 #include "content/common/gpu/gpu_messages.h"
 #include "content/gpu/gpu_info_collector.h"
 #include "ui/gfx/gl/gl_implementation.h"
 #include "ui/gfx/gl/gl_switches.h"
 
@@ skipping 47 matching lines @@
       it != node.children.end();
       ++it) {
     ListValue* sublist = DxDiagNodeToList(it->second);
     list->Append(NewDescriptionValuePair(it->first, sublist));
   }
   return list;
 }
 
 #endif  // OS_WIN
 
+std::string GetOSString() {
+  std::string rt;
+#if defined(OS_CHROMEOS)
+  rt = "ChromeOS";
+#elif defined(OS_WIN)
+  rt = "Win";
+  std::string version_str = base::SysInfo::OperatingSystemVersion();
+  size_t pos = version_str.find_first_not_of("0123456789.");
+  if (pos != std::string::npos)
+    version_str = version_str.substr(0, pos);
+  scoped_ptr<Version> os_version(Version::GetVersionFromString(version_str));
+  if (os_version.get() && os_version->components().size() >= 2) {
+    const std::vector<uint16>& version_numbers = os_version->components();
+    if (version_numbers[0] == 5)
+      rt += "XP";
+    else if (version_numbers[0] == 6 && version_numbers[1] == 0)
+      rt += "Vista";
+    else if (version_numbers[0] == 6 && version_numbers[1] == 1)
+      rt += "7";
+  }
+#elif defined(OS_LINUX)
+  rt = "Linux";
+#elif defined(OS_MACOSX)
+  rt = "Mac";
+#else
+  rt = "UnknownOS";
+#endif
+  return rt;
+}
+
 }  // namespace anonymous
 
 GpuDataManager::GpuDataManager()
     : complete_gpu_info_already_requested_(false) {
   // Certain tests doesn't go through the browser startup path that
   // initializes GpuDataManager on FILE thread; therefore, it is initialized
   // on UI thread later, and we skip the preliminary gpu info collection
   // in such situation.
   if (!BrowserThread::CurrentlyOn(BrowserThread::FILE))
     return;
@@ skipping 262 matching lines @@
     gpu_feature_flags_.set_flags(0);
     return;
   }
 
   {
     base::AutoLock auto_lock(gpu_info_lock_);
     gpu_feature_flags_ = gpu_blacklist->DetermineGpuFeatureFlags(
         GpuBlacklist::kOsAny, NULL, gpu_info_);
   }
 
-  uint32 max_entry_id = gpu_blacklist->max_entry_id();
-  if (!gpu_feature_flags_.flags()) {
-    UMA_HISTOGRAM_ENUMERATION("GPU.BlacklistTestResultsPerEntry",
-        0, max_entry_id + 1);
-    return;
-  }
-
   // Notify clients that GpuInfo state has changed
   RunGpuInfoUpdateCallbacks();
 
-  // TODO(zmo): move histograming to GpuBlacklist::DetermineGpuFeatureFlags.
-  std::vector<uint32> flag_entries;
-  gpu_blacklist->GetGpuFeatureFlagEntries(
-      GpuFeatureFlags::kGpuFeatureAll, flag_entries);
-  DCHECK_GT(flag_entries.size(), 0u);
-  for (size_t i = 0; i < flag_entries.size(); ++i) {
+  const GpuFeatureFlags::GpuFeatureType kGpuFeatures[] = {
+      GpuFeatureFlags::kGpuFeatureAccelerated2dCanvas,
+      GpuFeatureFlags::kGpuFeatureAcceleratedCompositing,
+      GpuFeatureFlags::kGpuFeatureWebgl
+  };
+  const size_t kNumFeatures =
+      sizeof(kGpuFeatures) / sizeof(GpuFeatureFlags::GpuFeatureType);
+
+  uint32 flags = gpu_feature_flags_.flags();
+  uint32 max_entry_id = gpu_blacklist->max_entry_id();
+  if (flags == 0) {

[vangelis, 2011/08/18 06:06:44]

shouldn't you also include here the case where flags == kDisableMultisampling ? 

[Zhenyao Mo, 2011/08/18 18:16:10]

We have an entry which counts the number of use-cases that multisampling is
disabled.  So we don't need to include the case here.  It should be easy to add
this allowed-case and the multisampling-disabled-case in the histogram.

In the histograms that counts allowed/disallowed gpu features, since we do it
per feature cases, so multisampling won't affect those histograms.

[vangelis, 2011/08/18 21:05:41]

Why do we then just explicitly skip the multisampling flag when you set the
value for features in the loop below?  It looks like you should be using
features = kGpuFeaturesAll to get results for multisampling blacklisting too.

[Zhenyao Mo, 2011/08/18 23:47:18]

Ah you are right.  Fixed.

     UMA_HISTOGRAM_ENUMERATION("GPU.BlacklistTestResultsPerEntry",
-        flag_entries[i], max_entry_id + 1);
+        0, max_entry_id + 1);
+  } else {
+    std::vector<uint32> flag_entries;
+    uint32 features = 0;
+    // We don't use GpuFeatureFlags::kGpuFeatureAll because we want to leave
+    // disabled multisampling out.
+    for (size_t i = 0; i < kNumFeatures; ++i)
+      features |= kGpuFeatures[i];
+    gpu_blacklist->GetGpuFeatureFlagEntries(
+        static_cast<GpuFeatureFlags::GpuFeatureType>(features), flag_entries);
+    DCHECK_GT(flag_entries.size(), 0u);
+    for (size_t i = 0; i < flag_entries.size(); ++i) {
+      UMA_HISTOGRAM_ENUMERATION("GPU.BlacklistTestResultsPerEntry",
+          flag_entries[i], max_entry_id + 1);
+    }
+  }
+
+  const std::string kOSString = GetOSString();
+  const std::string kGpuBlacklistFeatureHistogramNames[] = {
+      "GPU.BlacklistAccelerated2dCanvasTestResults" + kOSString,
+      "GPU.BlacklistAcceleratedCompositingTestResults" + kOSString,
+      "GPU.BlacklistWebglTestResults" + kOSString
+  };
+  for (size_t i = 0; i < kNumFeatures; ++i) {
+    // We can't use UMA_HISTOGRAM_ENUMERATION here because the same name is
+    // expected if the macro is used within a loop.
+    base::Histogram* histogram_pointer = base::LinearHistogram::FactoryGet(
+        kGpuBlacklistFeatureHistogramNames[i], 1, 2, 3,
+        base::Histogram::kUmaTargetedHistogramFlag);
+    histogram_pointer->Add(((flags & kGpuFeatures[i]) == 0) ? 0 : 1);

[vangelis, 2011/08/18 06:06:44]

Could the expression above be simplified as (flags & kGpuFeatures[i])  ? 
Potentially with a static cast to the expected integer value.

[Zhenyao Mo, 2011/08/18 18:16:10]

Done.

[Zhenyao Mo, 2011/08/18 20:12:28]

The Windows try bot fails to compile because of the implicit conversion from
bool to uint32.  So we have to cast it to bool then cast it to int again.

Maybe the original ((flags & kGpuFeatures[i]) == 0) ? 0 : 1 is easier to read
than two nested cast.

   }
 }
 
 GpuBlacklist* GpuDataManager::GetGpuBlacklist() {
   DCHECK(BrowserThread::CurrentlyOn(BrowserThread::UI));
   const CommandLine& browser_command_line = *CommandLine::ForCurrentProcess();
   if (browser_command_line.HasSwitch(switches::kIgnoreGpuBlacklist) ||
       browser_command_line.GetSwitchValueASCII(
           switches::kUseGL) == gfx::kGLImplementationOSMesaName)
     return NULL;
   // No need to return an empty blacklist.
   if (gpu_blacklist_.get() != NULL && gpu_blacklist_->max_entry_id() == 0)
     return NULL;
   return gpu_blacklist_.get();
 }