Bug 149053

Summary: Add a graphics benchmark
Product: WebKit Reporter: Said Abou-Hallawa <sabouhallawa>
Component: AnimationsAssignee: Said Abou-Hallawa <sabouhallawa>
Status: RESOLVED FIXED    
Severity: Normal CC: barraclough, commit-queue, dbates, dino, fpizlo, ggaren, jonlee, rniwa, sam, simon.fraser, webkit-bug-importer, zalan
Priority: P2 Keywords: InRadar
Version: WebKit Nightly Build   
Hardware: Unspecified   
OS: Unspecified   
Bug Depends on: 149691, 150052, 150053, 150054, 150055, 150059, 150060, 150066, 150067, 150071, 150073, 150075, 150076, 150078    
Bug Blocks:    
Attachments:
Description Flags
Patch
none
Patch
none
Patch
none
Patch
none
Patch
none
Patch
none
results-worst-5-percent
none
Patch
none
results-individual-scores
none
Patch
none
Patch
none
[shared]
dino: review+
[benchmark]
none
[stage]
none
[bouncing-css-particles]
none
[bouncing-canvas-particles]
none
[bouncing-svg-particles]
none
[text]
none
[template]
none
[runner]
none
Patch
none
Patch
none
Patch
none
Patch
none
Patch
none
Patch none

Description Said Abou-Hallawa 2015-09-10 18:24:12 PDT
The goals are:

-- Can be publicly released.
-- Can track its results with the Perf Monitor.
-- Can be expanded to include new tests easily.
Comment 1 Said Abou-Hallawa 2015-09-11 17:28:40 PDT
Created attachment 261037 [details]
Patch
Comment 2 Said Abou-Hallawa 2015-09-16 13:46:45 PDT
Created attachment 261323 [details]
Patch
Comment 3 Said Abou-Hallawa 2015-09-16 13:51:40 PDT
Added options to
1. Fix the test particles after the warmup period.
2. Use the measured FPS instead of using the Kalman's filtered FPS.

Use the Student's t distribution to calculate the confidence interval delta for the sampled data at 95% confidence level.
Comment 4 Simon Fraser (smfr) 2015-09-16 14:40:04 PDT
Comment on attachment 261323 [details]
Patch

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

> PerformanceTests/ChangeLog:45
> +        * Animometer/bouncing-canvas-images.html: Added.
> +        * Animometer/bouncing-canvas-shapes.html: Added.
> +        * Animometer/bouncing-images.html: Added.
> +        * Animometer/bouncing-shapes.html: Added.
> +        * Animometer/bouncing-svg-images.html: Added.
> +        * Animometer/bouncing-svg-shapes.html: Added.

I think the tests should go into their own subdirectory, and be as self-contained as possible. We might end up with multiple sets of tests eventually.

> PerformanceTests/Animometer/bouncing-canvas-images.html:12
> +  <script src="resources/main.js"></script>
> +  <script src="resources/statistics.js"></script>

I don't think tests should pull in main.js.

You may want a utilities.js for tests. I don't know why they need statistics.js.

> PerformanceTests/Animometer/bouncing-svg-images.html:4
> +  <link rel="stylesheet" href="resources/benchmark.css">

I don't think tests should pull in any CSS from the harness.

> PerformanceTests/Animometer/resources/benchmark.js:63
> +        var currentTime = new Date().getTime();

performance.now()?

