fix websockify bottleneck

[totaam]

Issue migrated from trac ticket # 1134

component: external | priority: minor | resolution: wontfix

2016-02-24 04:02:37: antoine created the issue

---

Apparently, websockify can become the bottleneck with the [/wiki/Clients/HTML5 HTML5 client] when stressed with packet storms (ie: lots of screen refreshes as happens when you scroll a window for example).

I will attach some patches to websockify which try to fix the most glaring issue I spotted: our use case may be slightly unusal in that we transfer some fairly large frames, and when dealing with those the existing packet handling code makes unnecessary copies.

There may well be other issues that need fixing / tuning. I'm not even sure this will make enough of a difference.. but let's try.

[totaam]

2016-02-24 04:03:37: antoine uploaded file nojoin.patch (3.3 KiB)

first version: just never join the headers and packet data

[totaam]

2016-02-24 04:04:14: antoine uploaded file nojoin-v2.patch (4.6 KiB)

split the header formatting into its own function

[totaam]

2016-02-24 04:05:06: antoine uploaded file nojoin-v3.patch (4.6 KiB)

define a convenience send method that we can just re-use everywhere

[totaam]

2016-02-24 04:05:35: antoine uploaded file nojoin-v4.patch (4.7 KiB)

do merge the header and the packet data if the size is sufficiently small (4KB)

[totaam]

2016-02-24 04:31:14: antoine changed status from new to assigned

[totaam]

2016-02-24 04:31:14: antoine commented

---

I am not convinced that this will fix the problem but maybe it will help a bit.
Even if it did help a lot, maybe there's still another bug somewhere: surges should not cause the network traffic to stall, even if the code spends too much time aggregating header and data...
It's also worth double-checking that the bottleneck really is in websockify and not somewhere else, the other candidates are: xpra's [/browser/xpra/trunk/src/xpra/scripts/fdproxy.py fdproxy] (which now has a configurable buffer size, set to 64KB), or even the browser's websocket code. [ browser xpra trunk src xpra scripts fdproxy.py fdproxy] (which now has a configurable buffer size, set to 64KB), or even the browser's websocket code. [Profiling](-browser-xpra-trunk-src-xpra-scripts-fdproxy.py fdproxy] (which now has a configurable buffer size, set to 64KB), or even the browser's websocket code. [../wiki/Profiling) may help with that.

---

Looking at the rest of the websockify code:

• the default socket buffer size is 64KB, which should be plenty
• the TCP-to-websocket code looks fine and as efficient as it's going to get with those patches on top. I guess there could still be inefficiencies in the python http server classes, but that's very unlikely. Cythonizing it would probably not help much as it's mostly doing IO.
• the websocket-to-TCP code suffers from the same issue that I fixed in xpra a long long time ago (r207 back in 2011): because of the nature of the protocol, it will just try to parse the packet repeatedly until it succeeds - which is very inefficient. This is more of a problem with large frames where the code will repeatedly parse the header to find the payload length, only to drop it when it finds the payload is still incomplete. To fix this, the header parsing should be cached once the header is complete. But I don't think that's the cause of the problem either: the buffer doesn't get modified in that parsing code, so unless we receive very small chunks and go through this code thousands of times - it shouldn't matter.

[totaam]

2016-06-17 03:55:55: antoine commented

---

Things are a lot better now that websockify runs in-process (#1136), but we should still verify that the patches above are worth carrying, or not.

The way forward:

• find a way to benchmark the changes above with large-ish packets and with regular traffic, maybe write a unit test for it?
• test with naggle on and off
• submit upstream if worth it

Other related tickets:

• http / websockify ssl support #1213 SSL
• websockify and named pipe timeouts on win32 #1211 win32 timeouts
• better TCP_NODELAY handling: only use it when it is useful #619 TCP_NODELAY aka naggle

[totaam]

2016-07-07 08:58:20: antoine changed priority from major to minor

[totaam]

2016-07-07 08:58:20: antoine commented

---

This ticket may have been made redundant by #1136, would be good to evaluate the benefits of the patches above still.

[totaam]

2016-07-12 17:52:23: antoine commented

---

Milestone renamed

[totaam]

2016-08-06 16:44:49: antoine commented

---

The nojoin patch has been submitted upstream: [https://github.com/novnc/websockify/pull/244]

[totaam]

2016-11-18 12:01:37: antoine commented

---

Don't have time for this right now.

[totaam]

2017-05-22 16:47:48: antoine commented

---

Once the patches are merged, we can revert r15924

[totaam]

2017-07-01 07:33:44: antoine commented

---

(re-scheduling - websockify upstream is still making changes)

[totaam]

2017-10-25 11:09:34: antoine commented

---

still no new upstream release and they've closed the ticket, re-scheduling

[totaam]

2018-08-01 18:58:05: antoine commented

---

See also #1926.

[totaam]

2019-03-20 05:00:18: antoine changed status from assigned to closed

[totaam]

2019-03-20 05:00:18: antoine set resolution to wontfix

[totaam]

2019-03-20 05:00:18: antoine commented

---

We're no longer using websockify: #2121