| Summary: | [WebSocket] Add performance tests for data transmission | ||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|
| Product: | WebKit | Reporter: | Li Yin <li.yin> | ||||||||
| Component: | Tools / Tests | Assignee: | Nobody <webkit-unassigned> | ||||||||
| Status: | RESOLVED WONTFIX | ||||||||||
| Severity: | Normal | CC: | abarth, ap, dpranke, ojan, rniwa, webkit.review.bot, yutak | ||||||||
| Priority: | P2 | ||||||||||
| Version: | 528+ (Nightly build) | ||||||||||
| Hardware: | Unspecified | ||||||||||
| OS: | Unspecified | ||||||||||
| Bug Depends on: | |||||||||||
| Bug Blocks: | 77037 | ||||||||||
| Attachments: |
|
||||||||||
|
Description
Li Yin
2012-03-06 20:10:52 PST
To be able to run WebSocket performance tests, we have to make sure run-perf-tests start the pywebsocket server before it runs any tests, as new-run-webkit-tests does. HTTP tests may be a nice-to-have, too. Ryosuke and Dirk: Do you think adding WebSocket performance tests is a good idea? How about HTTP tests? I think you guys have better sense than me. (In reply to comment #1) > To be able to run WebSocket performance tests, we have to make sure run-perf-tests start the pywebsocket server before it runs any tests, as new-run-webkit-tests does. HTTP tests may be a nice-to-have, too. > > Ryosuke and Dirk: Do you think adding WebSocket performance tests is a good idea? How about HTTP tests? I think you guys have better sense than me. Does the WebSocket performance matter? i.e. are they worth spending bots' time on? Since each new test we add will lengthen the cycle time of bots, I'd rather not add new tests unless they're useful (i.e. you're going to use them to improve performance, etc...) (In reply to comment #2) > (In reply to comment #1) > > To be able to run WebSocket performance tests, we have to make sure run-perf-tests start the pywebsocket server before it runs any tests, as new-run-webkit-tests does. HTTP tests may be a nice-to-have, too. > > > > Ryosuke and Dirk: Do you think adding WebSocket performance tests is a good idea? How about HTTP tests? I think you guys have better sense than me. > > Does the WebSocket performance matter? i.e. are they worth spending bots' time on? Since each new test we add will lengthen the cycle time of bots, I'd rather not add new tests unless they're useful (i.e. you're going to use them to improve performance, etc...) I am designing a test to track the time of sending data through websocket, if the performance has been promoted, it will decrease the waiting time of ui, currently, the executing time of sending in firefox11.0 is shorter than chromium17, when the data size reaches 1MB. Created attachment 130786 [details]
Patch
Comment on attachment 130786 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=130786&action=review > Tools/Scripts/webkitpy/performance_tests/perftestsrunner.py:70 > + self._port.start_websocket_server() Can we put all websocket tests in one directory and then start websocket server only when tests in that directory are ran? > PerformanceTests/Websocket/sendData.html:1 > +<html> Missing DOCTYPE. Comment on attachment 130786 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=130786&action=review > PerformanceTests/Websocket/sendData.html:8 > + var NUM = 1024*1024; Please don't use abbreviations like NUM. Also, Number doesn't really describe what it is. Why is this particular number picked for this value? The variable name should self-evidentaly describe that. Created attachment 130944 [details]
Patch
Comment on attachment 130944 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=130944&action=review > Tools/Scripts/webkitpy/performance_tests/perftestsrunner.py:234 > + if relative_test_path.startswith('Websocket'): I think the correct capitalization is WebSocket. > PerformanceTests/Websocket/sendData.html:9 > + <script type="text/javascript"> > + var size = 1024*1024; We don't normally indent elements and script contents like this. > PerformanceTests/Websocket/sendData.html:11 > + var wsocket = new WebSocket("ws://localhost:8880/websocket/tests/hybi/send"); Is there anyway to manually start this local server? It'll be nice to have some description on how to run this test manually inside a browser. (In reply to comment #8) > > > PerformanceTests/Websocket/sendData.html:11 > > + var wsocket = new WebSocket("ws://localhost:8880/websocket/tests/hybi/send"); > > Is there anyway to manually start this local server? It'll be nice to have some description on how to run this test manually inside a browser. I think run-webkit-websocket-server does this ... Created attachment 130954 [details]
Patch
The send api can block the web ui, if it takes too much time, it can't be tolerated by web developer, and from my test, Firefox11 had a better performance than chromium when the sending data size reached 1MB. In addition, chromium19 had a lower efficiency than chromium17, chromium17 was about 1.3 times faster than chromium19. So, I think it's necessary to add this test case, and running this case takes only 1 second in my developing machine, which will not increase the burden of Performance Tests. Comment on attachment 130954 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=130954&action=review r- because of nits. > Tools/Scripts/webkitpy/performance_tests/perftestsrunner.py:237 > + if relative_test_path.startswith('WebSocket'): > + if self._port._websocket_server is None: > + self._port.start_websocket_server() > + My suggestion is to add a new PerfTest class like WebSocketPerfTest and then add some sort of prepare virtual method that gets called on every test. > PerformanceTests/WebSocket/sendData.html:11 > + var wsocket = new WebSocket("ws://localhost:8880/websocket/tests/hybi/echo"); Please don't use abbreviations like wsocket. > PerformanceTests/WebSocket/sendData.html:15 > + /* > + * if you run this page in web browser, please run command > + * "Tools/Scripts/run-webkit-websocketserver" to start websocket server. > + */ Could you put this in the body so that it's visible when you open it? You can hide it (e.g. by display = 'none' or removing it from the document) when it's ran properly (i.e. when it's hosted on localhost). > PerformanceTests/WebSocket/sendData.html:29 > + PerfTestRunner.run(startFunc, 10, 5, completeFunc); > + }; > + > + function startFunc() { > + wsocket.send(data); > + } > + > + function completeFunc() { > + wsocket.close(); > + } Since startFunc and completeFunc are only called by run, it's better to write it as: PerfTestRunner.run(function () { wsocket.send(data); }, 10, 5, function () { wsocket.close(); }); By the way, could lower the number of function calls per iteration and increase the number of runs instead? Taking just 5 samples isn't enough to compute min/max/stdev. Comment on attachment 130954 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=130954&action=review > Tools/ChangeLog:4 > + https://bugs.webkit.org/show_bug.cgi?id=80476 Style nit : Please add a line to here. Comment on attachment 130954 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=130954&action=review > PerformanceTests/WebSocket/sendData.html:20 > + PerfTestRunner.run(startFunc, 10, 5, completeFunc); You should use runPerSecond function added in http://trac.webkit.org/changeset/116916 instead. Comment on attachment 130954 [details]
Patch
r- per my previous comments.
It is not very very hot spot compared with bulk data in JS parsing. So mark it to be WONTFIX. My apology for wasting reviewers' time. |