Bug 102079 - Disable Nagle algorithm on WebSocket implementation
Summary: Disable Nagle algorithm on WebSocket implementation
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebCore Misc. (show other bugs)
Version: 528+ (Nightly build)
Hardware: Macintosh OS X 10.8
: P2 Normal
Assignee: Takashi Toyoshima
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2012-11-13 06:32 PST by Takashi Toyoshima
Modified: 2012-12-20 23:53 PST (History)
6 users (show)

See Also:


Attachments
Patch (2.28 KB, patch)
2012-11-13 06:42 PST, Takashi Toyoshima
buildbot: commit-queue-
Details | Formatted Diff | Diff
Patch (2.42 KB, patch)
2012-11-27 06:47 PST, Takashi Toyoshima
buildbot: commit-queue-
Details | Formatted Diff | Diff
Patch (2.31 KB, patch)
2012-12-06 06:06 PST, Takashi Toyoshima
buildbot: commit-queue-
Details | Formatted Diff | Diff
Patch (1.83 KB, patch)
2012-12-10 21:21 PST, Takashi Toyoshima
buildbot: commit-queue-
Details | Formatted Diff | Diff
Patch (2.97 KB, patch)
2012-12-20 01:25 PST, Takashi Toyoshima
buildbot: commit-queue-
Details | Formatted Diff | Diff
Patch (3.00 KB, patch)
2012-12-20 06:23 PST, Takashi Toyoshima
buildbot: commit-queue-
Details | Formatted Diff | Diff
Patch (1.78 KB, patch)
2012-12-20 06:53 PST, Takashi Toyoshima
ap: review+
ap: commit-queue-
Details | Formatted Diff | Diff
Patch (1.87 KB, patch)
2012-12-20 21:41 PST, Takashi Toyoshima
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Takashi Toyoshima 2012-11-13 06:32:20 PST
WebSocket users need to send and receive small packets without latency.
But, currently CF port has a flaky latency issue in some environment.
This is because CF port doesn't disable Nagle.
Combination of Nagle and delayed-ACK causes performance problem.

I confirm performance regression happens on communicating from Japan to US using laptop machine over Wifi.
I can see the same problem in Safari for OS X and iOS.
Comment 1 Takashi Toyoshima 2012-11-13 06:33:02 PST
FYI, Chrome disables Nagle and doesn't have this problem.
Comment 2 Takashi Toyoshima 2012-11-13 06:42:52 PST
Created attachment 173883 [details]
Patch
Comment 3 Build Bot 2012-11-13 07:11:18 PST
Comment on attachment 173883 [details]
Patch

Attachment 173883 [details] did not pass win-ews (win):
Output: http://queues.webkit.org/results/14820564
Comment 4 Alexey Proskuryakov 2012-11-13 10:31:45 PST
10>..\platform\network\cf\SocketStreamHandleCFNet.cpp(41) : fatal error C1083: Cannot open include file: 'netinet/in.h': No such file or directory
Comment 5 Alexey Proskuryakov 2012-11-13 10:51:20 PST
Is the issue you are seeing the one described here <http://www.stuartcheshire.org/papers/NagleDelayedAck/>?

Can you please provide a more detailed description of the issue, and attach a tcpdump capture?
Comment 6 Takashi Toyoshima 2012-11-14 03:56:41 PST
Hi Alexey,

I sent some data related to this problem in email directly.
Comment 7 Alexey Proskuryakov 2012-11-14 21:28:48 PST
<rdar://problem/12706490>
Comment 8 Takashi Toyoshima 2012-11-27 06:47:17 PST
Created attachment 176258 [details]
Patch

Previous patch fails to set NODELAY.
Comment 9 Build Bot 2012-11-27 07:14:50 PST
Comment on attachment 176258 [details]
Patch

Attachment 176258 [details] did not pass win-ews (win):
Output: http://queues.webkit.org/results/15013102
Comment 10 Takashi Toyoshima 2012-12-06 06:06:18 PST
Created attachment 178004 [details]
Patch

Hopefully, this will fix cf/win build.
Comment 11 Takashi Toyoshima 2012-12-06 06:07:29 PST
Comment on attachment 178004 [details]
Patch

fix MIME Type and patch flag.
Comment 12 Build Bot 2012-12-06 06:43:29 PST
Comment on attachment 178004 [details]
Patch

Attachment 178004 [details] did not pass win-ews (win):
Output: http://queues.webkit.org/results/15152855
Comment 13 Alexey Proskuryakov 2012-12-06 09:56:06 PST
I think that we should use an SPI here:

    CFStreamCreatePairWithSocketToHost(0, host.get(), port(), &readStream, &writeStream);
+    CFWriteStreamSetProperty(writeStream, _kCFStreamSocketSetNoDelay, kCFBooleanTrue);

and define the symbol at the top of the file:

#if PLATFORM(WIN)
#define EXTERN extern "C" __declspec(dllimport)
#else
#define EXTERN extern "C"
#endif

