Conversation
|
Thanks for looking at this! I don't know if you are on the discord, but just for you information: With the 0.4.0 release this week, there is a review pause until the release is over, so only expect the earliest a review after the release on saturday. |
What? Is this #182 (fake out of memory)? Or an actual leak that we should be concerned about?
That last part makes no sense. localhost is the best case scenario, as the network latency is quite small the bandwidth large and the chance of packet loss is quite big. Of course you are not seeing any latency difference. Also we could easily cut down the latency cost by choosing a size similar to the MTU. I wonder what the actual impact of this is.
Honestly this does not sound like a solid benchmarking strategy. Ideally to have reproducible results, I'd suggest to choose a larger area and then only do a single change and measure the total network data (F7) and total server time (printed to console) |
attempts to fix #3250
I added an additional buffer to
SecureChannel, which stores the data-to-send in a temporary plaintext buffer. after the buffer has reached the negotiated TLS record size, it flushes/encrypts the plaintext buffer into theSendBuffer.The maximum buffer size (comptime) is determined by
c.MBEDTLS_SSL_OUT_CONTENT_LEN. however, the negotiated TLS record size can differ between TLS versions. that is why I explicitly get the negotiated TLS record length instead of relying onc.MBEDTLS_SSL_OUT_CONTENT_LENorc.MBEDTLS_SSL_OUT_CONTENT_LEN-1, in order to guarantee correctness for future TLS versions.Furthermore, I reckon there should be a flush point for the plaintext buffer, so that data-to-send does not stay in plaintext buffer for too long. I chose to place it in
Connection.processNextPackets, because that is the place where we determine whether the next UDP packet should be sent or not.That might not be optimal in all cases though, because of two things:
secureChannel.sendNextPacketAndGetSizecan return null). ButSendBufferalso decides whether to send based on the length of the send buffer. So we actually need the ciphertext size beforeSendBuffercan decide. (kinda a chicken/egg problem). one could try to estimate ciphertext size based on plaintext size and let theSendBufferdecide based on that. this adds quite some complexity. so I chose not to do it.Connection.processNextPacketsis called in the network thread, so it would do the encryption in the network thread, while encryption normally happens in the game/update thread. That might have consequences for the responsiveness of the network thread, if a particularly big plaintext buffer has to be flushed/encrypted at that stage.As for testing/benchmarking this, I did the following method (on both master and this branch, debug build):
/tp 0 0 0,/pos1,/tp 63 63 63,/pos2/set cubyz:dirtand/set cubyz:airYou can then observe, that the external message overhead grows around 2x faster on master than on this branch.
Furthermore, you can observe, that the game loop does not lag behind as much. That may be because some of the encryption is now done in network thread. But another contributing factor could be, that the TLS encryption is faster, because it is more likely to encrypt multiple block updates within a single record.
I also realize, that the benchmark does not really represent what usually happens in a cubyz world. #2585 would have been a good candidate, since #3250 comes out of it, but it's not based on current master.
Lastly, this PR may have consequences for latency under certain conditions.
Before this PR, N blockupdates would have all had their own TLS record (encrypted and decrypted individually), and that TLS record would usually be sent in 1-2 UDP packets. So the first blockupdate would be perceived right when that UDP packet comes around.
But now, those N blockupdates might all land in the same TLS record, which in turn could be spread over many UDP packets, which in turn means that the first blockupdate is only seen, once all those UDP packets have been sent.
That would be particularly pathological for small UDP sizes and large TLS record sizes, which is the case for localhost connection. I didn't notice any latency differences between master and this branch though.