Make discarding the first result possible through...

Make discarding the first result possible through Benchmark.ValueCanBeAddedPredicate (add a "is_first_result" arg to it).

The main motivation here is to eliminate special cases for PageTest methods/attributes in user_story_runner.Run.

BUG=440101

Review URL: https://codereview.chromium.org/962793004

Cr-Commit-Position: refs/heads/master@{#319140}

tools/perf/benchmarks/gpu_times.py

@@ -22,7 +22,7 @@ class _GPUTimes(benchmark.Benchmark):
         overhead_level=cat_filter)
 
   @classmethod
-  def ValueCanBeAddedPredicate(cls, value):
+  def ValueCanBeAddedPredicate(cls, value, _):
     return (isinstance(value, gpu_timeline.GPUTimelineListOfValues) or
             isinstance(value, gpu_timeline.GPUTimelineValue))
 

tools/perf/benchmarks/page_cycler.py

@@ -9,6 +9,7 @@ from telemetry import benchmark
 
 class _PageCycler(benchmark.Benchmark):
   options = {'pageset_repeat': 6}
+  cold_load_percent = 50  # % of page visits for which a cold load is forced
 
   @classmethod
   def Name(cls):
@@ -20,14 +21,15 @@ class _PageCycler(benchmark.Benchmark):
         action='store_true',
         help='Enable the speed index metric.')
 
-    parser.add_option('--cold-load-percent', type='int', default=50,
-                      help='%d of page visits for which a cold load is forced')
+  @classmethod
+  def ValueCanBeAddedPredicate(cls, _, is_first_result):
+    return cls.cold_load_percent > 0 or not is_first_result
 
   def CreatePageTest(self, options):
     return page_cycler.PageCycler(
         page_repeat = options.page_repeat,
         pageset_repeat = options.pageset_repeat,
-        cold_load_percent = options.cold_load_percent,
+        cold_load_percent = self.cold_load_percent,
         report_speed_index = options.report_speed_index)
 
 
@@ -139,10 +141,10 @@ class PageCyclerNetsimTop10(_PageCycler):
   tag = 'netsim'
   page_set = page_sets.Top10PageSet
   options = {
-      'cold_load_percent': 100,
       'extra_wpr_args_as_string': '--shaping_type=proxy --net=cable',
       'pageset_repeat': 6,
   }
+  cold_load_percent = 100
 
   @classmethod
   def Name(cls):
@@ -152,7 +154,7 @@ class PageCyclerNetsimTop10(_PageCycler):
     return page_cycler.PageCycler(
         page_repeat = options.page_repeat,
         pageset_repeat = options.pageset_repeat,
-        cold_load_percent = options.cold_load_percent,
+        cold_load_percent = self.cold_load_percent,
         report_speed_index = options.report_speed_index,
         clear_cache_before_each_run = True)
 

tools/perf/benchmarks/session_restore.py

@@ -23,6 +23,7 @@ class _SessionRestoreTypical25(benchmark.Benchmark):
   TODO(slamm): Make SmallProfileCreator and this use the same page_set ref.
   """
   page_set = page_sets.Typical25PageSet
+  tag = None  # override with 'warm' or 'cold'
 
   @classmethod
   def Name(cls):
@@ -48,6 +49,10 @@ class _SessionRestoreTypical25(benchmark.Benchmark):
             small_profile_creator.SmallProfileCreator, profile_type, new_args)
       args.browser_options.profile_dir = profile_dir
 
+  @classmethod
+  def ValueCanBeAddedPredicate(cls, _, is_first_result):
+    return cls.tag == 'cold' or not is_first_result
+
   def CreateUserStorySet(self, _):
     """Return a user story set that only has the first user story.
 

tools/perf/benchmarks/startup.py

@@ -27,6 +27,10 @@ class _StartupWarm(benchmark.Benchmark):
   def Name(cls):
     return 'startup'
 
