Bug 113304 - SocketStreamHandle (Chromium port) should fully use IPC window in send()
Summary: SocketStreamHandle (Chromium port) should fully use IPC window in send()
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebCore Misc. (show other bugs)
Version: 528+ (Nightly build)
Hardware: All All
: P2 Normal
Assignee: Takeshi Yoshino
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2013-03-26 06:26 PDT by Takeshi Yoshino
Modified: 2013-03-26 22:05 PDT (History)
5 users (show)

See Also:


Attachments
Patch (2.21 KB, patch)
2013-03-26 06:31 PDT, Takeshi Yoshino
no flags Details | Formatted Diff | Diff
Patch (2.34 KB, patch)
2013-03-26 20:50 PDT, Takeshi Yoshino
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Takeshi Yoshino 2013-03-26 06:26:58 PDT
socket_stream of Chromium buffers send data up to 32KiB (exact) bytes.

However, SocketStreamHandleInternal::send() method keeps in-flight send data not greater than m_maxPendingSendAllowed - 1 that is 32KiB - 1. This means that SocketStreamHandleInternal consumes the buffered data in SocketStreamHandleBase by 32KiB - 1. It makes memory copy operations unaligned unnecessarily.

An experiment using the benchmark tool of pywebsocket and emulated 10ms RTT ($ sudo tc qdisc add dev lo root netem delay 10ms) showed this change improves send performance to ~3 times of current.

Message|
size   |  After     Before
KiB    |  (kB/s)    (kB/s)
-------+----------------------------------
10     |50171.485  21000.82
50     |63327.149  22520.343
100    |61798.431  22664.896
500    |67949.569  23572.744
1000   |68817.204  23858.341
5000   |68817.204  23836.127
10000  |64160.401  23437.858
50000  |59362.319  22314.23
100000 |59918.081  23852.784
Comment 1 Takeshi Yoshino 2013-03-26 06:31:21 PDT
Created attachment 195075 [details]
Patch
Comment 2 Takashi Toyoshima 2013-03-26 15:22:57 PDT
Comment on attachment 195075 [details]
Patch

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

I agree with this change.

> Source/WebCore/platform/network/chromium/SocketStreamHandle.cpp:80
>      if (m_pendingAmountSent + len >= m_maxPendingSendAllowed)

Nits: Now, this condition can be '>' instead of '>='.
Comment 3 Takashi Toyoshima 2013-03-26 15:25:56 PDT
+tkent for sanctifying to land this change.
Comment 4 Takeshi Yoshino 2013-03-26 20:50:29 PDT
Created attachment 195212 [details]
Patch
Comment 5 Takeshi Yoshino 2013-03-26 20:51:48 PDT
Thanks for review, Takashi.

(In reply to comment #2)
> Nits: Now, this condition can be '>' instead of '>='.

done
Comment 6 Kent Tamura 2013-03-26 21:41:44 PDT
Comment on attachment 195212 [details]
Patch

Looks ok
Comment 7 WebKit Review Bot 2013-03-26 22:05:41 PDT
Comment on attachment 195212 [details]
Patch

Clearing flags on attachment: 195212

Committed r146965: <http://trac.webkit.org/changeset/146965>
Comment 8 WebKit Review Bot 2013-03-26 22:05:46 PDT
All reviewed patches have been landed.  Closing bug.