xpra icon
Bug tracker and wiki

Opened 20 months ago

Last modified 4 months ago

#1134 assigned enhancement

fix websockify bottleneck

Reported by: Antoine Martin Owned by: Antoine Martin
Priority: minor Milestone: 2.2
Component: external Version: trunk
Keywords: Cc:

Description

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.

Attachments (4)

nojoin.patch (3.3 KB) - added by Antoine Martin 20 months ago.
first version: just never join the headers and packet data
nojoin-v2.patch (4.6 KB) - added by Antoine Martin 20 months ago.
split the header formatting into its own function
nojoin-v3.patch (4.6 KB) - added by Antoine Martin 20 months ago.
define a convenience send method that we can just re-use everywhere
nojoin-v4.patch (4.7 KB) - added by Antoine Martin 20 months ago.
do merge the header and the packet data if the size is sufficiently small (4KB)

Download all attachments as: .zip

Change History (13)

Changed 20 months ago by Antoine Martin

Attachment: nojoin.patch added

first version: just never join the headers and packet data

Changed 20 months ago by Antoine Martin

Attachment: nojoin-v2.patch added

split the header formatting into its own function

Changed 20 months ago by Antoine Martin

Attachment: nojoin-v3.patch added

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

Changed 20 months ago by Antoine Martin

Attachment: nojoin-v4.patch added

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

comment:1 Changed 20 months ago by Antoine Martin

Status: newassigned

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.
Last edited 20 months ago by Antoine Martin (previous) (diff)

comment:2 Changed 16 months ago by Antoine Martin

Milestone: 0.170.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:

comment:3 Changed 16 months ago by Antoine Martin

Priority: majorminor

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

comment:4 Changed 16 months ago by Antoine Martin

Milestone: 0.181.0

Milestone renamed

comment:5 Changed 15 months ago by Antoine Martin

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

comment:6 Changed 11 months ago by Antoine Martin

Milestone: 1.02.0

Don't have time for this right now.

comment:7 Changed 8 months ago by Antoine Martin

Milestone: 2.02.1

comment:8 Changed 5 months ago by Antoine Martin

Once the patches are merged, we can revert r15924

comment:9 Changed 4 months ago by Antoine Martin

Milestone: 2.12.2

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

Note: See TracTickets for help on using tickets.