Bug 151208 – Calculate the graphics benchmark test gain adaptively

• Status: RESOLVED
• Resolution: FIXED
• Component: Animations

[Said Abou-Hallawa]

Tuning the PID controller of the graphics benchmark manually is time consuming and it does not necessarily give the perfect/fair results. We need to calculate the gain adaptively and get rid of the gain and limits parameters we have to choose manually for every test. Many ways were suggested by automatic control books and papers but we chose to use the classic Ziegler–Nichols method.

The basic idea is start with Ku = 0, eliminate the I and D terms of the PID and return the P term only; P = Ku * e. Then increment the Kp gradually till the set point is reached. We stop incrementing Ku and measure the oscillation period Tu. After measuring the Ku and Tu, we calculate the controller parameters according to Ziegler–Nichols method for control type = some overshot; see https://en.wikipedia.org/wiki/Ziegler–Nichols_method.

I could not find clear description on how Ku is initialized and by how much it can be incremented. This is very cortical to the graphics benchmark since some tests saturate with small number of items, e.g. 50 items, while others saturate with large number of items, e.g. more than 5000 items. So incrementing by small values will require long time to reach the set-point for tests with large number of items. And incrementing by large values will cause the test to be unstable.

The ideal solution is
1. Start with only one item in the test.
2. Increment Ku very slowly at the beginning.
3. Increment Ku significantly but moderately if the test is not responsive to the small tuning values.

-- For the first point we can choose Ku = -1 / e which will return P = Ku * e = -1. This will add one item to the test.
-- For the second point we calculate an ultimate distance from y0 after time t such that it moves very slowly at the beginning but very fast later on. We can use a cubic formula for the ultimate distance such that it reaches the set point after 1000ms. Then we can use the ratio between the ultimate distance and the current distance to tell us how much the system is responsive. The ratio will be very small at the beginning with all tests but will be very high with tests which require many items to saturate.
-- For the third point we can use the natural logarithm for the ratio of the ultimate distance to the current distance. The natural logarithm should be good but moderate value for large ratios.

[Said Abou-Hallawa]

Created attachment 265429 [details]
Patch

[Said Abou-Hallawa]

<rdar://problem/22882967>

[Said Abou-Hallawa]

Created attachment 265449 [details]
Patch

[Said Abou-Hallawa]

Created attachment 265462 [details]
Patch

[Simon Fraser (smfr)]

Comment on attachment 265462 [details]
Patch

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

This patch is trying to do too many things which makes it hard to review.

