Bug 155475 – Add a basic WebGL test to Animometer

• Component: New Bugs
• Status: RESOLVED FIXED

[Dean Jackson]

Add a basic WebGL test to Animometer

[Dean Jackson]

Created attachment 274055 [details]
Patch

[Radar WebKit Bug Importer]

<rdar://problem/25156860>

[Radar WebKit Bug Importer]

<rdar://problem/25156869>

[Jon Lee]

Comment on attachment 274055 [details]
Patch

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

Nice!

> PerformanceTests/Animometer/tests/3d/resources/webgl.js:51
> +].join("\n");

Is there a benefit to doing it this way instead of just putting it in the .html?

> PerformanceTests/Animometer/tests/3d/resources/webgl.js:64
> +            this._numTriangles = 1000;

This should be initialized to 0.

> PerformanceTests/Animometer/tests/3d/resources/webgl.js:150
> +                this._uniformData[i * 6 + 0] = Stage.random(0.2, 0.4);

Instead of doing this calculation, maintain a separate variable that just increments as it travels along the buffer?

> PerformanceTests/Animometer/tests/3d/resources/webgl.js:160
> +        {

You should include an early exit if count == 0.

> PerformanceTests/Animometer/tests/3d/resources/webgl.js:161
> +            this._numTriangles += count * 10;

Did the ramp controller stop too early, and is why you need to scale everything by 10? This shouldn't be necessary.

> PerformanceTests/Animometer/tests/3d/resources/webgl.js:164
> +            this._reset();

Might be better to try to follow the logic of some of the other tests which maintain a separate index, to avoid having to recreate the uniform data every time the controller tunes.

> PerformanceTests/Animometer/tests/3d/resources/webgl.js:175
> +            var elapsed = (Date.now() - this._startTime) / 1000;

There is Stage.dateCounterValue(1000)

> PerformanceTests/Animometer/tests/3d/resources/webgl.js:181
> +                gl.uniform1f(this._uScale, this._uniformData[i * 6 + 0]);

Instead of doing this calculation, maintain a separate variable that just increments as it travels along the buffer?

> PerformanceTests/Animometer/tests/3d/resources/webgl.js:194
> +                gl.drawArrays(gl.TRIANGLES, 0, 3);

Would it be more realistic to just do this all in one go instead of drawing each triangle separately?

> PerformanceTests/Animometer/tests/3d/resources/webgl.js:201
> +            return this._numTriangles / 10;

Seems better to track the actual number.

[Dean Jackson]

Created attachment 274450 [details]
Patch

[Dean Jackson]

Comment on attachment 274055 [details]
Patch

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

>> PerformanceTests/Animometer/tests/3d/resources/webgl.js:51
>> +].join("\n");
> 
> Is there a benefit to doing it this way instead of just putting it in the .html?

I moved this into a <script> tag.

>> PerformanceTests/Animometer/tests/3d/resources/webgl.js:64
>> +            this._numTriangles = 1000;
> 
> This should be initialized to 0.

Fixed.

>> PerformanceTests/Animometer/tests/3d/resources/webgl.js:160
>> +        {
> 
> You should include an early exit if count == 0.

Done.

>> PerformanceTests/Animometer/tests/3d/resources/webgl.js:161
>> +            this._numTriangles += count * 10;
> 
> Did the ramp controller stop too early, and is why you need to scale everything by 10? This shouldn't be necessary.

No, this was just my misunderstanding. Fixed.

>> PerformanceTests/Animometer/tests/3d/resources/webgl.js:164
>> +            this._reset();
> 
> Might be better to try to follow the logic of some of the other tests which maintain a separate index, to avoid having to recreate the uniform data every time the controller tunes.

I've done this.

>> PerformanceTests/Animometer/tests/3d/resources/webgl.js:175
>> +            var elapsed = (Date.now() - this._startTime) / 1000;
> 
> There is Stage.dateCounterValue(1000)

Done.

>> PerformanceTests/Animometer/tests/3d/resources/webgl.js:181
>> +                gl.uniform1f(this._uScale, this._uniformData[i * 6 + 0]);
> 
> Instead of doing this calculation, maintain a separate variable that just increments as it travels along the buffer?

Done.

>> PerformanceTests/Animometer/tests/3d/resources/webgl.js:194
>> +                gl.drawArrays(gl.TRIANGLES, 0, 3);
> 
> Would it be more realistic to just do this all in one go instead of drawing each triangle separately?

Yes. But I'm trying to test raw throughput. I'll add another test that draws everything at once.

>> PerformanceTests/Animometer/tests/3d/resources/webgl.js:201
>> +            return this._numTriangles / 10;
> 
> Seems better to track the actual number.

Fixed.

[Simon Fraser (smfr)]

Comment on attachment 274450 [details]
Patch

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

> PerformanceTests/Animometer/tests/3d/resources/webgl.js:38
> +

Blank line here and not above.

> PerformanceTests/Animometer/tests/3d/resources/webgl.js:80
> +                0, 0.1, 0, 1,
> +                -0.1, -0.1, 0, 1,
> +                0.1, -0.1, 0, 1

I would line these up with spaces.

> PerformanceTests/Animometer/tests/3d/resources/webgl.js:101
> +            var scriptElement = document.getElementById(id);
> +            return scriptElement.text;

No need for scriptElement

[Dean Jackson]

Committed r198442: <http://trac.webkit.org/changeset/198442>