Bug 157738 - Improvements to Animometer benchmark
Summary: Improvements to Animometer benchmark
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebKit Misc. (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Jon Lee
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2016-05-15 22:48 PDT by Jon Lee
Modified: 2016-06-21 20:12 PDT (History)
7 users (show)

See Also:


Attachments
1. CSS cleanup (34.58 KB, patch)
2016-06-16 15:40 PDT, Jon Lee
no flags Details | Formatted Diff | Diff
2. Detect 3 canvas sizes (9.06 KB, patch)
2016-06-16 15:41 PDT, Jon Lee
no flags Details | Formatted Diff | Diff
3. Show test confidence intervals, fix selection issues (16.91 KB, patch)
2016-06-16 15:43 PDT, Jon Lee
no flags Details | Formatted Diff | Diff
4. Update serialization format to reduce size and make it extensible (27.21 KB, patch)
2016-06-16 15:44 PDT, Jon Lee
no flags Details | Formatted Diff | Diff
5. Refine ramp lower bound evaluation, lengthen test (9.52 KB, patch)
2016-06-16 15:44 PDT, Jon Lee
no flags Details | Formatted Diff | Diff
6. Refine ramp upper bound evaluation, let ramp profiles dictate final calculation (22.98 KB, patch)
2016-06-16 15:45 PDT, Jon Lee
no flags Details | Formatted Diff | Diff
7. performance.now fallback (2.88 KB, patch)
2016-06-16 15:45 PDT, Jon Lee
no flags Details | Formatted Diff | Diff
8. Confidence interval in final score (24.17 KB, patch)
2016-06-16 15:46 PDT, Jon Lee
no flags Details | Formatted Diff | Diff
9. Test updates (8.28 KB, patch)
2016-06-16 15:46 PDT, Jon Lee
no flags Details | Formatted Diff | Diff
Complete patch (125.94 KB, patch)
2016-06-16 15:49 PDT, Jon Lee
no flags Details | Formatted Diff | Diff
Patch with feedback (128.25 KB, patch)
2016-06-21 17:52 PDT, Jon Lee
no flags Details | Formatted Diff | Diff
Harness updates (119.94 KB, patch)
2016-06-21 18:01 PDT, Jon Lee
dino: review+
Details | Formatted Diff | Diff
Test updates (8.44 KB, patch)
2016-06-21 18:01 PDT, Jon Lee
dino: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Jon Lee 2016-05-15 22:48:44 PDT
Improvements to Animometer benchmark
Comment 1 Jon Lee 2016-06-16 15:40:44 PDT
Created attachment 281484 [details]
1. CSS cleanup
Comment 2 Jon Lee 2016-06-16 15:41:56 PDT
Created attachment 281486 [details]
2. Detect 3 canvas sizes
Comment 3 Jon Lee 2016-06-16 15:43:19 PDT
Created attachment 281487 [details]
3. Show test confidence intervals, fix selection issues
Comment 4 Jon Lee 2016-06-16 15:44:00 PDT
Created attachment 281488 [details]
4. Update serialization format to reduce size and make it extensible
Comment 5 Jon Lee 2016-06-16 15:44:37 PDT
Created attachment 281489 [details]
5. Refine ramp lower bound evaluation, lengthen test
Comment 6 Jon Lee 2016-06-16 15:45:19 PDT
Created attachment 281490 [details]
6. Refine ramp upper bound evaluation, let ramp profiles dictate final calculation
Comment 7 Jon Lee 2016-06-16 15:45:34 PDT
Created attachment 281491 [details]
7. performance.now fallback
Comment 8 Jon Lee 2016-06-16 15:46:04 PDT
Created attachment 281492 [details]
8. Confidence interval in final score
Comment 9 Jon Lee 2016-06-16 15:46:57 PDT
Created attachment 281494 [details]
9. Test updates
Comment 10 Jon Lee 2016-06-16 15:49:55 PDT
Created attachment 281495 [details]
Complete patch
Comment 11 Said Abou-Hallawa 2016-06-16 19:48:00 PDT
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?
Comment 12 Said Abou-Hallawa 2016-06-16 20:04:32 PDT
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?
Comment 13 Jon Lee 2016-06-16 23:41:54 PDT
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.
Comment 14 Jon Lee 2016-06-16 23:43:21 PDT
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.
Comment 15 Said Abou-Hallawa 2016-06-17 11:04:23 PDT
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?
Comment 16 Jon Lee 2016-06-17 11:52:14 PDT
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.
Comment 17 Said Abou-Hallawa 2016-06-17 12:04:45 PDT
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?
Comment 18 Said Abou-Hallawa 2016-06-17 12:34:10 PDT
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?
Comment 19 Said Abou-Hallawa 2016-06-17 15:15:37 PDT
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.
Comment 20 Said Abou-Hallawa 2016-06-17 15:16:50 PDT
Comment on attachment 281491 [details]
7. performance.now fallback

Unofficial r=me.
Comment 21 Said Abou-Hallawa 2016-06-17 15:51:27 PDT
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?
Comment 22 Said Abou-Hallawa 2016-06-17 15:58:37 PDT
Comment on attachment 281494 [details]
9. Test updates

Unofficial r=me.
Comment 23 Jon Lee 2016-06-19 15:30:05 PDT
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.
Comment 24 Jon Lee 2016-06-19 15:38:31 PDT
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.
Comment 25 Jon Lee 2016-06-19 23:31:05 PDT
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.
Comment 26 Jon Lee 2016-06-19 23:34:26 PDT
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.
Comment 27 Jon Lee 2016-06-21 17:52:28 PDT
Created attachment 281799 [details]
Patch with feedback

Patch with feedback incorporated
Comment 28 Jon Lee 2016-06-21 18:01:34 PDT
Created attachment 281800 [details]
Harness updates
Comment 29 Jon Lee 2016-06-21 18:01:59 PDT
Created attachment 281801 [details]
Test updates
Comment 30 Jon Lee 2016-06-21 20:11:34 PDT
Committed r202314: <http://trac.webkit.org/changeset/202314>
Comment 31 Jon Lee 2016-06-21 20:12:42 PDT
Committed r202315: <http://trac.webkit.org/changeset/202315>