Bug 157738 – Improvements to Animometer benchmark

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

[Jon Lee]

Improvements to Animometer benchmark

[Jon Lee]

Created attachment 281484 [details]
1. CSS cleanup

[Jon Lee]

Created attachment 281486 [details]
2. Detect 3 canvas sizes

[Jon Lee]

Created attachment 281487 [details]
3. Show test confidence intervals, fix selection issues

[Jon Lee]

Created attachment 281488 [details]
4. Update serialization format to reduce size and make it extensible

[Jon Lee]

Created attachment 281489 [details]
5. Refine ramp lower bound evaluation, lengthen test

[Jon Lee]

Created attachment 281490 [details]
6. Refine ramp upper bound evaluation, let ramp profiles dictate final calculation

[Jon Lee]

Created attachment 281491 [details]
7. performance.now fallback

[Jon Lee]

Created attachment 281492 [details]
8. Confidence interval in final score

[Jon Lee]

Created attachment 281494 [details]
9. Test updates

[Jon Lee]

Created attachment 281495 [details]
Complete patch

[Said Abou-Hallawa]

Comment on attachment 281484 [details]
1. CSS cleanup

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

Unofficial r=me.

> PerformanceTests/Animometer/developer.html:7
> +    <title>MotionMark 1.0 - developer</title>

Do we have a plan to change the directory name also to: PerformanceTests/Animometer/ -> PerformanceTests/MotionMark?

> PerformanceTests/Animometer/developer.html:9
> +    <link rel="stylesheet" href="resources/runner/animometer.css">

Should we change all the sources anemometer.* -> motionmark.*?

> PerformanceTests/Animometer/developer.html:39
>                          <ul>

Do we need an extra indentation for <ul> relative to <form>?

> PerformanceTests/Animometer/developer.html:40
> +                        <li>

Do we need an extra indentation for <li> relative to <ul>?

> PerformanceTests/Animometer/resources/debug-runner/animometer.css:13
> +    background-color: rgb(96, 96, 96);
> +    background-image: initial;
> +    background-repeat: initial;
> +    background-size: initial;
> +    animation: initial;
> +    will-change: initial;

I could not understand why we have to set many properties to initial. Do we override the behavior of the parent element? But I do not see any need for that.

> PerformanceTests/Animometer/resources/debug-runner/animometer.css:189
> +#intro .orientation-check button {
> +    margin: 0 auto;
> +}

I think it is a little bit strange that we start the benchmark from inside a parent of class orientation-check. Can't we use a different name like "start-benchmark" or "benchmark-controller" or something else?

> PerformanceTests/Animometer/resources/debug-runner/animometer.css:195
> +        display: flex;

Why is this still using flex box layout?

> PerformanceTests/Animometer/resources/debug-runner/animometer.css:392
> +#results-tables {

I found it difficult to relate these css properties to the html elements hierarchy

<section id="results">
    <div class="body">
        <div id="results-tables">
            <div>
                <table id="results-score"></table>
                <table id="results-data"></table>
            </div>
            <table id="results-header"></table>
        </div>
    </div>
</section>

For example it looks like the tables are only inside results-tables but we always name them as "#results table"

> PerformanceTests/Animometer/resources/debug-runner/animometer.css:393
> +    direction: rtl;

Why we need the direction of this to be rtl?

[Said Abou-Hallawa]

Comment on attachment 281486 [details]
2. Detect 3 canvas sizes

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

Unofficial r=me

> PerformanceTests/Animometer/index.html:45
> +                    <div class="score" onclick="benchmarkController.showDebugInfo()"></div>

How does the user know that clicking the score will show the debugInfo?

[Jon Lee]

Comment on attachment 281484 [details]
1. CSS cleanup

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

>> PerformanceTests/Animometer/developer.html:39
>>                          <ul>
> 
> Do we need an extra indentation for <ul> relative to <form>?

Sure.

>> PerformanceTests/Animometer/developer.html:40
>> +                        <li>
> 
> Do we need an extra indentation for <li> relative to <ul>?

Sure.

>> PerformanceTests/Animometer/resources/debug-runner/animometer.css:13
>> +    will-change: initial;
> 
> I could not understand why we have to set many properties to initial. Do we override the behavior of the parent element? But I do not see any need for that.

This resets the properties that are defined in the release version of the benchmark.

>> PerformanceTests/Animometer/resources/debug-runner/animometer.css:189
>> +}
> 
> I think it is a little bit strange that we start the benchmark from inside a parent of class orientation-check. Can't we use a different name like "start-benchmark" or "benchmark-controller" or something else?

Sure.

>> PerformanceTests/Animometer/resources/debug-runner/animometer.css:195
>> +        display: flex;
> 
> Why is this still using flex box layout?

This is all for the developer version of the page, so I don't think it's as necessary to be as restrictive.

