Remove unnecessary allocations and Write calls. #215
Conversation
This commit improves the performance of the rx and tx loop by 1) moving allocations to outside the loop and by 2) eliminating an unnecessary Write call. As long as gvproxy implements the same improvements, this should result in ~20% more reqs/sec. For posterity, here's the upstream PR for the gvproxy side of things: containers/gvisor-tap-vsock#215
This commit improves the performance of the rx and tx loop by 1) moving allocations to outside the loop and by 2) eliminating an unnecessary Write call. As long as gvproxy implements the same improvements, this should result in ~20% more reqs/sec. For posterity, here's the upstream PR for the gvproxy side of things: containers/gvisor-tap-vsock#215
This commit improves the performance of the rx and tx loop by 1) moving allocations to outside the loop and by 2) eliminating an unnecessary Write call. As long as gvproxy implements the same improvements, this should result in ~20% more reqs/sec. For posterity, here's the upstream PR for the gvproxy side of things: containers/gvisor-tap-vsock#215
|
/approve |
|
please sign your commit. i.e.
|
| } | ||
| if _, err := conn.Write(frame); err != nil { | ||
| errCh <- errors.Wrap(err, "cannot write packet to socket") | ||
| if _, err := conn.Write(append(size, frame...)); err != nil { |
There was a problem hiding this comment.
I'm slightly puzzled by this one. The append involves a memory copy and possibly a memory allocation, while at the same time you move another allocation out of the loop. Is conn.Write() a lot more expensive than a memory allocation?
There was a problem hiding this comment.
That's a good point. Over here, the overhead of the append call is less than the overhead of two Write calls: I saw a ~7% increase in both throughput (measured via iperf3) and requests per second (measured via a custom test bed). Also, I noticed that the buffer pre-allocation in rxStream made things worse, so I removed that code. That said, we could get rid of the append by adding a re-usable buffer but that requires a more involved fix.
Regarding the changes to main_linux.go: I haven't tested these changes directly. Instead, I tested our modified fork: https://github.com/brave/nitriding-daemon/blob/master/proxy.go#L124-L185 I'm happy to refactor main_linux.go's rx/tx loop to resemble what we've done.
There was a problem hiding this comment.
One other performant alternate for append might be https://pkg.go.dev/net#Buffers.WriteTo
This makes use of writev syscall to send multiple buffers over a single write.
We can give a try with this and see if it improves. I noticed that there is a point in document that buffers are optimised with writev for some connections so need to verify if it actually improves for us
NullHypothesis
force-pushed
the
reduce-overhead
branch
from
1fb8057 to
3cc752f
Compare
This commit improves networking performance by 1) moving an unnecessary allocation to outside the hot loop and by 2) eliminating an unnecessary Write call. This result in ~7% faster throughput as measured via iperf3 and a custom Web service to measure requests per second. Note that this is *despite* the append call. Signed-off-by: Philipp Winter <phw@brave.com>
NullHypothesis
force-pushed
the
reduce-overhead
branch
from
3cc752f to
4f4385e
Compare
|
I've ran some testing over this, |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: baude, cfergeau, NullHypothesis The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
This commit improves performance by 1) moving an unnecessary allocation to outside the hot loop and by 2) eliminating an unnecessary Write call.