Bug 80476

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:

Version:

528+ (Nightly build)

Hardware:

Unspecified

OS:

Unspecified

Bug Depends on:

Bug Blocks:

77037

Attachments:

Description	Flags
Patch	none
Patch	none
Patch	rniwa: review-

Li Yin

Reported 2012-03-06 20:10:52 PST

Add the performance test case to track the efficiency of sending data.

Attachments
Patch (3.32 KB, patch) 2012-03-08 00:13 PST, Li Yin	no flags	Details Formatted Diff Diff
Patch (3.15 KB, patch) 2012-03-08 17:57 PST, Li Yin	no flags	Details Formatted Diff Diff
Patch (3.21 KB, patch) 2012-03-08 19:23 PST, Li Yin	rniwa: review-	Details Formatted Diff Diff
Show Obsolete (2) View All Add attachment proposed patch, testcase, etc.

Yuta Kitamura

Comment 1 2012-03-07 10:39:37 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.

Ryosuke Niwa

Comment 2 2012-03-07 11:58:43 PST

(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...)

Li Yin

Comment 3 2012-03-07 16:44:14 PST

(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.

Li Yin

Comment 4 2012-03-08 00:13:31 PST

Created attachment 130786 [details] Patch

Ryosuke Niwa

Comment 5 2012-03-08 00:27:23 PST

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.

Ryosuke Niwa

Comment 6 2012-03-08 00:29:00 PST

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.

Li Yin

Comment 7 2012-03-08 17:57:55 PST

Created attachment 130944 [details] Patch

Ryosuke Niwa

Comment 8 2012-03-08 18:05:31 PST

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.

Dirk Pranke

Comment 9 2012-03-08 18:08:28 PST

(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 ...

Li Yin

Comment 10 2012-03-08 19:23:36 PST

Created attachment 130954 [details] Patch

Li Yin

Comment 11 2012-03-10 05:33:47 PST

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.

Ryosuke Niwa

Comment 12 2012-04-22 21:41:34 PDT

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.

Gyuyoung Kim

Comment 13 2012-05-21 07:49:20 PDT

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.

Ryosuke Niwa

Comment 14 2012-05-21 08:06:57 PDT

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.

Ryosuke Niwa

Comment 15 2012-05-21 08:07:21 PDT

Comment on attachment 130954 [details] Patch r- per my previous comments.

Li Yin

Comment 16 2012-09-01 21:40:43 PDT

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.

Note You need to log in before you can comment on or make changes to this bug.