+  @classmethod
+  def ValueCanBeAddedPredicate(cls, _, is_first_result):
+    return not is_first_result
+
   def CreatePageTest(self, options):
     return startup.Startup(cold=False)
 

tools/perf/measurements/page_cycler.py

@@ -61,8 +61,6 @@ class PageCycler(page_test.PageTest):
           (int(pageset_repeat) - 1) * (100 - cold_load_percent) / 100)
       number_warm_runs = number_warm_pageset_runs * page_repeat
       self._cold_run_start_index = number_warm_runs + page_repeat
-      self._discard_first_result = (not cold_load_percent or
-                                    self._discard_first_result)
     else:
       self._cold_run_start_index = pageset_repeat * page_repeat
 

tools/perf/measurements/startup.py

@@ -24,9 +24,6 @@ class Startup(page_test.PageTest):
   def CustomizeBrowserOptions(self, options):
     if self._cold:
       options.clear_sytem_cache_for_browser_and_profile_on_start = True
-    else:
-      self.discard_first_result = True
-
     options.AppendExtraBrowserArgs([
         '--enable-stats-collection-bindings'
     ])

tools/telemetry/telemetry/benchmark.py

@@ -138,17 +138,22 @@ class Benchmark(command_line.Command):
   def ProcessCommandLineArgs(cls, parser, args):
     pass
 
+  # pylint: disable=unused-argument
   @classmethod
-  def ValueCanBeAddedPredicate(cls, value):  # pylint: disable=unused-argument
-    """ Returns whether |value| can be added to the test results.
+  def ValueCanBeAddedPredicate(cls, value, is_first_result):
+    """Returns whether |value| can be added to the test results.
+
     Override this method to customize the logic of adding values to test
     results.
 
     Args:
         value: a value.Value instance.
+        is_first_result: True if |value| is the first result for its
+            corresponding user story.
 
-    Returns: a boolean. True if value should be added to the test results and
-        False otherwise.
+    Returns:
+        True if |value| should be added to the test results.
+        Otherwise, it returns False.
     """
     return True
 

tools/telemetry/telemetry/benchmark_unittest.py

@@ -132,7 +132,7 @@ class BenchmarkTest(unittest.TestCase):
   def testBenchmarkPredicate(self):
     class PredicateBenchmark(TestBenchmark):
       @classmethod
-      def ValueCanBeAddedPredicate(cls, value):
+      def ValueCanBeAddedPredicate(cls, value, is_first_result):
         return False
 
     original_run_fn = user_story_runner.Run

tools/telemetry/telemetry/results/page_test_results.py

@@ -23,7 +23,7 @@ from telemetry.value import trace
 class PageTestResults(object):
   def __init__(self, output_stream=None, output_formatters=None,
                progress_reporter=None, trace_tag='', output_dir=None,
-               value_can_be_added_predicate=lambda v: True):
+               value_can_be_added_predicate=lambda v, is_first: True):
     """
     Args:
       output_stream: The output stream to use to write test results.
@@ -36,9 +36,10 @@ class PageTestResults(object):
           used for buildbot.
       output_dir: A string specified the directory where to store the test
           artifacts, e.g: trace, videos,...
-      value_can_be_added_predicate: A function that takes an value.Value
-          instance as input. It returns True if the value can be added to the
-          test results and False otherwise.
+      value_can_be_added_predicate: A function that takes two arguments:
+          a value.Value instance and a boolean (True when the value is part
+          of the first result for the user story). It returns True if the value
+          can be added to the test results and False otherwise.
     """
     # TODO(chrishenry): Figure out if trace_tag is still necessary.
 
@@ -55,6 +56,7 @@ class PageTestResults(object):
 
     self._current_page_run = None
     self._all_page_runs = []
+    self._all_user_stories = set()
     self._representative_value_for_each_value_name = {}
     self._all_summary_values = []
     self._serialized_trace_file_ids_to_paths = {}
@@ -153,23 +155,23 @@ class PageTestResults(object):
     self._current_page_run = user_story_run.UserStoryRun(page)
     self._progress_reporter.WillRunPage(self)
 
-  def DidRunPage(self, page, discard_run=False):  # pylint: disable=W0613
+  def DidRunPage(self, page):  # pylint: disable=W0613
     """
     Args:
       page: The current page under test.
