split header and data functions #244
Conversation
…ting them together
|
|
||
| self.send_parts.append(encbuf) | ||
| opcode = 2-int(self.base64) #based64: opcode=1, binary: opcode=2 | ||
| encbufs = self.send_hybi(buf, opcode, base64=self.base64, record=self.rec) | ||
|
|
There was a problem hiding this comment.
You dropped the recording functionality. Also, the condition opcode calculation, while more concise, obfuscates the actual logic that is happening; the original is preferable to me.
There was a problem hiding this comment.
Oh, nevermind about the record functionality, I'm just blind I guess.
|
I'm okay with this change (apart from my inline comment). Have you tested with python 2.4 and python 3.X? |
|
@totaam out of curiosity, how has the html5 interface in xpra been working out? I assume this was a performance enhancement discovered as part of using websockify in xpra? |
Can't remember about 2.4, will re-check and get back to you.
The opcode thing, sure. Back to a simple "if-else" instead?
Great. In order to support some of the best features of xpra (sound forwarding, video encodings, etc), we've had to make a lot of drastic changes to the html5 code recently. The network performance issues we found turned out to be in the xpra html5 network code, but looking into websockify was useful anyway. I was going to quantify the benefits of this patch, but moved on to other things. |
|
Yeah, simple if-else is preferable to me. It's been a couple years since I've played with xpra, I'll have to try it again soon. |
|
Done. |
|
|
||
| self.send_parts.append(encbuf) | ||
| opcode = 2 | ||
| encbufs = self.send_hybi(buf, opcode, base64=self.base64, record=self.rec) |
There was a problem hiding this comment.
send_hybi doesn't look like it returns anything. Also, doesn't this make the whiel self.send_parts below redundant?
|
LGTM modulo the comment above. Will merge once that's addressed. |
Nagle was disabled for websockify in f23780e how does this affect things here? |
|
@DirectXMan12 right you are, the send_parts code wasn't used at all either. @samhed this shouldn't make much of a difference even with naggle disabled: the packet join size of 4K is high enough that you won't be sending just the packet header alone. Even then, it may or may not cost an extra TCP packet for payload sizes above 4K. (depending on the OS network stack, send queue levels, etc..) What some transports do is to disable naggle when sending multiple chunks and re-enable it immediately after. (I don't think it is worth the hassle - could be worth a try) |
* no daemonizing support * no SIGCHLD signal * no multiprocessing support * override copyfile so we can retry on WSAEWOULDBLOCK
| if record: | ||
| record.write("%s,\n" % repr("{%s{" % tdelta + encbufs[1])) | ||
| if len(buf)<=4096: | ||
| self.request.send(header+buf) |
There was a problem hiding this comment.
I didn't catch this earlier, but it looks live we've lost detection of partial sends here, which could be bad.
There was a problem hiding this comment.
How so?
I don't see it in the quoted lines.
There was a problem hiding this comment.
because socket.send isn't guaranteed to have sent all the requested data, and it returns however much it sends. Previously, the related methods here would return the result of self.request.send, and we would check to make sure the entirety of the buffer was sent. If not, we'd remember the rest, and send it later. This patch removes that functionality entirely (it can be seen here in the fact that we don't do anything with the result of self.request.send).
|
I'm a bit sceptical about this. Can you share some tests that show the performance issue/improvement? |
Main benefits:
The threshold is set arbitrarily at 4KB, you could turn it into a constant and if this is set high enough you would not see any difference with the current version of the code, except it will perform better with large packets.