EXTERN const CFStringRef _kCFStreamSocketSetNoDelay;
Comment 14 Takashi Toyoshima 2012-12-10 21:21:20 PST
Created attachment 178705 [details]
Patch

Thanks.
Your suggestion works fine!
Comment 15 Build Bot 2012-12-10 21:49:10 PST
Comment on attachment 178705 [details]
Patch

Attachment 178705 [details] did not pass win-ews (win):
Output: http://queues.webkit.org/results/15236855
Comment 16 Takashi Toyoshima 2012-12-11 00:45:39 PST
16>   Creating library C:\cygwin\home\buildbot\WebKit\WebKitBuild\Debug\lib\WebKit.lib and object C:\cygwin\home\buildbot\WebKit\WebKitBuild\Debug\lib\WebKit.exp
16>WebCore.lib(SocketStreamHandleCFNet.obj) : error LNK2001: unresolved external symbol __imp___kCFStreamSocketSetNoDelay
16>C:\cygwin\home\buildbot\WebKit\WebKitBuild\Debug\bin\WebKit.dll : fatal error LNK1120: 1 unresolved externals

Oops, stub lib doesn't have an entry for _kCFStreamSocketSetNoDelay seemingly.

I'll try the 3rd patch with include wsock2.h.
Comment 17 Takashi Toyoshima 2012-12-20 01:25:04 PST
Created attachment 180294 [details]
Patch

I'm sorry but I still don't have perfect build environment for CF/WIN. So could you allow me to post a patch for try.
Comment 18 Build Bot 2012-12-20 05:12:18 PST
Comment on attachment 180294 [details]
Patch

Attachment 180294 [details] did not pass win-ews (win):
Output: http://queues.webkit.org/results/15406973
Comment 19 Takashi Toyoshima 2012-12-20 06:23:59 PST
Created attachment 180331 [details]
Patch
Comment 20 Build Bot 2012-12-20 06:43:29 PST
Comment on attachment 180331 [details]
Patch

Attachment 180331 [details] did not pass win-ews (win):
Output: http://queues.webkit.org/results/15456012
Comment 21 Takashi Toyoshima 2012-12-20 06:53:13 PST
Created attachment 180335 [details]
Patch

Hum... can I give up to support CF/Win...
Comment 22 Alexey Proskuryakov 2012-12-20 09:22:38 PST
Comment on attachment 180335 [details]
Patch

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

> Source/WebCore/platform/network/cf/SocketStreamHandleCFNet.cpp:282
> +    CFWriteStreamSetProperty(writeStream, _kCFStreamSocketSetNoDelay, kCFBooleanTrue);

Please add a comment here:

    // <rdar://problem/12855587> _kCFStreamSocketSetNoDelay is not exported on Windows
Comment 23 Takashi Toyoshima 2012-12-20 21:41:06 PST
Created attachment 180479 [details]
Patch
Comment 24 WebKit Review Bot 2012-12-20 21:42:51 PST
Comment on attachment 180479 [details]
Patch

Rejecting attachment 180479 [details] from review queue.

toyoshim@chromium.org does not have reviewer permissions according to http://trac.webkit.org/browser/trunk/Tools/Scripts/webkitpy/common/config/committers.py.

- If you do not have reviewer rights please read http://webkit.org/coding/contributing.html for instructions on how to use bugzilla flags.

- If you have reviewer rights please correct the error in Tools/Scripts/webkitpy/common/config/committers.py by adding yourself to the file (no review needed).  The commit-queue restarts itself every 2 hours.  After restart the commit-queue will correctly respect your reviewer rights.
Comment 25 WebKit Review Bot 2012-12-20 22:08:26 PST
Comment on attachment 180479 [details]
Patch

Clearing flags on attachment: 180479

Committed r138348: <http://trac.webkit.org/changeset/138348>
Comment 26 WebKit Review Bot 2012-12-20 22:08:31 PST
All reviewed patches have been landed.  Closing bug.
Comment 27 Ryosuke Niwa 2012-12-20 23:07:03 PST
This caused a build failure on Mac:
http://build.webkit.org/builders/Apple%20Lion%20Debug%20%28Build%29/builds/9716/steps/compile-webkit/logs/stdio
Undefined symbols for architecture x86_64:
  "__kCFStreamSocketSetNoDelay", referenced from:
      __ZN7WebCore18SocketStreamHandle13createStreamsEv in SocketStreamHandleCFNet.o
ld: symbol(s) not found for architecture x86_64
Comment 28 Alexey Proskuryakov 2012-12-20 23:26:30 PST
Disabled this new code on Lion in r138351 as a build fix.
Comment 29 Takashi Toyoshima 2012-12-20 23:34:34 PST
ap: thanks!
Comment 30 Ryosuke Niwa 2012-12-20 23:43:29 PST
One more fix: http://trac.webkit.org/changeset/138352
Comment 31 Takashi Toyoshima 2012-12-20 23:53:57 PST
rniwa:
Thanks!
To my great relief, iOS supports _kCFStreamSocketSetNoDelay ;-)