Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Backstory
I've been working on a server that acts as a garbler for several quite large circuits. It's performance was inappropriately poor. Profiler made it clear that most of the time we wait for connection read/write. After digging some more I've noticed that
ot.CO
makes a lot of very small flushes. In my case I have ~20 MiB of data transferred over the TCP connection, which would be fine for several huge batches, butot.CO
Send
andRecieve
were sending 40-72 bytes, flushing, and waiting for the "response" to arrive before sending the next part. It is mentioned in theot/co.go
header that the implementation is based on emp-toolkit. I've noticed that they do this stage separation of write everything first, then flush, then wait for response. AndCO
does a loop with send-flush-read inside which makesp2p.Conn
flush these small batches of data.Fix
I propose to use the same stage separation approach. Basically, here we only flush once after everything is written. The downside: we'll loop twice, but it's still sort of
O(n)
. I've already tested this fix with my server, observed behavior: 4-5 minutes of execution before, 12-14 seconds after, which is a huge difference.Tests
Output
Benchmarks
This change doesn't seem to be influencing current benchmarks much:
Before
After
At first I wanted to update
ot.Pipe
with something likeNewLatencyPipe(time.Duration)
and to performtime.Sleep
on e.g. eachWrite
to better represent my case of flushing often on a real TCP connection. The problem withPipe
here is that it doesn't really useFlush
, it simply reads/writes on demand.p2p.Conn
over something pipe-like with this latency emulation would probably be a more representative benchmark, butio.Pipe
won't do in this case, we'd deadlock. djherbis/nio has a nice pipe that'd probably do, but adding dependency for a benchmark seems to be an overkill. So, I decided to avoid benchmark changes at the moment, but would love to hear your opinion on this. If you find the proposed approach reasonable, I can do it.