Bug 154028 – Add a ramp controller

• Component: WebCore Misc.
• Status: RESOLVED FIXED

[Jon Lee]

Add a ramp controller

[Jon Lee]

Created attachment 270925 [details]
Patch

[Said Abou-Hallawa]

Comment on attachment 270925 [details]
Patch

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

> PerformanceTests/Animometer/resources/debug-runner/graph.js:63
> +        }, this);

Do we really need a for loop here? I think flattening it is more efficient and require less lines of code.

> PerformanceTests/Animometer/resources/debug-runner/graph.js:79
> +                if (!regression.startIndex)

Why regression with startIndex = 0 are excluded from calculating the range? I might be overlooked the code or confused.

> PerformanceTests/Animometer/resources/debug-runner/graph.js:243
> +            });

Can't we use  Math.max.apply()? Something like:

var complexityMax = Math.max.apply(Math, graphData.timeRegressions.map(function(regression) {
  return regression.complexity || 0;
}));

> PerformanceTests/Animometer/tests/resources/main.js:303
> +        // x is complexity, y is fps. In this formulation s1 = 60fps and t1 = 0

This is confusing comment. My understanding is the following:
1. We have two curves x-t curve (for the complexity) and y-t curve (for the fameLength or fps).
2. We want to get the linear regression for the x-y curve (complexity is x and fps is y)

-- Can't we replace s1 and s2 by y1 and y2? So we should remember only three variables: t, x and y.  
-- Can't we replace (y - m)^2 by something like (y - y_m)^2? Or something similar but just make it relates to y.
-- I do not understand the calculation m = s2 + t2*x? m is supposed to be fps so how can time * complexity yields fps?

> PerformanceTests/Animometer/tests/resources/main.js:312
> +        for (var i = startIndex; iterationDirection * (endIndex - i) > -1; i += iterationDirection) {

I do not understand why the order of traversing the sampled data matters here. We are getting sum(x), sum(y), sum(x^2), sum(y^2) and sum(xy). right? Would not 
for (var i = Math.min(startIndex, endIndex); i < Math.max(startIndex, endIndex); ++i) { ... }

give the same result?

> PerformanceTests/Animometer/tests/resources/main.js:317
> +            var yy = y * y;

Do we need to create these variables?

> PerformanceTests/Animometer/tests/resources/main.js:318
> +            a2 += 1;

Should not a2 = Math.abs(endIndex - startIndex);

> PerformanceTests/Animometer/tests/resources/main.js:358
> +            k2 -= yy;

Can't we create two arrays for these sums in the previous loop? For example
arr1[i].x:  the sum of complexities from startIndex till i.
arr2[i].y: the sum of sum of frameLengths from i+1 till endIndex.

> PerformanceTests/Animometer/tests/resources/main.js:365
> +            var F = a2*c2 - b2*b2;

Can't we create another array {A, B, C, D, E, F} in the previous loop?

> PerformanceTests/Animometer/tests/resources/main.js:374
> +            var error2 = (k2 + a2*s2*s2 + c2*t2*t2 - 2*d2*s2 - 2*h2*t2 + 2*b2*s2*t2) || 0;

Can't we create another array for {s1,s2, t1, t2, error1, error2} in the previous loop?  I think this loop is a little bit long and shortening it can make understanding the math easier.

> PerformanceTests/Animometer/tests/resources/main.js:386
> +            // Projected point is not between this and the next sample

Maybe it is worth mentioning what exactly this block of math is calculating or adding a link explaining its math.

> PerformanceTests/Animometer/tests/resources/main.js:418
> +                set_x_best(x_prime, x);

Can't we move this code in a shared function instead of repeating it twice? Or can't we delete if (i == startIndex) {...} and change the following if-statments to be

if (i != startIndex && (x_prime > getComplexity(samples, i + iterationDirection) || x_prime < x)) { ... }

if (i == startIndex || error1 + error2 < error_best) { ... }

> PerformanceTests/Animometer/tests/resources/main.js:500
> +                    var nextTierComplexity = Math.round(Math.pow(10, this._tier));

Do we really need to increase the complexity exponentially? And even if we need to, cannot we use lower power? I guess aggressive increasing for the complexity might give inaccurate results about for the system behavior with large number of particles.

> PerformanceTests/Animometer/tests/resources/main.js:528
> +        else if (!this._rampDidWarmup) {

No need for the else.

> PerformanceTests/Animometer/tests/resources/main.js:543
> +        var desiredComplexity = stage.complexity();

This assignment is not needed since its value will be changed in both the if-block and the else-block.

[Jon Lee]

Comment on attachment 270925 [details]
Patch

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

>> PerformanceTests/Animometer/resources/debug-runner/graph.js:63
>> +        }, this);
> 
> Do we really need a for loop here? I think flattening it is more efficient and require less lines of code.

