Bug 153960 – Update how the benchmark is run

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

[Jon Lee]

Update how the benchmark is run

[Jon Lee]

Created attachment 270815 [details]
Patch

[Said Abou-Hallawa]

Comment on attachment 270815 [details]
Patch

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

> PerformanceTests/Animometer/resources/runner/benchmark-runner.js:99
> +            samplers[test.name] = results;

Should we change the array name: samplers => sampleResults (or something similar)?

> PerformanceTests/Animometer/tests/resources/main.js:37
> +        return timestamp > this._endTimestamp;

Shouldn't the condition be timestamp >= this._endTimestamp? Any extra work, like changing the complexity, skipping layout frames or drawing will exceed the test time limit.

> PerformanceTests/Animometer/tests/resources/main.js:42
> +        return this._sampler.process();

Should not the function be renamed processResults() or something else? It is hard to see the relationship between 'results' and 'sampler.process'.

> PerformanceTests/Animometer/tests/resources/main.js:62
> +                time: timestamp - this._startTimestamp,

Shouldn't time => timeDelta, intervalLength,  or duration?

> PerformanceTests/Animometer/tests/resources/main.js:142
> +        if (!this._startedSampling && timestamp > this._samplingTimestamp) {

Why do we have to skip this statement for the first time when timestamp == this._samplingTimestamp?

> PerformanceTests/Animometer/tests/resources/main.js:374
> +            this._stage.animate(0);

I do not understand this. Why we have to animate the stage with timeDelta=0 for 100 ms?

> PerformanceTests/Animometer/tests/resources/main.js:380
> +        if (this._controller.shouldStop(this._currentTimestamp)) {

Shouldn't _controller.update() come after checking _controller.shouldStop()?

> PerformanceTests/ChangeLog:59
> +                The previous version of this controller ignored the frame that came after the

Unneeded spaces.

> PerformanceTests/ChangeLog:82
> +        the estimation error converges, so calculate the gain ahead of time.

This is great change. Thanks for taking the time for investigating the math and simplifying the code.

[Said Abou-Hallawa]

Unofficial r=me

[Jon Lee]

Comment on attachment 270815 [details]
Patch

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

>> PerformanceTests/Animometer/resources/runner/benchmark-runner.js:99
>> +            samplers[test.name] = results;
> 
> Should we change the array name: samplers => sampleResults (or something similar)?

Sure. I'll do that in a follow-up.

>> PerformanceTests/Animometer/tests/resources/main.js:37
>> +        return timestamp > this._endTimestamp;
> 
> Shouldn't the condition be timestamp >= this._endTimestamp? Any extra work, like changing the complexity, skipping layout frames or drawing will exceed the test time limit.

In _animateLoop, if I move the update() call to after shouldStop(), then I need to make sure that this last frame has a chance to process and get sampled. Otherwise some controllers like the ramp will not get the last ramp sampled.

>> PerformanceTests/Animometer/tests/resources/main.js:42
>> +        return this._sampler.process();
> 
> Should not the function be renamed processResults() or something else? It is hard to see the relationship between 'results' and 'sampler.process'.

Done. I went with processSamples().

>> PerformanceTests/Animometer/tests/resources/main.js:62
>> +                time: timestamp - this._startTimestamp,
> 
> Shouldn't time => timeDelta, intervalLength,  or duration?

Here, I'm offsetting all of the absolute timestamps by the start timestamp so that the first sample is at time 0.

>> PerformanceTests/Animometer/tests/resources/main.js:142
>> +        if (!this._startedSampling && timestamp > this._samplingTimestamp) {
> 
> Why do we have to skip this statement for the first time when timestamp == this._samplingTimestamp?

You're right. Updated to timestamp >= this._samplingTimestamp.

>> PerformanceTests/Animometer/tests/resources/main.js:374
>> +            this._stage.animate(0);
> 
> I do not understand this. Why we have to animate the stage with timeDelta=0 for 100 ms?

I want to warm up the scene by mimicking the animation loop we will take once the test starts. I need to keep this._previousTImestamp where it is so we know when 100 ms is passed. I was ok with keeping the scene still during this time period, otherwise I'd have to keep track of another timestamp to properly pass the frame length.

>> PerformanceTests/Animometer/tests/resources/main.js:380
>> +        if (this._controller.shouldStop(this._currentTimestamp)) {
> 
> Shouldn't _controller.update() come after checking _controller.shouldStop()?

I can move it to afterwards.

[Jon Lee]

Comment on attachment 270815 [details]
Patch

committed 196288