Bug 154063

Summary: Add new benchmark tests
Product: WebKit Reporter: Jon Lee <jonlee>
Component: WebKit Misc.Assignee: Jon Lee <jonlee>
Status: RESOLVED FIXED    
Severity: Normal CC: cdumez, commit-queue, rniwa, sabouhallawa, simon.fraser
Priority: P2    
Version: WebKit Nightly Build   
Hardware: Unspecified   
OS: Unspecified   
Attachments:
Description Flags
Patch none

Description Jon Lee 2016-02-10 00:42:49 PST
Add new benchmark tests
Comment 1 Jon Lee 2016-02-10 01:07:06 PST
Created attachment 270982 [details]
Patch
Comment 2 Jon Lee 2016-02-10 12:24:22 PST
submitted 196372
Comment 3 Jon Lee 2016-02-10 12:26:10 PST
oops, wrong bug
Comment 4 Said Abou-Hallawa 2016-02-10 16:20:55 PST
Comment on attachment 270982 [details]
Patch

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

> PerformanceTests/Animometer/tests/master/focus.html:21
> +    }

#center-text is already added to stage.css by this patch with the same set of style rules. I think deleting this one here should not affect the appearance of the test.

> PerformanceTests/Animometer/tests/master/multiply.html:39
> +    }

It took me awhile to figure out where these styles rules are used. Maybe it was my problem not noticing the string concatenation in the js file to compose the class name. So I think it is fine to have them without a comment.

> PerformanceTests/Animometer/tests/master/resources/focus.js:37
> +        this._centerElement.style.zIndex = Math.round(10 * this.centerObjectDepth);

Should not these settings be moved to the <style> section of focus.html?

> PerformanceTests/Animometer/tests/master/resources/focus.js:40
> +        this._setPrefixedStyles(this._centerElement, 'filter', 'blur(' + blur + 'px)');

You can now use Utilities.setElementPrefixedProperty(this._centerElement, 'filter', 'blur(' + blur + 'px)');

> PerformanceTests/Animometer/tests/master/resources/focus.js:72
> +    _createTestElement: function()

The parent of the element can be passed and this function becomes responsible of connecting the new child to its parent.

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

If we create a separate class for the particle object, this code can be moved in the class constructor.

> PerformanceTests/Animometer/tests/master/resources/focus.js:124
> +            obj.element.style.transform = 'translateX(' + left + '%) translateY(' + top + '%)';

Should not we create a separate class for the obj and have this animation code live inside the class animate() function?

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

You do not need this function if you use Utilities.setElementPrefixedProperty()

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

Should not this function be moved to Utilities class in extensions.js?

> PerformanceTests/Animometer/tests/master/resources/image-data.js:36
> +        }.bind(this));

Can't we load the image in a hidden element in the html file like what we do in bouncing-canvas-images.html?

<img class="hidden" src="../resources/yin-yang.png">

> PerformanceTests/Animometer/tests/master/resources/image-data.js:83
> +        };

I do not think we have to create a separate object wrapping this element since all it contains is the element and the 2d context which can be retrieved at painting time. If we do that we get rid of this.testElements also and have all the elements stored stored in the children list of the stage element.

> PerformanceTests/Animometer/tests/master/resources/image-data.js:116
> +                didDraw = true;

I think we can delete this flag and replace the if-statemnet below by:

if (!dataLen)

> PerformanceTests/Animometer/tests/master/resources/image-data.js:124
> +                obj.context.drawImage(this.testImage, 0, 0, this.testImageWidth, this.testImageHeight)

Should not this if-statemnet be moved before the loop above and continue after it redraws.

> PerformanceTests/Animometer/tests/master/resources/image-data.js:147
> +    },

Should not the whole function be moved to Utilities in main.js?

> PerformanceTests/Animometer/tests/master/resources/multiply.js:31
> +                nextIndex += Math.floor(spiralCounter / 2);

Can't we use:

nextIndex += spiralCounter >> 1;

> PerformanceTests/Animometer/tests/master/resources/multiply.js:43
> +        centerSpiralCount = maxSide * Math.round(maxSide / 2) * 2;

I do not understand why we do this math to get the smallest even digit >= maxSide. But I think this can be written like also this this:

centerSpiralCount = maxSide * (maxSide + (maxSide & 1));

> PerformanceTests/Animometer/tests/master/resources/multiply.js:58
> +        tile.className = 'div-' + Stage.randomInt(0,6);

var tile = Utilities.createElement("div", { class: 'div-' + Stage.randomInt(0,6) }, this.element);

> PerformanceTests/Animometer/tests/master/resources/multiply.js:64
> +        var distance = 1.5 / tileSize * Math.sqrt(Math.pow(this.size.height / 2 - y - halfTileSize, 2) + Math.pow(this.size.width / 2 - x - halfTileSize, 2));

var distance = 1.5 / tileSize * this.size.mutiply(0.5).subtract(new Point(x - halfTileSize, y - halfTileSize)).length();

> PerformanceTests/Animometer/tests/master/resources/multiply.js:70
> +            distance: distance,

