Xpra: Ticket #1134: fix websockify bottleneck
Apparently, websockify can become the bottleneck with the 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.
Wed, 24 Feb 2016 04:03:36 GMT - Antoine Martin: attachment set
- attachment
set to nojoin.patch
first version: just never join the headers and packet data
Wed, 24 Feb 2016 04:04:13 GMT - Antoine Martin: attachment set
- attachment
set to nojoin-v2.patch
split the header formatting into its own function
Wed, 24 Feb 2016 04:05:05 GMT - Antoine Martin: attachment set
- attachment
set to nojoin-v3.patch
define a convenience send method that we can just re-use everywhere
Wed, 24 Feb 2016 04:05:34 GMT - Antoine Martin: attachment set
- attachment
set to nojoin-v4.patch
do merge the header and the packet data if the size is sufficiently small (4KB)
Wed, 24 Feb 2016 04:31:13 GMT - Antoine Martin: status changed
- status
changed from new to assigned
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 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.
Fri, 17 Jun 2016 02:55:54 GMT - Antoine Martin: milestone changed
- milestone
changed from 0.17 to 0.18
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:
Thu, 07 Jul 2016 07:58:19 GMT - Antoine Martin: priority changed
- priority
changed from major to minor
This ticket may have been made redundant by #1136, would be good to evaluate the benefits of the patches above still.
Tue, 12 Jul 2016 16:52:22 GMT - Antoine Martin: milestone changed
- milestone
changed from 0.18 to 1.0
Milestone renamed
Sat, 06 Aug 2016 15:44:49 GMT - Antoine Martin:
The nojoin patch has been submitted upstream: https://github.com/kanaka/websockify/pull/244
Fri, 18 Nov 2016 12:01:37 GMT - Antoine Martin: milestone changed
- milestone
changed from 1.0 to 2.0
Don't have time for this right now.
Thu, 16 Feb 2017 06:25:52 GMT - Antoine Martin: milestone changed
- milestone
changed from 2.0 to 2.1
Mon, 22 May 2017 15:47:47 GMT - Antoine Martin:
Once the patches are merged, we can revert r15924
Sat, 01 Jul 2017 06:33:44 GMT - Antoine Martin: milestone changed
- milestone
changed from 2.1 to 2.2
(re-scheduling - websockify upstream is still making changes)
Wed, 25 Oct 2017 10:09:33 GMT - Antoine Martin: milestone changed
- milestone
changed from 2.2 to 3.0
still no new upstream release and they've closed the ticket, re-scheduling
Wed, 01 Aug 2018 17:58:04 GMT - Antoine Martin:
See also #1926.
Wed, 20 Mar 2019 05:00:18 GMT - Antoine Martin: status changed; resolution set
- status
changed from assigned to closed
- resolution
set to wontfix
We're no longer using websockify: #2121
Sat, 23 Jan 2021 05:15:56 GMT - migration script:
this ticket has been moved to: https://github.com/Xpra-org/xpra/issues/1134