-
Notifications
You must be signed in to change notification settings - Fork 2.4k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
wire: only borrow/return binaryFreeList buffers at the message level #2073
Conversation
Pull Request Test Coverage Report for Build 7353202677Warning: This coverage report may be inaccurate.We've detected an issue with your CI configuration that might affect the accuracy of this pull request's coverage report.
💛 - Coveralls |
cc @ellemouton |
With this PR, combined with the rest on the v0.24 milestone, I was able to achieve a 12 hour sync over LAN! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Very nice changes and nicely split up into multiple commits.
I did as thorough a review as concentration allowed and didn't find anything wrong with the code.
The main thing I would change is to use a consistent pattern of:
buf := binarySerializer.Borrow()
defer binarySerializer.Return(buf)
everywhere to avoid a single mistake somewhere causing the buffers to all be occupied and becoming useless (which probably wouldn't show up in benchmark tests).
NOTE: This is a rebased version of #1426
The original PR body follows:
This PR optimizes the
wire
packages serialization of small buffers by minimizing the number of borrow/return round trips during message serialization. Currently thewire
package uses abinaryFreeList
from which 8-byte buffers are borrowed and returned for the purpose of serializing small integers and varints.Problem
To understand the problem, consider calling
WriteVarInt
on a number greater than0xfc
(which requires writing the discriminant and a 2, 4, or 8 byte value following).For instance, writing 20,000 will invoke
PutUint8
and thenPutUint16
. Expanding this out to examine the message passing, we see:Each
<-
requires a channelselect
, which more-or-less bears the performance implication of a mutex. This cost, in addition to need to wake up other goroutines and switch executions, imparts a significant performance penalty. In the context of block serialization, several hundred thousand of these operations may be performed.Solution
In our example above, we can improve this by only using two
<-
, one to borrow and one to return, as so:As expected, cutting the number channels sends in half cuts also cuts the latency in half, which can be seen in the benchmarks below for larger
VarInt
s,The remainder of this PR is to propagate this pattern all the way up to the top level of messages in the wire package, such that deserializing a message only incurs one borrow and one return. Any subroutines are made to conditionally borrow from the
binarySerializer
if the invoker has not provided them with a buffer, and conditionally return if they indeed were required to borrow.A good example of how these channel sends/receives can add up is in MsgTx serialization, which is now upwards of 80% faster as a result of these optimizations:
Preliminary Benchmarks
Notes
I'm still in the process of going through and adding benchmarks to top-level messages in order to guage the overall performance benefit, expect more to be added at a later point.
There are a few remaining messages which have not yet been optimized, e.g.
MsgAlert
,MsgVesion
, etc. I plan to add those as well but decided to start with the ones that were more performance critical.