step and distance seems to be the same for all tiles. So why do we have to repeat them with each tile?

> PerformanceTests/Animometer/tests/master/resources/multiply.js:88
> +        var time = Date.now();

This variable is not used.

> PerformanceTests/Animometer/tests/master/resources/multiply.js:91
> +        var progress = this._benchmark.timestamp % 10000 / 10000;

Why do we use 10000 here? Do we want to repeat the animation every 10 seconds? Or are we assuming the test time is only 10 seconds?

> PerformanceTests/Animometer/tests/master/resources/multiply.js:110
> +        }

This loop can be written like this:

for (var i = 0; i < this._offsetIndex; ++i) {
    var tile = this.tiles[i];
    tile.active = true;
    tile.rotate += tile.step;
    tile.element.style.transform = "rotate(" + tile.rotate + "deg)";

    var influence = 1 - (tile.distance * distanceFactor);
    tile.element.style.backgroundColor = hslPrefix + l * Math.tan(influence / 1.25) + "%," + influence + ")";
}

for (var i = this._offsetIndex; i < this.tiles.length && this.tiles[i].active; ++i) {
    this.tiles[i].active = false;
    this.tiles[i].element.style.backgroundColor = "";
}

> PerformanceTests/Animometer/tests/master/resources/multiply.js:116
> +    }

This function is also defined in focus.js.

> PerformanceTests/Animometer/tests/master/resources/stage.css:29
> +    font-weight: 200;

I think the css rules in this file should be generic. So for #center-text I think it is fine to include only the properties that makes the text center. But I think properties like color, font-size and font-weight should be moved to the specific tests rules.  For example I may write a new test and add these in the html file:

<link rel="stylesheet" type="text/css" href="resources/stage.css">
#center-text {
    color: red;
    font-size: 6em;
    font-weight: 400;
}

And if the test does not include rules for #center-text, then the #centerr-text will get the default values.

> PerformanceTests/Animometer/tests/resources/main.js:886
> +        var stage = window.getComputedStyle(document.getElementById("stage"));

Should not this variable be renamed "style" or "stageStyle" instead of "stage"?
Comment 5 Said Abou-Hallawa 2016-02-10 16:29:45 PST
Unofficial r=me.
Comment 6 Jon Lee 2016-02-10 21:28:05 PST
Comment on attachment 270982 [details]
Patch

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

>> PerformanceTests/Animometer/tests/master/focus.html:21
>> +    }
> 
> #center-text is already added to stage.css by this patch with the same set of style rules. I think deleting this one here should not affect the appearance of the test.

whoops. removed.

>> PerformanceTests/Animometer/tests/master/resources/focus.js:37
>> +        this._centerElement.style.zIndex = Math.round(10 * this.centerObjectDepth);
> 
> Should not these settings be moved to the <style> section of focus.html?

No. They are used throughout the test and affect the visuals so I think we should keep them in script in case we want to tweak later.

>> PerformanceTests/Animometer/tests/master/resources/focus.js:40
>> +        this._setPrefixedStyles(this._centerElement, 'filter', 'blur(' + blur + 'px)');
> 
> You can now use Utilities.setElementPrefixedProperty(this._centerElement, 'filter', 'blur(' + blur + 'px)');

yes, i'll update.

>> PerformanceTests/Animometer/tests/master/resources/focus.js:72
>> +    _createTestElement: function()
> 
> The parent of the element can be passed and this function becomes responsible of connecting the new child to its parent.

But then the addition and removal of the element are located in two separate functions, making it harder to understand how the class works.

>> PerformanceTests/Animometer/tests/master/resources/focus.js:103
>> +            }
> 
> If we create a separate class for the particle object, this code can be moved in the class constructor.

ok

>> PerformanceTests/Animometer/tests/master/resources/focus.js:124
>> +            obj.element.style.transform = 'translateX(' + left + '%) translateY(' + top + '%)';
> 
> Should not we create a separate class for the obj and have this animation code live inside the class animate() function?

ok.

>> PerformanceTests/Animometer/tests/master/resources/focus.js:147
>> +    },
> 
> You do not need this function if you use Utilities.setElementPrefixedProperty()

done.

>> PerformanceTests/Animometer/tests/master/resources/focus.js:159
>> +    }
> 
> Should not this function be moved to Utilities class in extensions.js?

this and _getprogress probably could.

>> PerformanceTests/Animometer/tests/master/resources/image-data.js:36
>> +        }.bind(this));
> 
> Can't we load the image in a hidden element in the html file like what we do in bouncing-canvas-images.html?
> 
> <img class="hidden" src="../resources/yin-yang.png">

I think this guarantees we receive the load event because we hook to it before we set the image source.

>> PerformanceTests/Animometer/tests/master/resources/image-data.js:83
>> +        };
> 
> I do not think we have to create a separate object wrapping this element since all it contains is the element and the 2d context which can be retrieved at painting time. If we do that we get rid of this.testElements also and have all the elements stored stored in the children list of the stage element.

sure.

