Bug 95668

Created attachment 161954 [details] Patch

Comment on attachment 161954 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=161954&action=review > PerformanceTests/ChangeLog:9 > + Test function for the runAsync and runPerSecondAsync should invoke Do we really need both runAsync and runPerSecondAsync?

(In reply to comment #2) > (From update of attachment 161954 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=161954&action=review > > > PerformanceTests/ChangeLog:9 > > + Test function for the runAsync and runPerSecondAsync should invoke > > Do we really need both runAsync and runPerSecondAsync? Actually, I don't need runAsync. Adding runAsync corresponding to run looked cleaner to me. Should I drop runAsync?

I'd prefer just adding either one as a part of the bug 95704 since we don't normally add unused code.

Created attachment 164046 [details] Drop runAsync

(In reply to comment #4) > I'd prefer just adding either one as a part of the bug 95704 since we don't normally add unused code. Done. I dropped runAsync. Could you take a look again?

Comment on attachment 164046 [details] Drop runAsync View in context: https://bugs.webkit.org/attachment.cgi?id=164046&action=review > PerformanceTests/resources/runner.js:276 > + this._doneFunction = function () { if (test.done) test.done(); }; > + this._description = test.description || ""; > + this._runCount = test.runCount || 20; > + this._callsPerIteration = 1; > + this.unit = 'runs/s'; > + > + this._test = test; > + this._runner = this._perSecondAsyncRunner; > + this.initAndStartLoop(); We shouldn't have to duplicate all this code. > PerformanceTests/resources/runner.js:281 > + var timeToRun = this._test.timeToRun || 750; Is 750ms really long enough for async tests? How many runs per second do we typically get? Anything below 100 runs/s is statistically unsound. On the other hand, at 100 runs/s, each run is only 10ms at which point we have to worry about the time spent in context switch, etc... > PerformanceTests/resources/runner.js:305 > + self._test.run(nextIteration.bind(window, j + 1)); It's not clear to me how this "run" function is used. I would really like to see this patch being merged with the one that adds an actual test. Also, we need to be careful with calling functions like "bind". It may affect V8 behavior in subtle ways. > PerformanceTests/resources/runner.js:310 > + totalTime += Date.now() - startTime; > + i += callsPerIteration; > + if (self._completedRuns < 0 && totalTime < 100) > + callsPerIteration = Math.max(10, 2 * callsPerIteration); How slow is async function call here? If an async benchmark actually requires a context switch between two threads, then 100ms is probably not long enough.

Created attachment 164169 [details] add test, merge runPerSecond to async version

Comment on attachment 164046 [details] Drop runAsync View in context: https://bugs.webkit.org/attachment.cgi?id=164046&action=review >> PerformanceTests/resources/runner.js:276 >> + this.initAndStartLoop(); > > We shouldn't have to duplicate all this code. I merged runPerSecond to runPerSecondAsync. >> PerformanceTests/resources/runner.js:281 >> + var timeToRun = this._test.timeToRun || 750; > > Is 750ms really long enough for async tests? > How many runs per second do we typically get? Anything below 100 runs/s is statistically unsound. > On the other hand, at 100 runs/s, each run is only 10ms at which point we have to worry about the time spent in context switch, etc... For simple filesystem test like creating file or write small data to a file, it runs around 3500 times/sec on DumpRenderTree and over 1000 times/sec on Chrome. I think it's enough. >> PerformanceTests/resources/runner.js:305 >> + self._test.run(nextIteration.bind(window, j + 1)); > > It's not clear to me how this "run" function is used. I would really like to see this patch being merged with the one that adds an actual test. > > Also, we need to be careful with calling functions like "bind". It may affect V8 behavior in subtle ways. I see. I'll merge another change to this. I don't know how much time bind() need... Can we use anonymous function safely instead? >> PerformanceTests/resources/runner.js:310 >> + callsPerIteration = Math.max(10, 2 * callsPerIteration); > > How slow is async function call here? If an async benchmark actually requires a context switch between two threads, then 100ms is probably not long enough. My tests is enough fast, at least, even when it needs IPC round trip. If some test needs more time, it might need to be configurable.

(In reply to comment #9) > For simple filesystem test like creating file or write small data to a file, it runs around 3500 times/sec on DumpRenderTree and over 1000 times/sec on Chrome. I think it's enough. Okay. > I see. I'll merge another change to this. > > I don't know how much time bind() need... Can we use anonymous function safely instead? Ideally, the code that runs between each run is as small/fast as possible. In general, anything that non-trivial like creating new array, accessing a DOM node, creating an anonymous function, etc... should be avoided as much as possible. On the other hand, things like integer arithmetics should be fast and probably won't affect the js heap. > My tests is enough fast, at least, even when it needs IPC round trip. > If some test needs more time, it might need to be configurable. Okay.

Comment on attachment 164169 [details] add test, merge runPerSecond to async version Ugh... this is very outdated now. Please upload a new patch that applies to ToT.

Created attachment 169247 [details] Convert Animation/balls to use PerfTestRunner.measureValueAsync

Created attachment 169250 [details] Convert Animation/balls to use PerfTestRunner.measureValueAsync

Committed r131638: <http://trac.webkit.org/changeset/131638>