fix(codec): Allocate inbound buffer once #578
Conversation
Gets rid of reallocs when receiving large messages, increasing throughput by about 30%-40%.
There was a problem hiding this comment.
I think this looks good and I assume some tests would fail if it was wrong.
But would like to hear what @LucioFranco thinks.
| @@ -180,6 +180,7 @@ impl<T> Streaming<T> { | |||
| } | |||
| }; | |||
| let len = self.buf.get_u32() as usize; | |||
| self.buf.reserve(len); | |||
There was a problem hiding this comment.
Would this be a vector for attack? Since a user could pass any number here and reserve as much memory as possible even if they can't send that on the wire?
There was a problem hiding this comment.
Yes and no. As far as I see, even before this PR, there was also no protection against the user accumulating an arbitrary large buffer (by, well, just actually sending it). So in this way, this PR doesn't make the situation worse, except that it lowers the attacker's effort.
On macOS and Linux, allocating huge buffers doesn't actually fault & reserve the corresponding pages until they are actually written to (overcommit), so the situation doesn't change at all. On Windows, only the size of the page file can be overcommitted, so this would crash immediately on very large values (if the user is low on RAM already).
Unless I'm wrong and this is actually verified (but I just didn't see where, which is very much possible!), I'd argue: yes, this clearly is an issue, but one that should probably be discussed and fixed in a dedicated issue?
There was a problem hiding this comment.
Thanks for the great write up. Yeah, I think this makes sense then!
LucioFranco
changed the title
During profiling for bottlenecks in my tonic powered message queue, I noticed that a large amount of compute was spent in reallocating and
memmoving over read buffers.This PR gets rid of the reallocs entirely by pre-allocating the buffer for the entire message body directly after the message size is known instead of only reserving space for the next chunk when they come in.
In my testing with 8MiB messages, this improved throughput from 370MiB/s to 560MiB/s, which is roughly a 50% improvement. I originally mistyped the numbers, thus the "30-40%" in the commit message -- didn't want to force-push just for that.