>> PerformanceTests/Animometer/tests/master/resources/image-data.js:116
>> +                didDraw = true;
> 
> I think we can delete this flag and replace the if-statemnet below by:
> 
> if (!dataLen)

when is dataLen == 0? this flag is only set if at least one pixel in the canvas is opaque.

>> PerformanceTests/Animometer/tests/master/resources/image-data.js:124
>> +                obj.context.drawImage(this.testImage, 0, 0, this.testImageWidth, this.testImageHeight)
> 
> Should not this if-statemnet be moved before the loop above and continue after it redraws.

no. depending on the length of the animation we might have cleared out the entire canvas, and we need to repopulate the pixels.

>> PerformanceTests/Animometer/tests/master/resources/image-data.js:147
>> +    },
> 
> Should not the whole function be moved to Utilities in main.js?

This contributes to the visual effect specific to this test, so I think this doesn't need to move out.

>> PerformanceTests/Animometer/tests/master/resources/multiply.js:31
>> +                nextIndex += Math.floor(spiralCounter / 2);
> 
> Can't we use:
> 
> nextIndex += spiralCounter >> 1;

sure

>> PerformanceTests/Animometer/tests/master/resources/multiply.js:43
>> +        centerSpiralCount = maxSide * Math.round(maxSide / 2) * 2;
> 
> I do not understand why we do this math to get the smallest even digit >= maxSide. But I think this can be written like also this this:
> 
> centerSpiralCount = maxSide * (maxSide + (maxSide & 1));

This create a spiral of tiles from the center. The stage is much wider than its height, so we can't continue spiraling. instead we fill in the columns on either side of the square spiral.

>> PerformanceTests/Animometer/tests/master/resources/multiply.js:58
>> +        tile.className = 'div-' + Stage.randomInt(0,6);
> 
> var tile = Utilities.createElement("div", { class: 'div-' + Stage.randomInt(0,6) }, this.element);

ok

>> PerformanceTests/Animometer/tests/master/resources/multiply.js:64
>> +        var distance = 1.5 / tileSize * Math.sqrt(Math.pow(this.size.height / 2 - y - halfTileSize, 2) + Math.pow(this.size.width / 2 - x - halfTileSize, 2));
> 
> var distance = 1.5 / tileSize * this.size.mutiply(0.5).subtract(new Point(x - halfTileSize, y - halfTileSize)).length();

x + halfTimeSize, but sure.

>> PerformanceTests/Animometer/tests/master/resources/multiply.js:70
>> +            distance: distance,
> 
> step and distance seems to be the same for all tiles. So why do we have to repeat them with each tile?

x and y are not the same per tile.

>> PerformanceTests/Animometer/tests/master/resources/multiply.js:88
>> +        var time = Date.now();
> 
> This variable is not used.

removed.

>> PerformanceTests/Animometer/tests/master/resources/multiply.js:91
>> +        var progress = this._benchmark.timestamp % 10000 / 10000;
> 
> Why do we use 10000 here? Do we want to repeat the animation every 10 seconds? Or are we assuming the test time is only 10 seconds?

yes, repeat the animation. this just alters the color.

>> PerformanceTests/Animometer/tests/master/resources/multiply.js:110
>> +        }
> 
> This loop can be written like this:
> 
> for (var i = 0; i < this._offsetIndex; ++i) {
>     var tile = this.tiles[i];
>     tile.active = true;
>     tile.rotate += tile.step;
>     tile.element.style.transform = "rotate(" + tile.rotate + "deg)";
> 
>     var influence = 1 - (tile.distance * distanceFactor);
>     tile.element.style.backgroundColor = hslPrefix + l * Math.tan(influence / 1.25) + "%," + influence + ")";
> }
> 
> for (var i = this._offsetIndex; i < this.tiles.length && this.tiles[i].active; ++i) {
>     this.tiles[i].active = false;
>     this.tiles[i].element.style.backgroundColor = "";
> }

nice.

>> PerformanceTests/Animometer/tests/master/resources/multiply.js:116
>> +    }
> 
> This function is also defined in focus.js.

yup, will refactor out.

>> PerformanceTests/Animometer/tests/master/resources/stage.css:29
>> +    font-weight: 200;
> 
> I think the css rules in this file should be generic. So for #center-text I think it is fine to include only the properties that makes the text center. But I think properties like color, font-size and font-weight should be moved to the specific tests rules.  For example I may write a new test and add these in the html file:
> 
> <link rel="stylesheet" type="text/css" href="resources/stage.css">
> #center-text {
>     color: red;
>     font-size: 6em;
>     font-weight: 400;
> }
> 
> And if the test does not include rules for #center-text, then the #centerr-text will get the default values.

sure. i'll move it.

>> PerformanceTests/Animometer/tests/resources/main.js:886
>> +        var stage = window.getComputedStyle(document.getElementById("stage"));
> 
> Should not this variable be renamed "style" or "stageStyle" instead of "stage"?

yes you're right.
Comment 7 Jon Lee 2016-02-10 23:42:25 PST
committed r196415