Update how the benchmark is run
Created attachment 270815 [details] Patch
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.
Unofficial r=me
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.
Comment on attachment 270815 [details] Patch committed 196288