ok

>> PerformanceTests/Animometer/resources/debug-runner/graph.js:79
>> +                if (!regression.startIndex)
> 
> Why regression with startIndex = 0 are excluded from calculating the range? I might be overlooked the code or confused.

This is leftover from old code. I had regression segments split out before, and some of them did not have startIndex in them. I'll remove.

>> PerformanceTests/Animometer/resources/debug-runner/graph.js:243
>> +            });
> 
> Can't we use  Math.max.apply()? Something like:
> 
> var complexityMax = Math.max.apply(Math, graphData.timeRegressions.map(function(regression) {
>   return regression.complexity || 0;
> }));

done

>> PerformanceTests/Animometer/tests/resources/main.js:303
>> +        // x is complexity, y is fps. In this formulation s1 = 60fps and t1 = 0
> 
> This is confusing comment. My understanding is the following:
> 1. We have two curves x-t curve (for the complexity) and y-t curve (for the fameLength or fps).
> 2. We want to get the linear regression for the x-y curve (complexity is x and fps is y)
> 
> -- Can't we replace s1 and s2 by y1 and y2? So we should remember only three variables: t, x and y.  
> -- Can't we replace (y - m)^2 by something like (y - y_m)^2? Or something similar but just make it relates to y.
> -- I do not understand the calculation m = s2 + t2*x? m is supposed to be fps so how can time * complexity yields fps?

This regression is used for complexity-fps analysis, but it's a generic regression calculator, so the axes can represent anything.

For the time graph, y is fps and x is complexity; it just so happens that x and t linearly line up. So, s2 is the fps bias, and t2 is the fps/complexity.

>> PerformanceTests/Animometer/tests/resources/main.js:312
>> +        for (var i = startIndex; iterationDirection * (endIndex - i) > -1; i += iterationDirection) {
> 
> I do not understand why the order of traversing the sampled data matters here. We are getting sum(x), sum(y), sum(x^2), sum(y^2) and sum(xy). right? Would not 
> for (var i = Math.min(startIndex, endIndex); i < Math.max(startIndex, endIndex); ++i) { ... }
> 
> give the same result?

It doesn't, but I use this calculation twice--once for a slope based regression and another for a step-wise. In both cases the first segment is fixed at 60 fps with no slope. For the ramp regression, it goes from more complicated to 0, so we have to iterate through the data backwards in time to fit the regression model.

>> PerformanceTests/Animometer/tests/resources/main.js:317
>> +            var yy = y * y;
> 
> Do we need to create these variables?

you're right. I can move them down.

>> PerformanceTests/Animometer/tests/resources/main.js:318
>> +            a2 += 1;
> 
> Should not a2 = Math.abs(endIndex - startIndex);

it could be. It's just nice to have the parallel structure.

>> PerformanceTests/Animometer/tests/resources/main.js:358
>> +            k2 -= yy;
> 
> Can't we create two arrays for these sums in the previous loop? For example
> arr1[i].x:  the sum of complexities from startIndex till i.
> arr2[i].y: the sum of sum of frameLengths from i+1 till endIndex.

We could but I don't know what the benefit is with allocating large arrays to do this calculation.

>> PerformanceTests/Animometer/tests/resources/main.js:418
>> +                set_x_best(x_prime, x);
> 
> Can't we move this code in a shared function instead of repeating it twice? Or can't we delete if (i == startIndex) {...} and change the following if-statments to be
> 
> if (i != startIndex && (x_prime > getComplexity(samples, i + iterationDirection) || x_prime < x)) { ... }
> 
> if (i == startIndex || error1 + error2 < error_best) { ... }

I think the control flow ends being less understandable with this way. It's easier to see earlier iteration control flow concentrated at the top, and go further down the loop as we iterate through.

I can update this to setBest(s1, t1, s2, t2, x_prime, x);

>> PerformanceTests/Animometer/tests/resources/main.js:500
>> +                    var nextTierComplexity = Math.round(Math.pow(10, this._tier));
> 
> Do we really need to increase the complexity exponentially? And even if we need to, cannot we use lower power? I guess aggressive increasing for the complexity might give inaccurate results about for the system behavior with large number of particles.

We're just looking for ballpark. Yes, I think we should do it exponentially; no test goes past 10000 items, and this minimizes the time needed to setup before the benchmark starts.

>> PerformanceTests/Animometer/tests/resources/main.js:528
>> +        else if (!this._rampDidWarmup) {
> 
> No need for the else.

done

>> PerformanceTests/Animometer/tests/resources/main.js:543
>> +        var desiredComplexity = stage.complexity();
> 
> This assignment is not needed since its value will be changed in both the if-block and the else-block.

done.

[Said Abou-Hallawa]

Unofficial r=me

[Jon Lee]

Comment on attachment 270925 [details]
Patch

committed 196372