-      discard_run: Whether to discard the entire run and all of its
-          associated results.
     """
     assert self._current_page_run, 'Did not call WillRunPage.'
     self._progress_reporter.DidRunPage(self)
-    if not discard_run:
     self._all_page_runs.append(self._current_page_run)
+    self._all_user_stories.add(self._current_page_run.user_story)
     self._current_page_run = None
 
   def AddValue(self, value):
     assert self._current_page_run, 'Not currently running test.'
     self._ValidateValue(value)
-    if not self._value_can_be_added_predicate(value):
+    is_first_result = (
+      self._current_page_run.user_story not in self._all_user_stories)
+    if not self._value_can_be_added_predicate(value, is_first_result):
       return
     # TODO(eakuefner/chrishenry): Add only one skip per pagerun assert here
     self._current_page_run.AddValue(value)

tools/telemetry/telemetry/results/page_test_results_unittest.py

@@ -3,6 +3,7 @@
 # found in the LICENSE file.
 
 import os
+import unittest
 
 from telemetry import page as page_module
 from telemetry.page import page_set
@@ -44,7 +45,6 @@ class PageTestResultsTest(base_test_results_unittest.BaseTestResultsUnittest):
     self.assertTrue(results.all_page_runs[0].failed)
     self.assertTrue(results.all_page_runs[1].ok)
 
-
   def testSkips(self):
     results = page_test_results.PageTestResults()
     results.WillRunPage(self.pages[0])
@@ -84,38 +84,6 @@ class PageTestResultsTest(base_test_results_unittest.BaseTestResultsUnittest):
     values = results.FindAllPageSpecificValuesNamed('a')
     assert len(values) == 2
 
-  def testResultsFiltering(self):
-    def AcceptValueNamed_a(value):
-      return value.name == 'a'
-    results = page_test_results.PageTestResults(
-        value_can_be_added_predicate=AcceptValueNamed_a)
-    results.WillRunPage(self.pages[0])
-    results.AddValue(scalar.ScalarValue(self.pages[0], 'a', 'seconds', 3))
-    results.AddValue(scalar.ScalarValue(self.pages[0], 'b', 'seconds', 3))
-    results.DidRunPage(self.pages[0])
-
-    results.WillRunPage(self.pages[1])
-    results.AddValue(scalar.ScalarValue(self.pages[1], 'a', 'seconds', 3))
-    results.AddValue(scalar.ScalarValue(self.pages[1], 'd', 'seconds', 3))
-    results.DidRunPage(self.pages[1])
-
-    results.PrintSummary()
-
-    values = results.FindPageSpecificValuesForPage(self.pages[0], 'a')
-    self.assertEquals(1, len(values))
-    v = values[0]
-    self.assertEquals(v.name, 'a')
-    self.assertEquals(v.page, self.pages[0])
-
-    values = results.FindPageSpecificValuesForPage(self.pages[0], 'b')
-    self.assertEquals(0, len(values))
-
-    values = results.FindAllPageSpecificValuesNamed('a')
-    self.assertEquals(len(values), 2)
-
-    values = results.all_page_specific_values
-    self.assertEquals(len(values), 2)
-
   def testUrlIsInvalidValue(self):
     results = page_test_results.PageTestResults()
     results.WillRunPage(self.pages[0])
@@ -266,3 +234,69 @@ class PageTestResultsTest(base_test_results_unittest.BaseTestResultsUnittest):
 
     results.CleanUp()
     self.assertFalse(results.FindAllTraceValues())
+
+
+class PageTestResultsFilterTest(unittest.TestCase):
+  def setUp(self):
+    ps = page_set.PageSet(file_path=os.path.dirname(__file__))
+    ps.AddUserStory(page_module.Page('http://www.foo.com/', ps, ps.base_dir))
+    ps.AddUserStory(page_module.Page('http://www.bar.com/', ps, ps.base_dir))
+    self.page_set = ps
+
+  @property
+  def pages(self):
+    return self.page_set.pages
+
+  def testFilterValue(self):
+    def AcceptValueNamed_a(value, _):
+      return value.name == 'a'
+    results = page_test_results.PageTestResults(
+        value_can_be_added_predicate=AcceptValueNamed_a)
+    results.WillRunPage(self.pages[0])
+    results.AddValue(scalar.ScalarValue(self.pages[0], 'a', 'seconds', 3))
+    results.AddValue(scalar.ScalarValue(self.pages[0], 'b', 'seconds', 3))
+    results.DidRunPage(self.pages[0])
+
+    results.WillRunPage(self.pages[1])
+    results.AddValue(scalar.ScalarValue(self.pages[1], 'a', 'seconds', 3))
+    results.AddValue(scalar.ScalarValue(self.pages[1], 'd', 'seconds', 3))
+    results.DidRunPage(self.pages[1])
+    results.PrintSummary()
+    self.assertEquals(
+        [('a', 'http://www.foo.com/'), ('a', 'http://www.bar.com/')],
+        [(v.name, v.page.url) for v in results.all_page_specific_values])
+
+  def testFilterIsFirstResult(self):
+    def AcceptSecondValues(_, is_first_result):
+      return not is_first_result
+    results = page_test_results.PageTestResults(
+        value_can_be_added_predicate=AcceptSecondValues)
+
+    # First results (filtered out)
+    results.WillRunPage(self.pages[0])
+    results.AddValue(scalar.ScalarValue(self.pages[0], 'a', 'seconds', 7))
+    results.AddValue(scalar.ScalarValue(self.pages[0], 'b', 'seconds', 8))
+    results.DidRunPage(self.pages[0])
+    results.WillRunPage(self.pages[1])
+    results.AddValue(scalar.ScalarValue(self.pages[1], 'a', 'seconds', 5))
+    results.AddValue(scalar.ScalarValue(self.pages[1], 'd', 'seconds', 6))
+    results.DidRunPage(self.pages[1])
+
+    # Second results
+    results.WillRunPage(self.pages[0])
+    results.AddValue(scalar.ScalarValue(self.pages[0], 'a', 'seconds', 3))
+    results.AddValue(scalar.ScalarValue(self.pages[0], 'b', 'seconds', 4))
+    results.DidRunPage(self.pages[0])
+    results.WillRunPage(self.pages[1])
+    results.AddValue(scalar.ScalarValue(self.pages[1], 'a', 'seconds', 1))
+    results.AddValue(scalar.ScalarValue(self.pages[1], 'd', 'seconds', 2))
+    results.DidRunPage(self.pages[1])
+    results.PrintSummary()
+    expected_values = [
+        ('a', 'http://www.foo.com/', 3),
+        ('b', 'http://www.foo.com/', 4),
+        ('a', 'http://www.bar.com/', 1),
+        ('d', 'http://www.bar.com/', 2)]
+    actual_values = [(v.name, v.page.url, v.value)
+                     for v in results.all_page_specific_values]
+    self.assertEquals(expected_values, actual_values)

tools/telemetry/telemetry/results/results_options.py

@@ -108,7 +108,7 @@ def _GetProgressReporter(output_skipped_tests_summary, suppress_gtest_report):
 
 
 def CreateResults(benchmark_metadata, options,
-                  value_can_be_added_predicate=lambda v: True):
+                  value_can_be_added_predicate=lambda v, is_first: True):
   """
   Args:
     options: Contains the options specified in AddResultsOptions.