> PerformanceTests/Animometer/resources/benchmark.js:134
> +Benchmark.prototype = {
> +    parseParameters : function() {
> +        var query = window.location.search.substr(1);
> +        var result = {};
> +        query.split("&").forEach(function(part) {
> +            var item = part.split("=");
> +            result[item[0]] = decodeURIComponent(item[1]);
> +        });
> +      return result;
> +    },
> +    // Called from the load event listener or from this.run()

I think this should go into its own file. Separate "harness" code from "benchmark" code.

> PerformanceTests/Animometer/resources/bouncing-canvas-images.js:9
> +function BouncingCanvasImage(stage) {
> +    BouncingParticle.call(this, stage);
> +    this._context = stage.context;
> +    this._imageElement = stage.imageElement;
> +}
> +
> +BouncingCanvasImage.prototype = Object.create(BouncingParticle.prototype);
> +BouncingCanvasImage.prototype.constructor = BouncingCanvasImage;
> +

Each of these files should go into a directory along with its html and resources.
Comment 5 Said Abou-Hallawa 2015-09-18 20:53:14 PDT
Created attachment 261567 [details]
Patch
Comment 6 Said Abou-Hallawa 2015-09-18 21:24:13 PDT
Comment on attachment 261323 [details]
Patch

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

>> PerformanceTests/Animometer/bouncing-canvas-images.html:12
>> +  <script src="resources/statistics.js"></script>
> 
> I don't think tests should pull in main.js.
> 
> You may want a utilities.js for tests. I don't know why they need statistics.js.

I restructured the test in the following hierarchy:

resources/  (includes the shared resources and scripts between the runner and the tests)
runner/ (includes the test harness and its resources)
   animometer.html (the  test harness page)
   resources/  (includes the resources and scripts which are used by the  test harness)
tests/
   resources/ (includes the shared resources and the scripts which are needed to run the benchmark for every individual test)
   bouncing-particles/ (includes all the bouncing particles based test pages)
      resources/ (includes all the scripts which are used by the bouncing particles based test pages)
   text/ (includes all the text based test pages)
      resources/ (includes all the scripts which are used by the text based test pages)

>> PerformanceTests/Animometer/bouncing-svg-images.html:4
>> +  <link rel="stylesheet" href="resources/benchmark.css">
> 
> I don't think tests should pull in any CSS from the harness.

There two css files right now:

Animometer/tests/resources/benchmark.css (which is used by all the tests)
Animometer/runner/resources/animometer.css (which us used by the test harness)

>> PerformanceTests/Animometer/resources/benchmark.js:63
>> +        var currentTime = new Date().getTime();
> 
> performance.now()?

Done.

>> PerformanceTests/Animometer/resources/benchmark.js:134
>> +    // Called from the load event listener or from this.run()
> 
> I think this should go into its own file. Separate "harness" code from "benchmark" code.

Done. It is moved to Animometer/tests/resources/utilities.js.

>> PerformanceTests/Animometer/resources/bouncing-canvas-images.js:9
>> +
> 
> Each of these files should go into a directory along with its html and resources.

All the bouncing particles based tests are now in the directory Animometer/tests/bouncing-particles.
Comment 7 Said Abou-Hallawa 2015-09-18 21:25:28 PDT
Created attachment 261571 [details]
Patch
Comment 8 Said Abou-Hallawa 2015-09-21 11:26:29 PDT
Created attachment 261666 [details]
Patch
Comment 9 Said Abou-Hallawa 2015-09-21 11:27:53 PDT
(In reply to comment #8)
> Created attachment 261666 [details]
> Patch

This patch forces repainting all the text in the layering-text.html test.
Comment 10 Said Abou-Hallawa 2015-09-22 17:27:22 PDT
Created attachment 261783 [details]
Patch
Comment 11 Said Abou-Hallawa 2015-09-22 17:30:20 PDT
Created attachment 261784 [details]
results-worst-5-percent
Comment 12 Said Abou-Hallawa 2015-09-22 17:36:56 PDT
(In reply to comment #10)
> Created attachment 261783 [details]
> Patch

This patch include the worst 5% measurement. It shows how smooth the test is running. The bigger the difference between its value and the average is, the more jagged the rendering will be. The final score should be penalized by this how big this difference is for every individual test.
Comment 13 Said Abou-Hallawa 2015-09-23 16:56:28 PDT
Created attachment 261849 [details]
Patch
Comment 14 Said Abou-Hallawa 2015-09-23 17:01:21 PDT
Comment on attachment 261849 [details]
Patch

This patch adds a score column for every test. See the picture results-individual-scores.png. The test score calculation is:

test_score = geometric_mean([sample_mean, average_worst_5_percent]).

I fixed also the final score calculation to be:

final_score = geometric_mean(test_scores)
Comment 15 Said Abou-Hallawa 2015-09-23 17:03:18 PDT
Created attachment 261850 [details]
results-individual-scores
Comment 16 Said Abou-Hallawa 2015-09-23 18:14:30 PDT
Created attachment 261856 [details]
Patch
Comment 17 Said Abou-Hallawa 2015-09-23 18:35:21 PDT
Created attachment 261858 [details]
Patch
Comment 18 Dean Jackson 2015-09-25 18:10:11 PDT
Comment on attachment 261858 [details]
Patch

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

This patch is way too huge to review. I'm not sure what most things do. The best I can do is give some comments and rubber-stamp.

If you want to address the comments and then split it into smaller patches, then we can probably do a more careful review.

> PerformanceTests/Animometer/resources/algorithm.js:19
> +    _parentIndex : function(i) {
> +        return i > 0 ? Math.floor((i - 1) / 2) : -1;
> +    },
> +    _leftIndex : function(i) {

In general, WebKit's JS programming style is to not put a space between the symbol name and the definition. And to put the { on the next line.

_parentIndex: function(i)
{

},

Then a blank line to the next thing in the prototype.

(I won't repeat this comment, but it obviously applies to all files)

> PerformanceTests/Animometer/resources/algorithm.js:25
> +    // Returns the child index that may violate the heap property at index i

Remember to end comments with a .
(I won't repeat this everywhere)

> PerformanceTests/Animometer/resources/algorithm.js:39
> +        return !this._size ? NaN : this._values[0];

This would make more sense as
return this._size ? this._values[0] : NaN;

> PerformanceTests/Animometer/resources/main.js:47
> +    width : function() {
> +        return this.left + this.right;
> +    },
> +    height : function() {
> +        return this.top + this.bottom;
> +    }

You could make these getters.

get width()
{
  return this.left + this.right;
}

Then you can just use point.width rather than insets.width().

Same applies to the Point class, and other stuff in the patch.

> PerformanceTests/Animometer/resources/main.js:80
> +SimplePromise.prototype.then = function (callback) {
> +    if (this._callback)
> +        throw "SimplePromise doesn't support multiple calls to then";
> +    this._callback = callback;
> +    this._chainedPromise = new SimplePromise;
> +    
> +    if (this._resolved)
> +        this.resolve(this._resolvedValue);
> +
> +    return this._chainedPromise;
> +}
> +
> +SimplePromise.prototype.resolve = function (value) {
> +    if (!this._callback) {
> +        this._resolved = true;
> +        this._resolvedValue = value;
> +        return;
> +    }
> +
> +    var result = this._callback(value);
> +    if (result instanceof SimplePromise) {
> +        var chainedPromise = this._chainedPromise;
> +        result.then(function (result) { chainedPromise.resolve(result); });
> +    } else
> +        this._chainedPromise.resolve(result);
> +}

Could you use the system Promises if they exist?

> PerformanceTests/Animometer/resources/main.js:103
> +function ProgressBar(element, ranges) {
> +    this.element = element;
> +    this.ranges = ranges;
> +    this.currentRange = 0;
> +}
> +
> +ProgressBar.prototype = {
> +    _progressToPercent : function(progress) {
> +        return progress * (100 / this.ranges);
> +    },
> +    incRange : function() {
> +        ++this.currentRange;
> +    },
> +    setPos : function(progress) {
> +        this.element.style.width = this._progressToPercent(this.currentRange + progress) + "%";
> +    }
> +}

This is a pretty weird class.

It's not really a progress bar. It's something that sets the width of a specified element.

> PerformanceTests/Animometer/resources/main.js:115
> +        for (var title = 0; title < titles.length; ++title) {

You can write this as:

titles.forEach(function (title) {
  // no need to call titles[title] any more

});

> PerformanceTests/Animometer/resources/main.js:117
> +            th.innerHTML = titles[title].text;

Use textContent instead.
th.textContent = title.text;

> PerformanceTests/Animometer/resources/main.js:139
> +            for (var i = 0, len = queue.length; i < len; ++i) {

Seems like this could be a forEach too.

> PerformanceTests/Animometer/resources/main.js:156
> +                for (var i = 0; i < list.length; ++i)

And this.

> PerformanceTests/Animometer/resources/main.js:170
> +    _showSamples : function(row, testName, axises, samples, samplingTimeOffset) {

Should be axes not axises.

> PerformanceTests/Animometer/resources/main.js:176
> +            window.showGraph(testName, axises, samples, samplingTimeOffset);

Seems strange that you install showGraph on the global object. Isn't there a controller object of some kind?

> PerformanceTests/Animometer/resources/main.js:179
> +        button.textContent = "Click...";

Maybe the label should be "Show Graph", or "Graph"

> PerformanceTests/Animometer/resources/main.js:197
> +        for (var measure = 0; measure < sampler.experiments.length; ++measure) {
> +            this._showValue(row, testName, sampler.experiments[measure].mean());
> +            this._showValue(row, testName, sampler.experiments[measure].confidenceIntervalDelta(95));
> +            this._showValue(row, testName, sampler.experiments[measure].concern(5));
> +            this._showValue(row, testName, sampler.experiments[measure].standardDeviation());
> +            this._showValue(row, testName, sampler.experiments[measure].percentage());
> +            axises.push(titles[measure + 1].text);
> +        }

It's a shame you can't replace this with a forEach, because you use the loop counter to look up the title.

As for the delta and concern values, I suggest these be extracted out into some form of global constant at the top of the file.

this._showValue(row, testName, sampler.experiments[measure].concern(INITIAL_CONCERN));

This same comment applies in a few places.

> PerformanceTests/Animometer/resources/main.js:210
> +        for (var testName in suiteSamplers) {

Definitely use suiteSamplers.forEach(function (testName) {.... here.

You'll have to .bind(this), but I still think this is a better overall style.

> PerformanceTests/Animometer/resources/main.js:229
> +            for (var suiteName in iterationsSamplers[i]) {

And again.

> PerformanceTests/Animometer/runner/resources/animometer.css:16
> +main {
> +    display: block;
> +    position: absolute;
> +    width: 800px;
> +    height: 600px;
> +    top: 50%;
> +    left: 50%;
> +    margin-top: -321px;
> +    margin-left: -421px;
> +    padding: 15px;

Seems like you're doing this to center the main element in the page. Just use flexbox.

html, body {
  height: 100%;
}

body {
  display: flex;
  align-items: center;
  justify-content: center;
}

main {
  width: 800px;
  height: 600px;
}

> PerformanceTests/Animometer/runner/resources/animometer.js:105
> +    graph("#graphContainer", new Point(700, 400), new Insets(20, 50, 20, 50), axises, samples, samplingTimeOffset);

This code seems very dependent on the sizes given in the CSS file.

> PerformanceTests/Animometer/runner/resources/benchmark-runner.js:72
> +    frame.style.width = "800px";
> +    frame.style.height = "600px";
> +    frame.style.border = "0px none";
> +    frame.style.position = "absolute";

Same here. Why can't you do this in the CSS?

> PerformanceTests/Animometer/runner/resources/benchmark-runner.js:151
> +    var self = this;
> +    return state.prepareCurrentTest(this, this._frame).then(function(prepareReturnValue) {
> +        return self._runTestAndRecordResults(state);
> +    });

Use bind.

return state.prepareCurrentTest(this, this._frame).then(function(prepareReturnValue) {
   return this._runTestAndRecordResults(state);
 }.bind(this));

> PerformanceTests/Animometer/runner/resources/graph.js:3
> +    var element = element = document.querySelector(selector);

Did you mean to have that second element there?

> PerformanceTests/Animometer/runner/resources/graph.js:5
> +    while (element.firstChild)
> +        element.removeChild(element.firstChild);

Why not call element.textContent = null; ?

> PerformanceTests/Animometer/tests/bouncing-particles/resources/bouncing-canvas-images.js:40
> +    var imageSrc = options["imageSrc"] || "resources/yin-yang.svg";
> +    
> +    var imageElements = document.getElementsByClassName("hidden");
> +    console.assert(imageElements.length);
> +        
> +    for (var i = 0; i < imageElements.length; ++i) {
> +        if (imageElements[i].getAttribute("src") == imageSrc) {
> +            this.imageElement = imageElements[i];
> +            break;
> +        }                                   
> +    }

I think you could probably do this with a single document.querySelector that looks for .hidden and the attribute selector.

You also don't want the console.log.

> PerformanceTests/Animometer/tests/bouncing-particles/resources/bouncing-canvas-shapes.js:25
> +    default:
> +        this._context.fillStyle = this._color0;
> +        break;
> +

Don't put the default case first.

> PerformanceTests/Animometer/tests/bouncing-particles/resources/bouncing-canvas-shapes.js:142
> +window.benchmarkClient.create = function(suite, test, options, recordTable, progressBar) {
> +    window.benchmark = new BouncingCanvasShapesBenchmark(suite, test, options, recordTable, progressBar);
> +}

Again, I feel like it would be better to have a single global controller object, and everything else hanging off it.

> PerformanceTests/Animometer/tests/bouncing-particles/resources/bouncing-particles.js:94
> +    this.svgNamespace = "http://www.w3.org/2000/svg";
> +    this.xlinkNamespace = "http://www.w3.org/1999/xlink";

These should be globals.

The way you're doing it here means you're allocating the string for every instance.

> PerformanceTests/Animometer/tests/bouncing-particles/resources/bouncing-particles.js:116
> +    for(var i = 0; i < this._particles.length; ++i)

forEach opportunity

> PerformanceTests/Animometer/tests/bouncing-particles/resources/bouncing-shapes.js:10
> +    default:
> +        this.element.style.backgroundColor = stage.randomColor();
> +        break;

Don't put default at the start of the switch.

> PerformanceTests/Animometer/tests/bouncing-particles/resources/bouncing-svg-shapes.js:7
> +    default:

Another default first.

> PerformanceTests/Animometer/tests/bouncing-particles/resources/bouncing-svg-shapes.js:21
> +    switch (stage.clip) {
> +    case "star":
> +        stage.ensureClipStarIsCreated();    
> +        this.element.setAttribute("clip-path", "url(#star-clip)");
> +        break;
> +    }

There isn't a point having a switch with only one case and no default. Just make it a conditional.

> PerformanceTests/Animometer/tests/bouncing-particles/resources/bouncing-svg-shapes.js:98
> +    if (typeof this._gadientsCount === "undefined")
> +        this._gadientsCount = 0;    

Typo.

> PerformanceTests/Animometer/tests/bouncing-particles/resources/bouncing-svg-shapes.js:99
> +    gradient.setAttribute("id", "gadient-" + ++this._gadientsCount);

And here.

> PerformanceTests/Animometer/tests/resources/benchmark.js:65
> +        if (typeof this._referenceTime === "undefined")

In cases like this, I think it's ok to simply ask

if (!this._referenceTime)

You can probably initialise it to zero in the constructor to be even more safe.

> PerformanceTests/Animometer/tests/resources/math.js:131
> +function Controller(K, ysp, ulow, uhigh) {

This is a very generic name. What does it actually do?

> PerformanceTests/Animometer/tests/resources/utilities.js:7
> +            if (value[0] == '\'' )

I think you can simply do:

if (value[0] == "'")
Comment 19 Radar WebKit Bug Importer 2015-09-28 12:02:01 PDT
<rdar://problem/22882991>
Comment 20 Said Abou-Hallawa 2015-09-30 09:46:08 PDT
Created attachment 262169 [details]
[shared]
Comment 21 Said Abou-Hallawa 2015-09-30 09:47:34 PDT
Created attachment 262170 [details]
[benchmark]
Comment 22 Said Abou-Hallawa 2015-09-30 09:47:58 PDT
Created attachment 262171 [details]
[stage]
Comment 23 Said Abou-Hallawa 2015-09-30 09:48:40 PDT
Created attachment 262172 [details]
[bouncing-css-particles]
Comment 24 Said Abou-Hallawa 2015-09-30 09:49:06 PDT
Created attachment 262173 [details]
[bouncing-canvas-particles]
Comment 25 Said Abou-Hallawa 2015-09-30 09:49:26 PDT
Created attachment 262174 [details]
[bouncing-svg-particles]
Comment 26 Said Abou-Hallawa 2015-09-30 09:49:59 PDT
Created attachment 262175 [details]
[text]
Comment 27 Said Abou-Hallawa 2015-09-30 09:50:20 PDT
Created attachment 262176 [details]
[template]
Comment 28 Said Abou-Hallawa 2015-09-30 09:50:40 PDT
Created attachment 262177 [details]
[runner]
Comment 29 Said Abou-Hallawa 2015-09-30 09:52:09 PDT
Created attachment 262179 [details]
Patch
Comment 30 Simon Fraser (smfr) 2015-09-30 16:07:02 PDT
Comment on attachment 262175 [details]
[text]

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

> PerformanceTests/Animometer/tests/text/resources/layering-text.js:42
> +    "<div id='id_00' class='text-layer'>",
> +        "<h2 id='id_01'>Customized styles</h2>",
> +        "<p id='id_02'>If you want formatting choices that are not available from the built-in styles, Quick Style sets, and themes, you can create custom styles to suit your needs.</p>",
> +        "<p id='id_03'>The easiest way to create a custom style is to modify a built-in style and then save it as a new style.</p>",
> +        "<p id='id_04'>For example, you might want to format a paragraph of quoted material with a half-inch indent from the left and right margins, single spaced. There is no built-in style to accommodate this, but you can create a custom style by doing the following:</p>",
> +        "<ol type='1' id='id_05'>",
> +            "<li id='id_06'><p>Click in the paragraph you want to format.</p></li>",
> +            "<li id='id_07'><p>On the <b>Home</b> tab, click the <b>Paragraph</b> Dialog Box Launcher.</p></li>",
> +            "<li id='id_08'><p>In the <b>Indentation</b> section, type <b class='ocpLiteral'>0.5'</b> in the <b>Left</b> and <b>Right</b> boxes.</p></li>",
> +            "<li id='id_09'><p>In the <b>Spacing</b> section, in the <b>Line spacing</b> list, click <b>Single</b>.</p></li>",
> +            "<li id='id_10'><p>Click <b>OK</b>.</p></li>",
> +            "<li id='id_11'><p>Right-click in the paragraph, point to <b>Styles</b>, and then click <b>Save Selection as a New Quick Style</b>.</p></li>",
> +            "<li id='id_12'><p>In the <b>Name</b> box, type a name for the style, such as <b>Block quote</b>. </p></li>",
> +            "<li id='id_13'><p>If you want the style to be included in the gallery of styles on the <b>Home</b> tab, and if you want the style to be a linked style, click <b>OK</b>.</p></li>",
> +            "<li id='id_14'>",
> +                "<p id='id_15'>If you don't want the style to be included in the gallery, or if you want the style to be either a paragraph or a character style, click <b>Modify</b> and do one or both of the following:</p>",
> +                "<ul id='id_16'>",
> +                    "<li id='id_17'><p>At the bottom of the dialog box, clear the <b>Add to Quick Style</b> list box.</p></li>",
> +                    "<li id='id_18'><p>In the <b>Style type</b> list, click <b>Paragraph</b> or <b>Character</b>.</p></li>",
> +                "</ul>",
> +            "</li>",
> +        "</ol>",
> +        "<p id='id_19'>If you switch to a different Quick Style set, you may need to adjust the settings of your custom style. In the example here, if you create the Block quote style while the Word 2007 Quick Style set is applied, and then you switch to the Traditional Quick Style set, you can change the Block quote style to remove the first-line indentation that the Traditional Quick Style set introduces. To change a style, do the following:</p>",
> +        "<ol type='1' id='id_20'>",
> +            "<li id='id_21'><p>On the <b>Home</b> tab in the <b>Styles</b> group, right-click <b>Block quote</b>, and then click <b>Modify</b>.</p></li>",
> +            "<li id='id_22'><p>Click <b>Format</b>, and then click <b>Paragraph</b>.</p></li>",
> +            "<li id='id_23'><p>In the <b>Indentation</b> section, in the <b>Special</b> list, click <b>(none)</b>.</p></li>",
> +        "</ol>",
> +        "<p id='id_24'>The more characteristics that you specify for the style, the less the style is affected by switching Quick Style sets or themes.</p>",
> +    "</div>"

Can we use some text known to be free of copyright? Maybe Shakespeare? Are the id's important? This looks like an exported Word document.

> PerformanceTests/Animometer/tests/text/resources/layering-text.js:47
> +    var praseResult = {};

typo: praseResult

> PerformanceTests/Animometer/tests/text/resources/layering-text.js:53
> +    var spaceIndex = praseResult.tagStart.indexOf(" ");
> +    praseResult.nodeName = praseResult.tagStart.substring(1, spaceIndex != -1 ? spaceIndex : praseResult.tagStart.length - 1);
> +    praseResult.args = spaceIndex != -1 ? window.benchmarkClient.parseArguments(praseResult.tagStart.substring(spaceIndex + 1, praseResult.tagStart.length - 1)) : {};
> +    var tagEnd = "</" + praseResult.nodeName + ">";                
> +    praseResult.tagEnd = textItem.endsWith(tagEnd) ? tagEnd : "";

Can we do this with DOM and not parsing? Could make a document fragment.

> PerformanceTests/Animometer/tests/text/resources/layering-text.js:118
> +    window.benchmarkClient.extendObject(textItemFlags, LayeringTextStage.textItemsFlags[this._textItemIndex]);

Odd to be calling into window.benchmarkClient here.

> PerformanceTests/Animometer/tests/text/resources/layering-text.js:126
> +    window.benchmarkClient.extendObject(textItemFlags, LayeringTextStage.textItemsFlags[this._textItemIndex]);    

And here.
Comment 31 Simon Fraser (smfr) 2015-09-30 16:40:49 PDT
Comment on attachment 262176 [details]
[template]

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

> PerformanceTests/Animometer/tests/template/template-canvas.html:7
> +    <script src="../../resources/main.js"></script>

Should refactor to not pull main.js into tests.
Comment 32 Simon Fraser (smfr) 2015-09-30 16:45:06 PDT
Comment on attachment 262172 [details]
[bouncing-css-particles]

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

> PerformanceTests/Animometer/tests/bouncing-particles/resources/bouncing-css-images.js:45
> +    if (particle.element && particle.element.parentNode && particle.element.parentNode == this.element)
> +        this.element.removeChild(particle.element);

Can this just be particle.element.remove()?

> PerformanceTests/Animometer/tests/bouncing-particles/resources/bouncing-css-shapes.js:63
> +    if (particle.element && particle.element.parentNode && particle.element.parentNode == this.element)
> +        this.element.removeChild(particle.element);

.remove()?
Comment 33 Simon Fraser (smfr) 2015-09-30 16:50:45 PDT
Comment on attachment 262173 [details]
[bouncing-canvas-particles]

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

> PerformanceTests/Animometer/tests/bouncing-particles/resources/bouncing-canvas-images.js:14
> +    this._applyRotation();
> +    this._context.drawImage(this._imageElement, 0, 0, this._size.x, this._size.y);

I think you should push and pop the context state around these lines.

> PerformanceTests/Animometer/tests/bouncing-particles/resources/bouncing-canvas-images.js:48
> +    window.benchmark = new BouncingCanvasImagesBenchmark(suite, test, options, recordTable, progressBar);

Odd to set window.benchmark. Why not return the benchmark?

> PerformanceTests/Animometer/tests/bouncing-particles/resources/bouncing-canvas-shapes.js:55
> +    this._applyFill();
> +    this._applyRotation();
> +    this._applyClipping();
> +    this._drawShape();

You should push/pop around these too.

> PerformanceTests/Animometer/tests/bouncing-particles/resources/bouncing-canvas-shapes.js:87
> +    window.benchmark = new BouncingCanvasShapesBenchmark(suite, test, options, recordTable, progressBar);

Return it?
Comment 34 Ryosuke Niwa 2015-09-30 16:54:09 PDT
Can we not review/land multiple patches on a single bug please?  It's extremely confusing to follow.
Comment 35 Dean Jackson 2015-09-30 17:07:27 PDT
Comment on attachment 262169 [details]
[shared]

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

> PerformanceTests/Animometer/resources/algorithm.js:32
> +    _parentIndex: function(i)
> +    {
> +        return i > 0 ? Math.floor((i - 1) / 2) : -1;
> +    },
> +    
> +    _leftIndex: function(i)
> +    {
> +        return i * 2 + 1 < this._size ? i * 2 + 1 : -1;
> +    },
> +    
> +    _rightIndex: function(i)
> +    {
> +        return i * 2 + 2 < this._size ? i * 2 + 2 : -1;
> +    },

These expressions are a bit hard to parse in my head. Could you add () to make it more clear?

> PerformanceTests/Animometer/resources/algorithm.js:44
> +    {
> +        var l = this._leftIndex(i);
> +        var r = this._rightIndex(i);
> +
> +        if (l != -1 && r != -1)
> +            return this._compare(this._values[l], this._values[r]) > 0 ? l : r;
> +        
> +        return l != -1 ? l : r;
> +    },

Use left and right as variable names because l looks a lot like 1 :)

> PerformanceTests/Animometer/resources/algorithm.js:80
> +        // Fixes the heap poperty at index i given that parent is the only node that

Typo: property

> PerformanceTests/Animometer/resources/algorithm.js:92
> +        // Fixes the heap poperty at index i given that each of the left and the right

Same.

> PerformanceTests/Animometer/resources/algorithm.js:121
> +var Algorithm =
> +{

Strangely, I think for these we do it all on one line.

> PerformanceTests/Animometer/resources/main.js:9
> +    // Used when the point object is used as a size object

Remember punctuation.

> PerformanceTests/Animometer/resources/main.js:15
> +    // Used when the point object is used as a size object    

Ditto.

> PerformanceTests/Animometer/resources/main.js:24
> +    get center()
> +    {
> +        return new Point(this.x / 2, this.y / 2);
> +    },

It's weird that a Point can have a center, so you should comment that this happens when treating a Point as a Size.
Comment 36 Dean Jackson 2015-09-30 17:26:51 PDT
Comment on attachment 262170 [details]
[benchmark]

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

> PerformanceTests/Animometer/tests/resources/benchmark.js:12
> +    messages: [ "Warming up", "Sampling", "Finished" ]

You should put a comment here to say that it must be in the same order/index as the keywords above.

> PerformanceTests/Animometer/tests/resources/math.js:229
> +        // Current error
> +        var e = this._ysp - y;
> +
> +        // Proportional term
> +        var P = this._Kp * e;
> +        
> +        // Derivative term is the slope of the curve at the current time
> +        var D = this._Kd * (e - this._eold) / h;
> +
> +        // Integral term is the area under the curve starting from the begining till the current time
> +        this._I += this._Ki * ((e + this._eold) / 2) * h;
> +
> +        // pid controller value
> +        var v = P + D + this._I;
> +        
> +        // Avoid spikes by applying actuator satuartion
> +        var u = this._sat(v, this._ulow, this._uhigh);        

Punctuation. Plus a typo in saturation.

> PerformanceTests/Animometer/tests/resources/math.js:271
> +        // Update the transition matrix
> +        this._matA[Matrix3.pos(0, 2)] = 1;
> +
> +        // Predicted state and covariance
> +        var vecX_prd = Matrix3.multiplyVector3(this._matA, this._vecX_est);
> +        var matP_prd = Matrix3.add(Matrix3.multiplyMatrix3(Matrix3.multiplyMatrix3(this._matA, this._matP_est), Matrix3.transpose(this._matA)), this._matQ);
> +
> +        // Estimation
> +        var vecB = Vector3.multiplyMatrix3(this._vecH, Matrix3.transpose(matP_prd));
> +        var S = Vector3.multiplyVector3(vecB, this._vecH) + this._R;
> +
> +        var vecGain = Vector3.scale(1/S, vecB);
> +        
> +        // Estimated state and covariance
> +        this._vecX_est = Vector3.add(vecX_prd, Vector3.scale(current - Vector3.multiplyVector3(this._vecH, vecX_prd), vecGain));
> +        this._matP_est = Matrix3.subtract(matP_prd, Matrix3.scale(Vector3.multiplyVector3(vecGain, this._vecH), matP_prd));
> +
> +        // Compute the estimated measurement

Punctuation.
Comment 37 Dean Jackson 2015-09-30 17:33:25 PDT
Comment on attachment 262174 [details]
[bouncing-svg-particles]

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

> PerformanceTests/Animometer/tests/bouncing-particles/resources/bouncing-svg-images.js:7
> +    var attrs = { 'x': 0, 'y': 0, 'width': this._size.x, 'height': this._size.y };
> +    var xlinkAttrs = { 'href': stage.imageSrc };

You don't need the '' around the keys here.

> PerformanceTests/Animometer/tests/bouncing-particles/resources/bouncing-svg-shapes.js:21
> +        var attrs = { 'x': 0, 'y': 0, 'width': this._size.x, 'height': this._size.y };

Or here.

> PerformanceTests/Animometer/tests/bouncing-particles/resources/bouncing-svg-shapes.js:27
> +        var attrs = { 'cx': this._size.x / 2, 'cy': this._size.y / 2, 'r': Math.min(this._size.x, this._size.y) / 2 };

Or here.
Comment 38 Said Abou-Hallawa 2015-09-30 18:15:43 PDT
Comment on attachment 262169 [details]
[shared]

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

>> PerformanceTests/Animometer/resources/algorithm.js:32
>> +    },
> 
> These expressions are a bit hard to parse in my head. Could you add () to make it more clear?

Done.

>> PerformanceTests/Animometer/resources/algorithm.js:44
>> +    },
> 
> Use left and right as variable names because l looks a lot like 1 :)

Done.

>> PerformanceTests/Animometer/resources/algorithm.js:80
>> +        // Fixes the heap poperty at index i given that parent is the only node that
> 
> Typo: property

Done.

>> PerformanceTests/Animometer/resources/algorithm.js:92
>> +        // Fixes the heap poperty at index i given that each of the left and the right
> 
> Same.

Done.

>> PerformanceTests/Animometer/resources/algorithm.js:121
>> +{
> 
> Strangely, I think for these we do it all on one line.

Done.

>> PerformanceTests/Animometer/resources/main.js:9
>> +    // Used when the point object is used as a size object
> 
> Remember punctuation.

Done.

>> PerformanceTests/Animometer/resources/main.js:15
>> +    // Used when the point object is used as a size object    
> 
> Ditto.

Done.

>> PerformanceTests/Animometer/resources/main.js:24
>> +    },
> 
> It's weird that a Point can have a center, so you should comment that this happens when treating a Point as a Size.

Done.

This patch was moved to https://bugs.webkit.org/show_bug.cgi?id=149691.
Comment 39 Said Abou-Hallawa 2015-10-01 14:20:24 PDT
Created attachment 262284 [details]
Patch
Comment 40 Said Abou-Hallawa 2015-10-01 14:22:31 PDT
Comment on attachment 262170 [details]
[benchmark]

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

>> PerformanceTests/Animometer/tests/resources/math.js:229
>> +        var u = this._sat(v, this._ulow, this._uhigh);        
> 
> Punctuation. Plus a typo in saturation.

Done.

>> PerformanceTests/Animometer/tests/resources/math.js:271
>> +        // Compute the estimated measurement
> 
> Punctuation.

Done.
Comment 41 Said Abou-Hallawa 2015-10-01 14:23:37 PDT
Comment on attachment 262172 [details]
[bouncing-css-particles]

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

>> PerformanceTests/Animometer/tests/bouncing-particles/resources/bouncing-css-images.js:45
>> +        this.element.removeChild(particle.element);
> 
> Can this just be particle.element.remove()?

Done.

>> PerformanceTests/Animometer/tests/bouncing-particles/resources/bouncing-css-shapes.js:63
>> +        this.element.removeChild(particle.element);
> 
> .remove()?

Done.
Comment 42 Said Abou-Hallawa 2015-10-01 14:25:19 PDT
Comment on attachment 262173 [details]
[bouncing-canvas-particles]

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

>> PerformanceTests/Animometer/tests/bouncing-particles/resources/bouncing-canvas-images.js:48
>> +    window.benchmark = new BouncingCanvasImagesBenchmark(suite, test, options, recordTable, progressBar);
> 
> Odd to set window.benchmark. Why not return the benchmark?

Done. It now return new BouncingCanvasImagesBenchmark and the caller sets the return value to window.benchmark.

>> PerformanceTests/Animometer/tests/bouncing-particles/resources/bouncing-canvas-shapes.js:55
>> +    this._drawShape();
> 
> You should push/pop around these too.

Done.

>> PerformanceTests/Animometer/tests/bouncing-particles/resources/bouncing-canvas-shapes.js:87
>> +    window.benchmark = new BouncingCanvasShapesBenchmark(suite, test, options, recordTable, progressBar);
> 
> Return it?

Done. It now return new BouncingCanvasShapesBenchmark and the caller sets the return value to window.benchmark.
Comment 43 Said Abou-Hallawa 2015-10-01 14:27:55 PDT
Comment on attachment 262174 [details]
[bouncing-svg-particles]

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

>> PerformanceTests/Animometer/tests/bouncing-particles/resources/bouncing-svg-images.js:7
>> +    var xlinkAttrs = { 'href': stage.imageSrc };
> 
> You don't need the '' around the keys here.

All ' ' were removed expect for 'stop-color' in BouncingSvgShapesStage.prototype.createGradient.

>> PerformanceTests/Animometer/tests/bouncing-particles/resources/bouncing-svg-shapes.js:21
>> +        var attrs = { 'x': 0, 'y': 0, 'width': this._size.x, 'height': this._size.y };
> 
> Or here.

Done.

>> PerformanceTests/Animometer/tests/bouncing-particles/resources/bouncing-svg-shapes.js:27
>> +        var attrs = { 'cx': this._size.x / 2, 'cy': this._size.y / 2, 'r': Math.min(this._size.x, this._size.y) / 2 };
> 
> Or here.

Done.
Comment 44 Said Abou-Hallawa 2015-10-01 14:30:32 PDT
Comment on attachment 262175 [details]
[text]

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

>> PerformanceTests/Animometer/tests/text/resources/layering-text.js:42
>> +    "</div>"
> 
> Can we use some text known to be free of copyright? Maybe Shakespeare? Are the id's important? This looks like an exported Word document.

Done. I got some text from https://en.wikipedia.org/wiki/Benchmark_(computing).

>> PerformanceTests/Animometer/tests/text/resources/layering-text.js:47
>> +    var praseResult = {};
> 
> typo: praseResult

Fixed.

>> PerformanceTests/Animometer/tests/text/resources/layering-text.js:53
>> +    praseResult.tagEnd = textItem.endsWith(tagEnd) ? tagEnd : "";
> 
> Can we do this with DOM and not parsing? Could make a document fragment.

Will do that later.

>> PerformanceTests/Animometer/tests/text/resources/layering-text.js:118
>> +    window.benchmarkClient.extendObject(textItemFlags, LayeringTextStage.textItemsFlags[this._textItemIndex]);
> 
> Odd to be calling into window.benchmarkClient here.

Done. extendObject() was moved to the Utilities object.

>> PerformanceTests/Animometer/tests/text/resources/layering-text.js:126
>> +    window.benchmarkClient.extendObject(textItemFlags, LayeringTextStage.textItemsFlags[this._textItemIndex]);    
> 
> And here.

Done. extendObject() was moved to the Utilities object.
Comment 45 Said Abou-Hallawa 2015-10-01 14:34:17 PDT
Comment on attachment 262176 [details]
[template]

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

>> PerformanceTests/Animometer/tests/template/template-canvas.html:7
>> +    <script src="../../resources/main.js"></script>
> 
> Should refactor to not pull main.js into tests.

Done. The shared objects like Point, Insets, etc were moved to a new file called Animometer/resources/extensions.js. The Animator, the Benchmark and running the benchmark itself are in Animometer/tests/resources/main.js.
Comment 46 Said Abou-Hallawa 2015-10-01 17:48:48 PDT
Created attachment 262304 [details]
Patch
Comment 47 Said Abou-Hallawa 2015-10-01 17:52:16 PDT
Now there three bugs with three separate patches:

https://bugs.webkit.org/show_bug.cgi?id=149691: has shared code between the runner and the test benchmark.
This bug: has the benchmark and the tests.
https://bugs.webkit.org/show_bug.cgi?id=149683: has the benchmark runner.

They can be landed separately.
Comment 48 Said Abou-Hallawa 2015-10-02 17:16:56 PDT
Created attachment 262368 [details]
Patch
Comment 49 Said Abou-Hallawa 2015-10-02 17:26:21 PDT
Added two examples for other animation techniques: 

tests/examples/canvas-electrons.html
tests/examples/canvas-stars.html
Comment 50 Said Abou-Hallawa 2015-10-02 17:37:16 PDT
Created attachment 262369 [details]
Patch
Comment 51 Said Abou-Hallawa 2015-10-02 17:38:36 PDT
The references to the statistics.js is removed. The confidence interval delta measurement is removed from the results table.
Comment 52 Ryosuke Niwa 2015-10-02 17:49:35 PDT
Comment on attachment 262369 [details]
Patch

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

> PerformanceTests/ChangeLog:296
> +        * Animometer/tests/resources/yin-yang.png: Added.
> +        * Animometer/tests/resources/yin-yang.svg: Added.
> +        These images are shared among all the tests.

Did you make these png images?

If not, are they BSD-licensed? Depending on the terms of their licenses, we may still need to include the credits.

> PerformanceTests/Animometer/tests/resources/stage.css:2
> +  height: 100%;

Wrong indentation. Use 4 spaces instead.

> PerformanceTests/Animometer/tests/resources/yin-yang.svg:2
> +  <defs>

Again, wrong indentation.
Use 4 spaces instead.

> PerformanceTests/Animometer/tests/template/resources/template-canvas.js:6
> +    // TODO: Fill in your object data.

We don't use TODO.
This since is a template, I don't think FIXME is appropriate either.
Just remove TODO: everywhere in this file.

> PerformanceTests/Animometer/tests/template/resources/template-css.js:15
> +    // TODO: Change objects in the stage.

Ditto about removing TODO:s in this file.

> PerformanceTests/Animometer/tests/template/resources/template-svg.js:15
> +    // TODO: Change objects in the stage.

Ditto.
Comment 53 Said Abou-Hallawa 2015-10-02 20:35:48 PDT
Created attachment 262378 [details]
Patch
Comment 54 Said Abou-Hallawa 2015-10-02 20:40:55 PDT
Comment on attachment 262369 [details]
Patch

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

>> PerformanceTests/ChangeLog:296
>> +        These images are shared among all the tests.
> 
> Did you make these png images?
> 
> If not, are they BSD-licensed? Depending on the terms of their licenses, we may still need to include the credits.

Yes. I made the SVG by hand and then took a screenshot from it and save it as PNG.

>> PerformanceTests/Animometer/tests/resources/stage.css:2
>> +  height: 100%;
> 
> Wrong indentation. Use 4 spaces instead.

Done.

>> PerformanceTests/Animometer/tests/resources/yin-yang.svg:2
>> +  <defs>
> 
> Again, wrong indentation.
> Use 4 spaces instead.

Done.

>> PerformanceTests/Animometer/tests/template/resources/template-canvas.js:6
>> +    // TODO: Fill in your object data.
> 
> We don't use TODO.
> This since is a template, I don't think FIXME is appropriate either.
> Just remove TODO: everywhere in this file.

Done.

>> PerformanceTests/Animometer/tests/template/resources/template-css.js:15
>> +    // TODO: Change objects in the stage.
> 
> Ditto about removing TODO:s in this file.

Done.

>> PerformanceTests/Animometer/tests/template/resources/template-svg.js:15
>> +    // TODO: Change objects in the stage.
> 
> Ditto.

Done.
Comment 55 Dean Jackson 2015-10-05 12:51:08 PDT
Comment on attachment 262378 [details]
Patch

rs=me, based on previous reviews, and that rniwa's comments were addressed.
Comment 56 WebKit Commit Bot 2015-10-05 13:39:22 PDT
Comment on attachment 262378 [details]
Patch

Clearing flags on attachment: 262378

Committed r190575: <http://trac.webkit.org/changeset/190575>
Comment 57 WebKit Commit Bot 2015-10-05 13:39:31 PDT
All reviewed patches have been landed.  Closing bug.