>> PerformanceTests/Animometer/resources/debug-runner/animometer.css:393
>> +    direction: rtl;
> 
> Why we need the direction of this to be rtl?

To make it possible to select the overall score and the individual test scores together. You'll notice that #results-score is nearest .score.

[Jon Lee]

Comment on attachment 281486 [details]
2. Detect 3 canvas sizes

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

>> PerformanceTests/Animometer/index.html:45
>> +                    <div class="score" onclick="benchmarkController.showDebugInfo()"></div>
> 
> How does the user know that clicking the score will show the debugInfo?

It's a power user feature. I need to move it because it makes it impossible to select just the score. But I'll do that in another patch.

[Said Abou-Hallawa]

Comment on attachment 281487 [details]
3. Show test confidence intervals, fix selection issues

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

Unofficial r=me.

> PerformanceTests/Animometer/index.html:7
> +    <title>MotionMark 1.0</title>

Do we have a plan to change the directory name also: PerformanceTests/Animometer/ -> PerformanceTests/MotionMark?

> PerformanceTests/Animometer/index.html:16
>      <script src="resources/runner/animometer.js" defer></script>

Should we change all the sources anemometer.* -> motionmark.*?

> PerformanceTests/Animometer/resources/debug-runner/animometer.css:403
> +#results .table-container > div {

I found it difficult to relate these css rules to this hierarchy of elements:

<section id="results">
    <div class="body">
        <div id="results-tables">
            <div>
                <table id="results-score"></table>
                <table id="results-data"></table>
            </div>
            <table id="results-header"></table>
        </div>
    </div>
</section>

For example tables exist only inside #results-tables but they are always referenced as "#results ...".

> PerformanceTests/Animometer/resources/debug-runner/animometer.css:407
> +#results #results-score {

Do we need to include "#results" here? Can't it be just #results-score? In other places I see it referenced like this: document.querySelector("#results-score").

> PerformanceTests/Animometer/resources/runner/animometer.js:273
> +                td.innerHTML = header.text(testResult, testName);

The header.text() function takes a single parameter. right?

[Jon Lee]

Comment on attachment 281487 [details]
3. Show test confidence intervals, fix selection issues

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

>> PerformanceTests/Animometer/resources/debug-runner/animometer.css:407
>> +#results #results-score {
> 
> Do we need to include "#results" here? Can't it be just #results-score? In other places I see it referenced like this: document.querySelector("#results-score").

"#results table" will override any rules declared there.

>> PerformanceTests/Animometer/resources/runner/animometer.js:273
>> +                td.innerHTML = header.text(testResult, testName);
> 
> The header.text() function takes a single parameter. right?

Fixed.

[Said Abou-Hallawa]

Comment on attachment 281488 [details]
4. Update serialization format to reduce size and make it extensible

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

Unofficial r=me.

> PerformanceTests/Animometer/resources/extensions.js:163
> +            return Number(number.toFixed(3));

Should not this be:
return Number(number.toFixed(precision));

> PerformanceTests/Animometer/resources/extensions.js:632
> +    createDatum: function()
> +    {
> +        return [];
> +    },

What is the need of this function?
Should not we add a function to either append or insert a datum in this.data instead?

> PerformanceTests/Animometer/resources/extensions.js:634
> +    getFieldInDatum: function(datum, fieldName)

Do we need "get" in the name? The class has a function named "at()" not "getAt()".
Also should not this function take an index for the datum and not the datum itself?

Instead of having this sequence of calls:
   var data = new SampleData(map, []);
   var datum = data.createDatum();
   data.setFieldInDatum(datum, "field1");
   data.setFieldInDatum(datum, "field2");
   data.push(datum);

We can do it this way:
   var data = new SampleData(map, []);
   data.appendDatum();
   data.setFieldInDatum(data.length() - 1 , "field1");
   data.setFieldInDatum(data.length() - 1 , "field2");
   data.push(datum);

> PerformanceTests/Animometer/tests/resources/main.js:154
> +        complexityAverageSamples.addField(Strings.json.complexity, 0);
> +        complexityAverageSamples.addField(Strings.json.frameLength, 1);
> +        complexityAverageSamples.addField(Strings.json.measurements.stdev, 2);

Should not this map be static somewhere instead of repeating the same set of calls?

> PerformanceTests/Animometer/tests/resources/main.js:219
> +            controllerSamples.setFieldInDatum(sample, Strings.json.time, timestamp - this._startTimestamp);
> +            controllerSamples.setFieldInDatum(sample, Strings.json.complexity, samples[1][i]);

sample is added to two SampleData but it is modified by the first one even the effect will be in both. Should not we add a class to the SampleDatum itself?

[Said Abou-Hallawa]

Comment on attachment 281489 [details]
5. Refine ramp lower bound evaluation, lengthen test

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

