[tools/perf] Add CodeAndMetadata metrics to v8_browsing benchmarks and add Ignition variants

tools/perf/benchmarks/v8_browsing.py

 # Copyright 2016 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.
 
 import re
 
+from benchmarks import v8_helper
 from core import perf_benchmark
 from telemetry import benchmark
 from telemetry.timeline import chrome_trace_config
 from telemetry.timeline import chrome_trace_category_filter
 from telemetry.web_perf import timeline_based_measurement
 import page_sets
 
 
 # See tr.v.Numeric.getSummarizedScalarNumericsWithNames()
 # https://github.com/catapult-project/catapult/blob/master/tracing/tracing/value/numeric.html#L323
@@ skipping 23 matching lines @@
       # Disable all categories by default.
       '-*',
       # Memory categories.
       'disabled-by-default-memory-infra',
       # V8 categories.
       'blink.console',
       'disabled-by-default-v8.gc',
       'renderer.scheduler',
       'v8',
       'webkit.console',
+      # TODO(crbug.com/616441, primiano): Remove this temporary workaround,
+      # which enables memory-infra V8 code stats in V8 code size benchmarks
+      # only (to not slow down detailed memory dumps in other benchmarks).
+      'disabled-by-default-memory-infra.v8.code_stats',
     ]
     options = timeline_based_measurement.Options(
         chrome_trace_category_filter.ChromeTraceCategoryFilter(
             ','.join(categories)))
     options.config.enable_android_graphics_memtrack = True
     # Trigger periodic light memory dumps every 1000 ms.
     memory_dump_config = chrome_trace_config.MemoryDumpConfig()
     memory_dump_config.AddTrigger('light', 1000)
     options.config.chrome_trace_config.SetMemoryDumpConfig(memory_dump_config)
     options.SetTimelineBasedMetrics(['v8AndMemoryMetrics'])
     return options
 
   def CreateStorySet(self, options):
-    return page_sets.SystemHealthStorySet(platform=self.PLATFORM, case='browse')
+    return page_sets.SystemHealthStorySet(
+        platform=self.PLATFORM, case='browse')

[petrcermak, 2016/08/19 11:06:29]

You don't need this line break ;-)

[rmcilroy, 2016/08/19 15:38:59]

Done.

 
   @classmethod
   def Name(cls):
-    return 'v8.browsing_%s' % cls.PLATFORM
+    return 'v8.browsing_%s%s' % (cls.PLATFORM, cls.TEST_SUFFIX)
 
   @classmethod
   def ValueCanBeAddedPredicate(cls, value, is_first_result):
     # TODO(crbug.com/610962): Remove this stopgap when the perf dashboard
     # is able to cope with the data load generated by TBMv2 metrics.
     if 'memory:chrome' in value.name:
       return ('renderer_processes' in value.name and
               not _IGNORED_MEMORY_STATS_RE.search(value.name))
     return (_V8_GC_HIGH_LEVEL_STATS_RE.search(value.name) and
             not _IGNORED_V8_STATS_RE.search(value.name))
 
   @classmethod
   def ShouldTearDownStateAfterEachStoryRun(cls):
     return True
 
 
 class V8DesktopBrowsingBenchmark(_V8BrowsingBenchmark):

[petrcermak, 2016/08/19 11:06:29]

You could remove some of the code duplication across the benchmarks by defining
two abstract superclasses. I can see two options to do this:

  _V8MobileBrowsingBenchmark
  _V8DesktopBrowsingBenchmark

  -or-

  _V8NonIgnitionBrowsingBenchmark
  _V8IgnitionBrowsingBenchmark

Perhaps the former would make more sense because there's more platform-related
code (4 → 2 ShouldDisable methods).

[rmcilroy, 2016/08/19 15:38:59]

Done.

   PLATFORM = 'desktop'
+  TEST_SUFFIX = ''
 
   @classmethod
   def ShouldDisable(cls, possible_browser):
     # http://crbug.com/628736
     if (possible_browser.platform.GetOSName() == 'mac' and
         possible_browser.browser_type == 'reference'):
       return True
 
     return possible_browser.platform.GetDeviceTypeName() != 'Desktop'
 

[petrcermak, 2016/08/19 11:06:30]

I ask you to add extra blank lines between top-level definitions below. Could
you please add it here as well (even though it's not technically part of your
patch)?

[rmcilroy, 2016/08/19 15:38:59]

Done.

 @benchmark.Disabled('reference')  # http://crbug.com/628631
 class V8MobileBrowsingBenchmark(_V8BrowsingBenchmark):
   PLATFORM = 'mobile'
+  TEST_SUFFIX = ''
 
   @classmethod
   def ShouldDisable(cls, possible_browser):
+    return possible_browser.platform.GetDeviceTypeName() == 'Desktop'
+

[petrcermak, 2016/08/19 11:06:29]

nit: add blank line (there should be 2 blank lines between top-level definition)

[rmcilroy, 2016/08/19 15:38:59]

Done.

+class V8DesktopIgnitionBrowsingBenchmark(_V8BrowsingBenchmark):
+  PLATFORM = 'desktop'
+  TEST_SUFFIX = '_ignition'
+
+  def SetExtraBrowserOptions(self, options):
+    super(V8DesktopIgnitionBrowsingBenchmark, self).SetExtraBrowserOptions(
+        options)
+    v8_helper.EnableIgnition(options)
+
+  @classmethod
+  def ShouldDisable(cls, possible_browser):
+    # http://crbug.com/628736
+    if (possible_browser.platform.GetOSName() == 'mac' and
+        possible_browser.browser_type == 'reference'):
+      return True
+
+    return possible_browser.platform.GetDeviceTypeName() != 'Desktop'
+

[petrcermak, 2016/08/19 11:06:29]

nit: add blank line (ditto)

[rmcilroy, 2016/08/19 15:38:59]

Done.

+@benchmark.Disabled('reference')  # http://crbug.com/628631
+class V8MobileIgnitionBrowsingBenchmark(_V8BrowsingBenchmark):
+  PLATFORM = 'mobile'
+  TEST_SUFFIX = '_ignition'
+
+  def SetExtraBrowserOptions(self, options):
+    super(V8MobileIgnitionBrowsingBenchmark, self).SetExtraBrowserOptions(
+        options)
+    v8_helper.EnableIgnition(options)
+
+  @classmethod
+  def ShouldDisable(cls, possible_browser):
     return possible_browser.platform.GetDeviceTypeName() == 'Desktop'