Bug 86696 – [Performance test] Support "description" for PerfTestRunner.run and PerfTestRunner.runPerSecond

• Status: RESOLVED
• Resolution: FIXED
• Component: Tools / Tests

[Kentaro Hara]

We want to add some description for each run, so that people can know what each run is testing.

[Kentaro Hara]

Created attachment 142394 [details]
Patch

[Ryosuke Niwa]

Comment on attachment 142394 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=142394&action=review

> Tools/ChangeLog:16
> +        DESCRIPTION: Benchmark for DOM attributes that return a Node object.

Nice!

> Tools/Scripts/webkitpy/performance_tests/perftest.py:98
> +        description_regex = re.compile(r'^description (?P<description>.*)$')

We should probably ignore cases here :)

> PerformanceTests/resources/runner.js:97
> +        this.log("description " + description);

Why don't we capitalize description and put a comma as in: "Description: ~~~"

> PerformanceTests/resources/runner.js:126
> +        this.logStatistics(this._results, this._description);

It seems odd that a function named logStatistics also prints description. Why don't we put the code here instead?

[Kentaro Hara]

Created attachment 142397 [details]
patch for landing

[Kentaro Hara]

(In reply to comment #2)
> > Tools/Scripts/webkitpy/performance_tests/perftest.py:98
> > +        description_regex = re.compile(r'^description (?P<description>.*)$')
> 
> We should probably ignore cases here :)

Done.

> > PerformanceTests/resources/runner.js:97
> > +        this.log("description " + description);
> 
> Why don't we capitalize description and put a comma as in: "Description: ~~~"

Done.

> > PerformanceTests/resources/runner.js:126
> > +        this.logStatistics(this._results, this._description);
> 
> It seems odd that a function named logStatistics also prints description. Why don't we put the code here instead?

Done.

[WebKit Review Bot]

Comment on attachment 142397 [details]
patch for landing

Clearing flags on attachment: 142397

Committed r117397: <http://trac.webkit.org/changeset/117397>

[Csaba Osztrogonác]

(In reply to comment #5)
> (From update of attachment 142397 [details])
> Clearing flags on attachment: 142397
> 
> Committed r117397: <http://trac.webkit.org/changeset/117397>

It broke perf tests on the bot:

Running PageLoad/svg/files/42450-under the see.svg (53 of 71)
Traceback (most recent call last):
  File "./Tools/Scripts/run-perf-tests", line 39, in <module>
    sys.exit(PerfTestsRunner().run())
  File "/home/webkitbuildbot/slaves/release64bitWebKit2-perf/buildslave/qt-linux-64-release-wk2-perf-tests/build/Tools/Scripts/webkitpy/performance_tests/perftestsrunner.py", line 138, in run
    unexpected = self._run_tests_set(sorted(list(tests), key=lambda test: test.test_name()), self._port)
  File "/home/webkitbuildbot/slaves/release64bitWebKit2-perf/buildslave/qt-linux-64-release-wk2-perf-tests/build/Tools/Scripts/webkitpy/performance_tests/perftestsrunner.py", line 226, in _run_tests_set
    if self._run_single_test(test, driver):
  File "/home/webkitbuildbot/slaves/release64bitWebKit2-perf/buildslave/qt-linux-64-release-wk2-perf-tests/build/Tools/Scripts/webkitpy/performance_tests/perftestsrunner.py", line 240, in _run_single_test
    new_results = test.run(driver, self._options.time_out_ms)
  File "/home/webkitbuildbot/slaves/release64bitWebKit2-perf/buildslave/qt-linux-64-release-wk2-perf-tests/build/Tools/Scripts/webkitpy/performance_tests/perftest.py", line 194, in run
    self.output_statistics(self.test_name(), results)
  File "/home/webkitbuildbot/slaves/release64bitWebKit2-perf/buildslave/qt-linux-64-release-wk2-perf-tests/build/Tools/Scripts/webkitpy/performance_tests/perftest.py", line 132, in output_statistics
    if results['description']:
KeyError: 'description'

[Kentaro Hara]

(In reply to comment #6)
> It broke perf tests on the bot:

looking

[Ryosuke Niwa]

Comment on attachment 142397 [details]
patch for landing

View in context: https://bugs.webkit.org/attachment.cgi?id=142397&action=review

> Tools/Scripts/webkitpy/performance_tests/perftest.py:132
> +        if results['description']:

Oops, you can't assume that results['description'] is always yet :(

[Kentaro Hara]

Reopening to attach new patch.

[Kentaro Hara]

Created attachment 142431 [details]
Patch

[Ryosuke Niwa]

Comment on attachment 142431 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=142431&action=review

> Tools/Scripts/webkitpy/performance_tests/perftest.py:193
> +            'description': '',

How about ChromiumStylePerfTest ?

[Kentaro Hara]

(In reply to comment #11)
> How about ChromiumStylePerfTest ?

I want to fix it and confirm the bahavior, but how can I run ChromiumStylePerfTest?

[Ryosuke Niwa]

(In reply to comment #12)
> (In reply to comment #11)
> > How about ChromiumStylePerfTest ?
> 
> I want to fix it and confirm the bahavior, but how can I run ChromiumStylePerfTest?

Tests in PerformanceTests/Inspector. However, they're skipped now.

[Kentaro Hara]

Created attachment 142434 [details]
patch for landing

[Kentaro Hara]

Committed r117410: <http://trac.webkit.org/changeset/117410>

[Csaba Osztrogonác]

(In reply to comment #15)
> Committed r117410: <http://trac.webkit.org/changeset/117410>

perf test is still broken, but with another error message:
Uploaded JSON but got a bad response:
The payload doesn't contain results or results are malformed

[Kentaro Hara]

It seems that we need to fix the server side too. Just a moment...

[Kentaro Hara]

Reopening to attach new patch.

[Kentaro Hara]

Created attachment 142446 [details]
Patch

[Csaba Osztrogonác]

Comment on attachment 142446 [details]
Patch

LGTM, r=me.

[Kentaro Hara]

Committed r117422: <http://trac.webkit.org/changeset/117422>

[Csaba Osztrogonác]

And I realized that unit tests are bleeding too: http://build.webkit.org/builders/Qt%20Linux%20Release/builds/47128/steps/webkitpy-test/logs/stdio

[Kentaro Hara]

(In reply to comment #22)
> And I realized that unit tests are bleeding too: http://build.webkit.org/builders/Qt%20Linux%20Release/builds/47128/steps/webkitpy-test/logs/stdio

Is this failure still happening even after landing r117422? Now 'results' does not have 'description'.

[Csaba Osztrogonác]

(In reply to comment #23)
> (In reply to comment #22)
> > And I realized that unit tests are bleeding too: http://build.webkit.org/builders/Qt%20Linux%20Release/builds/47128/steps/webkitpy-test/logs/stdio
> 
> Is this failure still happening even after landing r117422? Now 'results' does not have 'description'.

Oh, sorry for the noise r117422 fixed all of them, but bots are in late.