Unofficial r=me.

> PerformanceTests/Animometer/tests/resources/main.js:498
> +            progress = (timestamp - this._rampStartTimestamp) / (this._currentRampLength - this.intervalSamplingLength);

Can (this._currentRampLength - this.intervalSamplingLength) be zero?

> PerformanceTests/Animometer/tests/resources/main.js:512
>          if (interpolatedFrameLength < this.frameLengthRampLowerThreshold)
>              this._possibleMaximumComplexity = Math.floor(Utilities.lerp(Utilities.progressValue(this.frameLengthRampLowerThreshold, interpolatedFrameLength, this._lastTierFrameLength), this._maximumComplexity, this._lastTierComplexity));
> +        // If the regression doesn't fit the first segment at all, keep the minimum bound at 1
> +        if ((timestamp - this._sampler.samples[0][this._sampler.sampleCount - regression.n1]) / this._currentRampLength < .25)
> +            this._possibleMinimumComplexity = 1;

Should not write these two if-staments written as if (c2) ... else if (c1) ...
Also what is .25? Can we use constant to explain its meaning?

> PerformanceTests/ChangeLog:9
> +        the interval to 150 ms during sampling. Improve the estimation of the ramp parameters.

Did you mean 120ms?

[Said Abou-Hallawa]

Comment on attachment 281490 [details]
6. Refine ramp upper bound evaluation, let ramp profiles dictate final calculation

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

Unofficial r=me.

> PerformanceTests/Animometer/resources/runner/animometer.js:84
> +            var regressionOptions = { desiredFrameLength: desiredFrameLength };
> +            if (profile)
> +                regressionOptions.preferredProfile = profile;

Can't we write these statement like this:

var regressionOptions = { desiredFrameLength: desiredFrameLength,  preferredProfile: profile };

since preferredProfile will be undefined if profile is undefined anyway.

> PerformanceTests/Animometer/resources/statistics.js:307
> +            var error2 = (k2 + a2*s2*s2 + c2*t2*t2 - 2*d2*s2 - 2*h2*t2 + 2*b2*s2*t2) || Number.MAX_VALUE;

I think making the error equal to Number.MAX_VALUE is better than making it equal to zero. zero means we are suppressing the error as if it did not happen. At the end of this loop, we have the following if-statemnt:

    if (error1 + error2 < error1_best + error2_best)
        setBest(s1, t1, error1, s2, t2, error2, i, x_prime, x);
 
If error1 and error2 are both set to Number.MAX_VALUE, their sum will be Number.Infinity which should work as expected in the above if statement. Number.Infinity can also be used for the rejected regression calculation since Number.Infinity + Number.Infinity is NaN and Nan can't be less than any number.

> PerformanceTests/Animometer/tests/resources/main.js:524
> +            this._maximumComplexity = Math.round(this._minimumComplexity +
> +                Math.max(5,
> +                    this._possibleMaximumComplexity - this._minimumComplexity,
> +                    (this._changePointEstimator.mean() - this._minimumComplexity) * 2));

What is this statement doing? what is 5? I think we need a comment here.

> PerformanceTests/Animometer/tests/resources/main.js:612
> +    frameLengthTierThreshold: 1000/20,

Should not frameLengthTierThreshold = 1000/15? We have it 1000/30 for the RampController.

[Said Abou-Hallawa]

Comment on attachment 281491 [details]
7. performance.now fallback

Unofficial r=me.

[Said Abou-Hallawa]

Comment on attachment 281492 [details]
8. Confidence interval in final score

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

Unofficial r=me.

