Bug 154914 – Add some new controllers, and refine tests

• Status: RESOLVED
• Resolution: FIXED
• Component: WebCore Misc.

[Jon Lee]

Add a new 30-fps based ramp controller, and a fixed controller that does not step. Refine some of the tests.

[Jon Lee]

Created attachment 272647 [details]
1. Switch to ramp controller as default.

[Jon Lee]

Created attachment 272648 [details]
2. Improve tests.

[Jon Lee]

Created attachment 272649 [details]
3. Add a fixed controller, with no step.

[Jon Lee]

Created attachment 272650 [details]
4. Add a controller that centers around 30 fps instead of 60 fps.

[Jon Lee]

Created attachment 272651 [details]
Patch involving test harness

[Jon Lee]

Created attachment 272652 [details]
Patch with test updates (basically same as 2)

[Simon Fraser (smfr)]

Comment on attachment 272652 [details]
Patch with test updates (basically same as 2)

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

> PerformanceTests/Animometer/tests/master/resources/focus.js:138
> +        var focusProgress = .5 + .5 * Math.sin(time / this.focusDuration);

I would prefer 0.5

> PerformanceTests/Animometer/tests/master/resources/multiply.js:83
> +        this._distanceFactor = 1.5 * (1 - .5 * Math.max(this._offsetIndex - this._centerSpiralCount, 0) / this._sidePanelCount) / Math.sqrt(this._offsetIndex);