> PerformanceTests/Animometer/resources/extensions.js:164
> +        for (var i = 0; i < styleSheets.length; ++i) {
> +            if (!styleSheets[i].cssRules)
> +                continue;
> +
>              for (var j = 0; j < styleSheets[i].cssRules.length; ++j) {
>                  if (styleSheets[i].cssRules[j].selectorText == referenceRule) {

This is working around a bug in the JS, and should just go away by using media queries.

> PerformanceTests/Animometer/resources/extensions.js:336
> -    _showEmpty: function(row)
> +    _showEmptyCell: function(row)
>      {
> -        var td = document.createElement("td");
> -        row.appendChild(td);
> +        return DocumentExtension.createElement("td", {}, row);
>      },
>  
> -    _showText: function(row, text)
> +    _showText: function(row, text, color)
>      {
> -        var td = document.createElement("td");
> +        var td = DocumentExtension.createElement("td", {}, row);
>          td.textContent = text;
> -        row.appendChild(td);
> +        td.style.color = color || "black";
>      },
>  
> -    _showFixedNumber: function(row, value, digits)
> +    _showFixedNumber: function(row, value, digits, color)
>      {
> -        var td = document.createElement("td");
> +        var td = DocumentExtension.createElement("td", {}, row);
>          td.textContent = value.toFixed(digits || 2);
> -        row.appendChild(td);
> +        td.style.color = color || "black";
>      },

This doesn't seem relevant to the adaptive gain.

> PerformanceTests/Animometer/resources/extensions.js:404
> +    _isAccurateMeasurement: function(index, data, measurement, options)
>      {
> -        var row = document.createElement("tr");
> +        if (measurement == Strings["JSON_MEASUREMENTS"][3])
> +            return data[Strings["JSON_MEASUREMENTS"][3]] < 10;
> +            
> +        if (index == 1 && measurement == Strings["JSON_MEASUREMENTS"][0])
> +            return Math.abs(data[Strings["JSON_MEASUREMENTS"][0]] - options["frame-rate"]) < 2;
> +       
> +        return true;
> +    },
> +
> +    _isAccurateTest: function(testResults, options)
> +    {
> +        for (var index = 0; index < 2; ++index) {
> +            var data = testResults[Strings["JSON_EXPERIMENTS"][index]];
> +            for (var measurement in data) {
> +                if (!this._isAccurateMeasurement(index, data, measurement, options))
> +                    return false;
> +            }
> +        }    
> +        return true;
> +    },

It's not clear what the "accurate" is trying to imply. I'd read this as something that makes a judgement call about a result, but it seems to just fetch data from the json?

> PerformanceTests/Animometer/runner/resources/animometer.css:240
> +@media screen and (min-width: 1800px) {
> +    section {
> +        width: 1600px;
> +        height: 800px;
> +    }
> +}

So can you remove the insertCssRuleAfter() code now?

> PerformanceTests/Animometer/runner/resources/animometer.js:457
> +document.addEventListener("loaded", benchmarkController.initialize());

The event is called "load".

> PerformanceTests/Animometer/tests/resources/main.js:145
> -        if (this._method == "setInterval")
> -            this._animator.start();
> -        else
> -            this._animator.animateLoop();
> +        this._animator.animateLoop();

This could be done separately.

> PerformanceTests/Animometer/tests/resources/stage.js:44
> +        if (typeof this._size == "undefined") {

Seems like an odd way to do it. Can't you just set things in in an initialize function.

[Jon Lee]

Comment on attachment 265462 [details]
Patch

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

>> PerformanceTests/Animometer/runner/resources/animometer.js:457
>> +document.addEventListener("loaded", benchmarkController.initialize());
> 
> The event is called "load".

And presumably you'd be passing the function, not invoking it:

document.addEventListener("load", benchmarkController.initialize);

[Said Abou-Hallawa]

Created attachment 265528 [details]
Patch

[Said Abou-Hallawa]

Created attachment 265910 [details]
Patch

[Said Abou-Hallawa]

Comment on attachment 265910 [details]
Patch

I changed the patch a little to calculate the min-max value which prevents the system from fluctuating.

[Said Abou-Hallawa]

Comment on attachment 265462 [details]
Patch

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

>> PerformanceTests/Animometer/resources/extensions.js:164
>>                  if (styleSheets[i].cssRules[j].selectorText == referenceRule) {
> 
> This is working around a bug in the JS, and should just go away by using media queries.

This was done in https://bugs.webkit.org/show_bug.cgi?id=151327.

>> PerformanceTests/Animometer/resources/extensions.js:336
>>      },
> 
> This doesn't seem relevant to the adaptive gain.

This was moved to https://bugs.webkit.org/show_bug.cgi?id=151286.

>> PerformanceTests/Animometer/resources/extensions.js:404
>> +    },
> 
> It's not clear what the "accurate" is trying to imply. I'd read this as something that makes a judgement call about a result, but it seems to just fetch data from the json?

This has been moved to https://bugs.webkit.org/show_bug.cgi?id=151286.

>> PerformanceTests/Animometer/runner/resources/animometer.css:240
>> +}
> 
> So can you remove the insertCssRuleAfter() code now?

insertCssRuleAfter() was removed in https://bugs.webkit.org/show_bug.cgi?id=151327.

>>> PerformanceTests/Animometer/runner/resources/animometer.js:457
>>> +document.addEventListener("loaded", benchmarkController.initialize());
>> 
>> The event is called "load".
> 
> And presumably you'd be passing the function, not invoking it:
> 
> document.addEventListener("load", benchmarkController.initialize);

This was fixed in https://bugs.webkit.org/show_bug.cgi?id=151327.

>> PerformanceTests/Animometer/tests/resources/main.js:145
>> +        this._animator.animateLoop();
> 
> This could be done separately.

This was done in https://bugs.webkit.org/show_bug.cgi?id=151283.

>> PerformanceTests/Animometer/tests/resources/stage.js:44
>> +        if (typeof this._size == "undefined") {
> 
> Seems like an odd way to do it. Can't you just set things in in an initialize function.

This was fixed in https://bugs.webkit.org/show_bug.cgi?id=151289.

[Simon Fraser (smfr)]

Comment on attachment 265910 [details]
Patch

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

> PerformanceTests/Animometer/tests/resources/math.js:205
> +    BEFORE_YSP: 0,
> +    AFTER_YSP: 1

Can you expand YSP into words?

[Said Abou-Hallawa]

Created attachment 265916 [details]
Patch

[WebKit Commit Bot]

Comment on attachment 265916 [details]
Patch

Clearing flags on attachment: 265916

Committed r192669: <http://trac.webkit.org/changeset/192669>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.