> PerformanceTests/Animometer/resources/runner/animometer.js:168
> +            var bootstrapResult = Regression.bootstrap(complexitySamples.data, 2500, function(resampleData) {

Why do we choose iterationCount = 2500? Can't we use a constant for this value to make the code more readable?

> PerformanceTests/Animometer/resources/runner/animometer.js:172
> +                resampleData.sort(function(a, b) {
> +                    return a[complexityIndex] - b[complexityIndex];
> +                });

Why do we need to make 2500 array sorting for the resampleData? resampleData is supposed to be a random set of complexitySamples.data. Can we sort the array only once and then choose the elements randomly such that are sorted?

[Said Abou-Hallawa]

Comment on attachment 281494 [details]
9. Test updates

Unofficial r=me.

[Jon Lee]

Comment on attachment 281488 [details]
4. Update serialization format to reduce size and make it extensible

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

>> PerformanceTests/Animometer/resources/extensions.js:163
>> +            return Number(number.toFixed(3));
> 
> Should not this be:
> return Number(number.toFixed(precision));

Whoops! Yes!

>> PerformanceTests/Animometer/resources/extensions.js:632
>> +    },
> 
> What is the need of this function?
> Should not we add a function to either append or insert a datum in this.data instead?

I can refactor to make it less awkward.

>> PerformanceTests/Animometer/resources/extensions.js:634
>> +    getFieldInDatum: function(datum, fieldName)
> 
> Do we need "get" in the name? The class has a function named "at()" not "getAt()".
> Also should not this function take an index for the datum and not the datum itself?
> 
> Instead of having this sequence of calls:
>    var data = new SampleData(map, []);
>    var datum = data.createDatum();
>    data.setFieldInDatum(datum, "field1");
>    data.setFieldInDatum(datum, "field2");
>    data.push(datum);
> 
> We can do it this way:
>    var data = new SampleData(map, []);
>    data.appendDatum();
>    data.setFieldInDatum(data.length() - 1 , "field1");
>    data.setFieldInDatum(data.length() - 1 , "field2");
>    data.push(datum);

This function can handle both the index and the datum object. Creating the datum and then referencing it by index instead is indirect.

>> PerformanceTests/Animometer/tests/resources/main.js:154
>> +        complexityAverageSamples.addField(Strings.json.measurements.stdev, 2);
> 
> Should not this map be static somewhere instead of repeating the same set of calls?

I'd rather now, since that forces exposing how SampleData manages the field map.

>> PerformanceTests/Animometer/tests/resources/main.js:219
>> +            controllerSamples.setFieldInDatum(sample, Strings.json.complexity, samples[1][i]);
> 
> sample is added to two SampleData but it is modified by the first one even the effect will be in both. Should not we add a class to the SampleDatum itself?

That was intentional, I can add a comment to that effect. My concern with having SampleDatum be a class is that it will make processing results slower, but I hadn't tried it out. That was also the intent of having the createDatum() method.

[Jon Lee]

Comment on attachment 281489 [details]
5. Refine ramp lower bound evaluation, lengthen test

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

>> PerformanceTests/Animometer/tests/resources/main.js:498
>> +            progress = (timestamp - this._rampStartTimestamp) / (this._currentRampLength - this.intervalSamplingLength);
> 
> Can (this._currentRampLength - this.intervalSamplingLength) be zero?

Generally no. this._currentRampLength represents the time left after a warmup period for the ramp (since we do a large jump from low to high # of particles). There is a minuscule possibility that the warmup time of one frame is so bad that it takes up 2.88 seconds to render, and that would cause the denominator to be 0.

>> PerformanceTests/Animometer/tests/resources/main.js:512
>> +            this._possibleMinimumComplexity = 1;
> 
> Should not write these two if-staments written as if (c2) ... else if (c1) ...
> Also what is .25? Can we use constant to explain its meaning?

No. It's possible for both to be adjusted in the same interval. Sure I'll make it a constant.

>> PerformanceTests/ChangeLog:9
>> +        the interval to 150 ms during sampling. Improve the estimation of the ramp parameters.
> 
> Did you mean 120ms?

Yes. I'll correct it.

[Jon Lee]

Comment on attachment 281490 [details]
6. Refine ramp upper bound evaluation, let ramp profiles dictate final calculation

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

>> PerformanceTests/Animometer/tests/resources/main.js:524
>> +                    (this._changePointEstimator.mean() - this._minimumComplexity) * 2));
> 
> What is this statement doing? what is 5? I think we need a comment here.

I'll add a comment.

>> PerformanceTests/Animometer/tests/resources/main.js:612
>> +    frameLengthTierThreshold: 1000/20,
> 
> Should not frameLengthTierThreshold = 1000/15? We have it 1000/30 for the RampController.

It might be better to remove this controller, actually.

[Jon Lee]

Comment on attachment 281492 [details]
8. Confidence interval in final score

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

>> PerformanceTests/Animometer/resources/runner/animometer.js:168
>> +            var bootstrapResult = Regression.bootstrap(complexitySamples.data, 2500, function(resampleData) {
> 
> Why do we choose iterationCount = 2500? Can't we use a constant for this value to make the code more readable?

I'll make it a constant.

>> PerformanceTests/Animometer/resources/runner/animometer.js:172
>> +                });
> 
> Why do we need to make 2500 array sorting for the resampleData? resampleData is supposed to be a random set of complexitySamples.data. Can we sort the array only once and then choose the elements randomly such that are sorted?

No. resampleData is a re-sample with replacement, so points can be duplicated. This is done many times, and each time the array can be different, so each needs to be sorted.

[Jon Lee]

Created attachment 281799 [details]
Patch with feedback

Patch with feedback incorporated

[Jon Lee]

Created attachment 281800 [details]
Harness updates

[Jon Lee]

Created attachment 281801 [details]
Test updates

[Jon Lee]

Committed r202314: <http://trac.webkit.org/changeset/202314>

[Jon Lee]

Committed r202315: <http://trac.webkit.org/changeset/202315>