tools/telemetry/telemetry/user_story/user_story_runner.py

@@ -218,7 +218,6 @@ def Run(test, user_story_set, expectations, finder_options, results,
 
   user_story_groups = GetUserStoryGroupsWithSameSharedUserStoryClass(
       user_stories)
-  user_story_with_discarded_first_results = set()
 
   for group in user_story_groups:
     state = None
@@ -250,14 +249,7 @@ def Run(test, user_story_set, expectations, finder_options, results,
               try:
                 if state:
                   _CheckThermalThrottling(state.platform)
-                # TODO(slamm): Make discard_first_result part of user_story API.
-                # https://crbug.com/440101
-                discard_current_run = (
-                    getattr(test, 'discard_first_result', False) and
-                    user_story not in user_story_with_discarded_first_results)
-                if discard_current_run:
-                  user_story_with_discarded_first_results.add(user_story)
-                results.DidRunPage(user_story, discard_run=discard_current_run)
+                results.DidRunPage(user_story)
               except Exception:
                 if not has_existing_exception:
                   raise

tools/telemetry/telemetry/user_story/user_story_runner_unittest.py

@@ -355,61 +355,6 @@ class UserStoryRunnerTest(unittest.TestCase):
     # The AppCrashException gets added as a failure.
     self.assertEquals(1, len(self.results.failures))
 
-  def testDiscardFirstResult(self):
-    us = user_story_set.UserStorySet()
-    us.AddUserStory(DummyLocalUserStory(TestSharedPageState))
-    us.AddUserStory(DummyLocalUserStory(TestSharedPageState))
-    class Measurement(page_test.PageTest):
-      @property
-      def discard_first_result(self):
-        return True
-
-      def RunPage(self, page, _, results):
-        results.AddValue(string.StringValue(page, 'test', 't', page.name))
-
-      def ValidateAndMeasurePage(self, page, tab, results):
-        pass
-
-    results = results_options.CreateResults(
-        EmptyMetadataForTest(), self.options)
-    user_story_runner.Run(
-        Measurement(), us, self.expectations, self.options, results)
-
-    self.assertEquals(0, GetNumberOfSuccessfulPageRuns(results))
-    self.assertEquals(0, len(results.failures))
-    self.assertEquals(0, len(results.all_page_specific_values))
-
-
-    results = results_options.CreateResults(
-        EmptyMetadataForTest(), self.options)
-    self.options.page_repeat = 1
-    self.options.pageset_repeat = 2
-    user_story_runner.Run(
-        Measurement(), us, self.expectations, self.options, results)
-    self.assertEquals(2, GetNumberOfSuccessfulPageRuns(results))
-    self.assertEquals(0, len(results.failures))
-    self.assertEquals(2, len(results.all_page_specific_values))
-
-    results = results_options.CreateResults(
-        EmptyMetadataForTest(), self.options)
-    self.options.page_repeat = 2
-    self.options.pageset_repeat = 1
-    user_story_runner.Run(
-        Measurement(), us, self.expectations, self.options, results)
-    self.assertEquals(2, GetNumberOfSuccessfulPageRuns(results))
-    self.assertEquals(0, len(results.failures))
-    self.assertEquals(2, len(results.all_page_specific_values))
-
-    results = results_options.CreateResults(
-        EmptyMetadataForTest(), self.options)
-    self.options.page_repeat = 1
-    self.options.pageset_repeat = 1
-    user_story_runner.Run(
-        Measurement(), us, self.expectations, self.options, results)
-    self.assertEquals(0, GetNumberOfSuccessfulPageRuns(results))
-    self.assertEquals(0, len(results.failures))
-    self.assertEquals(0, len(results.all_page_specific_values))
-
   def testPagesetRepeat(self):
     us = user_story_set.UserStorySet()
     us.AddUserStory(DummyLocalUserStory(TestSharedPageState, name='blank'))