Bug 151286 – Highlight the alarming test results in the graphics benchmark results page

• Component: Animations
• Status: RESOLVED FIXED

[Said Abou-Hallawa]

Because the number of tests has been increasing and many data are displayed for every test, we need an easy way to show visually the inaccurate results. The criteria I chose is the following:

If the standard deviation of the test complexity or the frame rate is more than 10%, the standard deviation and the test name will be displayed in red.
If the average frame rate is not in the range = [(desired_frame_rate - 2) .. (desired_frame_rate - 2)], the average frame rate and the test name will be displayed in red.

[Said Abou-Hallawa]

Created attachment 265524 [details]
Patch

[Simon Fraser (smfr)]

Comment on attachment 265524 [details]
Patch

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

> PerformanceTests/Animometer/resources/extensions.js:323
> +        td.style.color = color || "black";

Can this be done in CSS?

> PerformanceTests/Animometer/resources/extensions.js:330
> +        td.style.color = color || "black";

Can this be done in CSS?

> PerformanceTests/Animometer/resources/extensions.js:372
> +    _isAlarmingMeasurement: function(index, data, measurement, options)

Maybe "unreliable" or "noisy" instead of "alarming".

> PerformanceTests/Animometer/resources/extensions.js:375
> +        if (measurement == Strings["JSON_MEASUREMENTS"][3])
> +            return data[Strings["JSON_MEASUREMENTS"][3]] >= 10;

3 is a magic number, as is 10.

> PerformanceTests/Animometer/resources/extensions.js:378
> +        if (index == 1 && measurement == Strings["JSON_MEASUREMENTS"][0])
> +            return Math.abs(data[Strings["JSON_MEASUREMENTS"][0]] - options["frame-rate"]) > 2;

0 and 2 are a magic numbers.

> PerformanceTests/Animometer/resources/extensions.js:383
> +    _isAlarmingTestResults: function(testResults, options)

results is plural, so areNoisyTestResults, or isNoisyTestResult

> PerformanceTests/Animometer/resources/extensions.js:400
> +                td.style.backgroundColor = "yellow";

CSS?. td.classList.add('empty')

.empty { backgrouncColor: yellow; }

> PerformanceTests/Animometer/resources/extensions.js:434
> +                    this._showFixedNumber(row, data[measurement], 2, this._isAlarmingMeasurement(index - 2, data, measurement, options) ? "red" : null);

"red" should come from CSS.

> PerformanceTests/Animometer/runner/resources/animometer.js:49
> +        this._resultsTable.showIterations(json[Strings["JSON_RESULTS"][0]], this.options);
>          sectionsManager.showJSON("json", json[Strings["JSON_RESULTS"][0]][0]);

This Strings["JSON_RESULTS"][][] stuff too hard to read.

[Said Abou-Hallawa]

Created attachment 265618 [details]
Patch

[Said Abou-Hallawa]

Comment on attachment 265524 [details]
Patch

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

>> PerformanceTests/Animometer/resources/extensions.js:323
>> +        td.style.color = color || "black";
> 
> Can this be done in CSS?

Done. Moved to CSS.

>> PerformanceTests/Animometer/resources/extensions.js:330
>> +        td.style.color = color || "black";
> 
> Can this be done in CSS?

Done. Text color was moved to CSS.

>> PerformanceTests/Animometer/resources/extensions.js:372
>> +    _isAlarmingMeasurement: function(index, data, measurement, options)
> 
> Maybe "unreliable" or "noisy" instead of "alarming".

Function was renamed to _isNoisyMeasurement().

>> PerformanceTests/Animometer/resources/extensions.js:375
>> +            return data[Strings["JSON_MEASUREMENTS"][3]] >= 10;
> 
> 3 is a magic number, as is 10.

A constant is used for the noise threshold. We may need to add a UI element to be set by the user.

>> PerformanceTests/Animometer/resources/extensions.js:378
>> +            return Math.abs(data[Strings["JSON_MEASUREMENTS"][0]] - options["frame-rate"]) > 2;
> 
> 0 and 2 are a magic numbers.

A constant is used for the noise threshold. We may need to add a UI element to be set by the user.

>> PerformanceTests/Animometer/resources/extensions.js:383
>> +    _isAlarmingTestResults: function(testResults, options)
> 
> results is plural, so areNoisyTestResults, or isNoisyTestResult

Function was renamed to isNoisyTest()?

>> PerformanceTests/Animometer/resources/extensions.js:400
>> +                td.style.backgroundColor = "yellow";
> 
> CSS?. td.classList.add('empty')
> 
> .empty { backgrouncColor: yellow; }

Done. Background color was moved to CSS.

>> PerformanceTests/Animometer/resources/extensions.js:434
>> +                    this._showFixedNumber(row, data[measurement], 2, this._isAlarmingMeasurement(index - 2, data, measurement, options) ? "red" : null);
> 
> "red" should come from CSS.

Done. Text color was moved to CSS.

>> PerformanceTests/Animometer/runner/resources/animometer.js:49
>>          sectionsManager.showJSON("json", json[Strings["JSON_RESULTS"][0]][0]);
> 
> This Strings["JSON_RESULTS"][][] stuff too hard to read.

Will be addressed in a separate patch.

[WebKit Commit Bot]

Comment on attachment 265618 [details]
Patch

Rejecting attachment 265618 [details] from commit-queue.

Failed to run "['/Volumes/Data/EWS/WebKit/Tools/Scripts/webkit-patch', '--status-host=webkit-queues.webkit.org', '--bot-id=webkit-cq-01', 'land-attachment', '--force-clean', '--non-interactive', '--parent-command=commit-queue', 265618, '--port=mac']" exit_code: 2 cwd: /Volumes/Data/EWS/WebKit

Last 500 characters of output:
824ee64cb6e6faad827b20803d9cae0b8db
r192486 = 342967a7ac7e367e3087ec7be1f1857a3ac17553
r192487 = 79e502480923be3865d58e4be67b1a4c49d008f5
r192489 = cd20c088fd743713948cbc21ed53930b5cc1437c
r192490 = 7bcfba0371cd3ecbc45cf6c3cba98bd648e069fe
r192491 = 4ac83a2d03fe019772cd95bba9292f33a499ce74
Done rebuilding .git/svn/refs/remotes/origin/master/.rev_map.268f45cc-cd09-0410-ab3c-d52691b4dbfc
First, rewinding head to replay your work on top of it...
Fast-forwarded master to refs/remotes/origin/master.

Full output: http://webkit-queues.webkit.org/results/438578

[Said Abou-Hallawa]

Created attachment 265630 [details]
Patch

[WebKit Commit Bot]

Comment on attachment 265630 [details]
Patch

Clearing flags on attachment: 265630

Committed r192494: <http://trac.webkit.org/changeset/192494>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.