0.5 (there's a 0.5 above).

[Jon Lee]

(In reply to comment #7)
> Comment on attachment 272652 [details]
> Patch with test updates (basically same as 2)
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=272652&action=review
> 
> > PerformanceTests/Animometer/tests/master/resources/focus.js:138
> > +        var focusProgress = .5 + .5 * Math.sin(time / this.focusDuration);
> 
> I would prefer 0.5
> 
> > PerformanceTests/Animometer/tests/master/resources/multiply.js:83
> > +        this._distanceFactor = 1.5 * (1 - .5 * Math.max(this._offsetIndex - this._centerSpiralCount, 0) / this._sidePanelCount) / Math.sqrt(this._offsetIndex);
> 
> 0.5 (there's a 0.5 above).

Done for both.

[Said Abou-Hallawa]

Comment on attachment 272651 [details]
Patch involving test harness

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

> PerformanceTests/Animometer/resources/debug-runner/animometer.js:498
> +        if (dashboard.options["controller"].indexOf("ramp") != -1)

It took me awhile to realize that you want to match "ramp" or "ramp30". I am not sure if this is clear enough especially I need to look at the HTML file and look for the values that the "controller' field can take to see that.

> PerformanceTests/Animometer/resources/runner/animometer.js:-370
> -            "frame-rate": 50,

Why are you deleting the default "frame-rate" value? What should it be if the controller == 'adaptive' and its 'frame-rate' is not specified?

> PerformanceTests/Animometer/tests/resources/main.js:225
> +        options["interval-length"] = 0;

"interval-length" is a little bit vague name. I think it is not easy to understand what exactly this option for. Beside unlike other options, there is no element for it in the developer.html.

> PerformanceTests/Animometer/tests/resources/main.js:226
> +        Controller.call(this, benchmark, options);

The order here is tricky where you make the derived class controls the setting of this._intervalLength. I have to go to the constructor of the Controller class to see this statement first to understand why you put options["interval-length"] = 0 before calling the constructor. 

        // Length of subsequent intervals; a value of 0 means use no intervals
        this._intervalLength = options["interval-length"] || 100;

Can't we get rid of options["interval-length"] completely since there is no element for it in the developer.html and add the following statement after calling the constructor of the Controller class in both FixedController and StepController:

this._intervalLength = 0;

By the way this is what we do with this.initialComplexity: we set its default value in the Controller constructor and then we override its value here.

> PerformanceTests/Animometer/tests/resources/main.js:572
> +Ramp30Controller = Utilities.createSubclass(RampController,

I think I do not like this inheritance. RampController is actually Ramp60Controller. And Ramp30Controller is inheriting from it and just overrides the four class members below. Can't we instead make RampController takes the frame rate as input and calculate these four members accordingly?

> PerformanceTests/ChangeLog:31
> +        * Animometer/developer.html: Increase the test length to 20 seconds.

What is the gain we get by increasing the test length to 20 seconds? Do we get more consistent results by this increase?

[Said Abou-Hallawa]

Comment on attachment 272652 [details]
Patch with test updates (basically same as 2)

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

> PerformanceTests/Animometer/tests/master/resources/focus.js:26
> +        this.container = document.createElement('div');

Don't we need to set the parent of this.container?

> PerformanceTests/Animometer/tests/master/resources/focus.js:47
> +    },

Should this be a utility function? Something like Utilities.hideElement().

> PerformanceTests/Animometer/tests/master/resources/focus.js:52
> +    },

Ditto.

> PerformanceTests/Animometer/tests/master/resources/focus.js:92
> +        var particle = document.querySelector("#center-text div");

In the HTML, there is a rule for the selector "#stage div div". Aren't "#center-text div" and "#stage div div" the same? If they are, can't we use the same selector for consistency?

> PerformanceTests/Animometer/tests/master/resources/focus.js:115
> +            this._offsetIndex = Math.max(0, this._offsetIndex + count);
> +            for (var i = this._offsetIndex; i < this._testElements.length; ++i)
> +                this._testElements[i].hide();

Do you need to hide from this._offsetIndex to this._testElements.length?  I think you need to loop from the new _offsetIndex to the original _offsetIndex; like what you do in the (count > 0) case?

for (var i = newIndex; i < this._offsetIndex; ++i)
    this._testElements[i].hide();

> PerformanceTests/Animometer/tests/master/resources/focus.js:120
> +        if (newIndex > this._testElements.length) {

Do we need this if-statment? The termination condition in the for-loop is doing the same thing.

> PerformanceTests/ChangeLog:12
> +        overflow. This way, when the blur is applied, it avoid causing layer resizes.

Nit: avoids

[Jon Lee]

Comment on attachment 272651 [details]
Patch involving test harness

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

>> PerformanceTests/Animometer/resources/debug-runner/animometer.js:498
>> +        if (dashboard.options["controller"].indexOf("ramp") != -1)
> 
> It took me awhile to realize that you want to match "ramp" or "ramp30". I am not sure if this is clear enough especially I need to look at the HTML file and look for the values that the "controller' field can take to see that.

I can rewrite this.

>> PerformanceTests/Animometer/resources/runner/animometer.js:-370
>> -            "frame-rate": 50,
> 
> Why are you deleting the default "frame-rate" value? What should it be if the controller == 'adaptive' and its 'frame-rate' is not specified?

These are hard-coded for this page, so there's no opportunity to change it from the user's side.

>> PerformanceTests/Animometer/tests/resources/main.js:225
>> +        options["interval-length"] = 0;
> 
> "interval-length" is a little bit vague name. I think it is not easy to understand what exactly this option for. Beside unlike other options, there is no element for it in the developer.html.

Right. The idea is that options can be added by script.

I don't have a better name for it.

>> PerformanceTests/Animometer/tests/resources/main.js:226
>> +        Controller.call(this, benchmark, options);
> 
> The order here is tricky where you make the derived class controls the setting of this._intervalLength. I have to go to the constructor of the Controller class to see this statement first to understand why you put options["interval-length"] = 0 before calling the constructor. 
> 
>         // Length of subsequent intervals; a value of 0 means use no intervals
>         this._intervalLength = options["interval-length"] || 100;
> 
> Can't we get rid of options["interval-length"] completely since there is no element for it in the developer.html and add the following statement after calling the constructor of the Controller class in both FixedController and StepController:
> 
> this._intervalLength = 0;
> 
> By the way this is what we do with this.initialComplexity: we set its default value in the Controller constructor and then we override its value here.

Sure.

>> PerformanceTests/Animometer/tests/resources/main.js:572
>> +Ramp30Controller = Utilities.createSubclass(RampController,
> 
> I think I do not like this inheritance. RampController is actually Ramp60Controller. And Ramp30Controller is inheriting from it and just overrides the four class members below. Can't we instead make RampController takes the frame rate as input and calculate these four members accordingly?

There's no calculation here, they are hard-coded values. I don't think we should try to make a generic ramp controller.

>> PerformanceTests/ChangeLog:31
>> +        * Animometer/developer.html: Increase the test length to 20 seconds.
> 
> What is the gain we get by increasing the test length to 20 seconds? Do we get more consistent results by this increase?

Yes.

[Jon Lee]

Comment on attachment 272652 [details]
Patch with test updates (basically same as 2)

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

>> PerformanceTests/Animometer/tests/master/resources/focus.js:26
>> +        this.container = document.createElement('div');
> 
> Don't we need to set the parent of this.container?

That's done in tune().

>> PerformanceTests/Animometer/tests/master/resources/focus.js:47
>> +    },
> 
> Should this be a utility function? Something like Utilities.hideElement().

In other tests I use visibility instead, so it's tied to the technique of the test. Not sure this is worth pulling out into a utility function.

>> PerformanceTests/Animometer/tests/master/resources/focus.js:92
>> +        var particle = document.querySelector("#center-text div");
> 
> In the HTML, there is a rule for the selector "#stage div div". Aren't "#center-text div" and "#stage div div" the same? If they are, can't we use the same selector for consistency?

No. "#center-text div" has some extra special values for the styles, as you can see below.

>> PerformanceTests/Animometer/tests/master/resources/focus.js:115
>> +                this._testElements[i].hide();
> 
> Do you need to hide from this._offsetIndex to this._testElements.length?  I think you need to loop from the new _offsetIndex to the original _offsetIndex; like what you do in the (count > 0) case?
> 
> for (var i = newIndex; i < this._offsetIndex; ++i)
>     this._testElements[i].hide();

I felt it was a small optimization and not worth it. Everything that's already hidden stays as such. It matters in the other case because the amount we have to tune might still leave some already-allocated particles hidden.

>> PerformanceTests/Animometer/tests/master/resources/focus.js:120
>> +        if (newIndex > this._testElements.length) {
> 
> Do we need this if-statment? The termination condition in the for-loop is doing the same thing.

Removed.

>> PerformanceTests/ChangeLog:12
>> +        overflow. This way, when the blur is applied, it avoid causing layer resizes.
> 
> Nit: avoids

Done.

[Jon Lee]

Committed r197498: <http://trac.webkit.org/changeset/197498>

[Jon Lee]

Committed r197499: <http://trac.webkit.org/changeset/197499>