Skip to content
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

Net: Massive speedup. Net locks overhaul #9441

Merged
merged 16 commits into from Jan 13, 2017

Conversation

@theuni
Copy link
Member

commented Dec 29, 2016

Depends on (includes) #9289. This is what I ultimately set out to fix with the net refactor.

In my (short) tests, it cuts network latency to ~50%. In some cases, more like 30%.

Test method: 1 fresh node (macbook) connected to a single other node (desktop). Testnet. Running until synced to block 10000. Results:

new = patched with this PR.
old = running #9289.
client / server
old / old: 100000 in 8:05
new / old: 100000 in 3:24
new / new: 100000 in 2:16

The results are very reproducible, always within a few seconds. Not only is it a nice improvement for the fresh node, but it compounds when its peer is running the updated code as well.

I had hoped to have the abstraction complete in time for 0.14, but it looks like that's unlikely at this point. For reference, it would look something more like theuni@1a6b10a.

Anyway, this is an attempt to fix the actual issue in time for 0.14, and putting off the last bit of refactor until after that. This addresses the issue observed in #9415, but also cleans up the nasty locking issues.

I labeled this WIP because there are probably still several racy bits. Quite a bit of test-writing remains.

See the individual commits for the details. tl;dr: We currently either process a peer's message or read from their socket, but never both simultaneously. The changes here remove that restriction.

src/net_processing.cpp Outdated

std::deque<CNetMessage>::iterator it = pfrom->vRecvMsg.begin();
while (!pfrom->fDisconnect && it != pfrom->vRecvMsg.end()) {
// Don't bother if send buffer is too full to respond anyway

This comment has been minimized.

Copy link
@rebroad

rebroad Dec 29, 2016

Contributor

I'm not sure if github is messing with the formatting, but this line looks unnecessarily indented.

This comment has been minimized.

Copy link
@theuni

theuni Dec 29, 2016

Author Member

Yes, the indentation was just kept to avoid creating a huge whitespace diff. This can be cleaned up in a move-only commit as a next step.

@theuni theuni force-pushed the theuni:connman-locks branch Dec 29, 2016

@theuni

This comment has been minimized.

Copy link
Member Author

commented Dec 31, 2016

@sipa I've spent all day on changing this around some and breaking this up into logical commits in order to satisfy myself (and i hope others) that it's safe. I'll push up a fresh version in a few min, and drop the WIP tag.

@theuni theuni force-pushed the theuni:connman-locks branch Dec 31, 2016

@theuni theuni changed the title WIP: Massive net speedup. Net locks overhaul. Net: Massive speedup. Net locks overhaul Dec 31, 2016

@theuni

This comment has been minimized.

Copy link
Member Author

commented Dec 31, 2016

Updated. Don't let the number of commits scare you off, the diffstat isn't too bad.

I attempted to break this up into small/simple commits, in order to slowly remove the need for cs_vRecvMsg. The last commit actually removes it.

I'm satisfied that this shouldn't be introducing any new races, as only a handful of vars were actually touched on the SocketHandler thread.

Edit: time-ordered.

@theuni theuni force-pushed the theuni:connman-locks branch Dec 31, 2016

src/net.cpp Outdated
if(notify)
condMsgProc.notify_one();
RecordBytesRecv(nBytes);
}

This comment has been minimized.

Copy link
@theuni

theuni Dec 31, 2016

Author Member

Whoops, rebase gone bad here. It ends up right in the end. Will fix.

src/net_processing.cpp Outdated

auto it = pfrom->vRecvMsg.begin();
while (!pfrom->fDisconnect && it != pfrom->vRecvMsg.end()) {
if (!pfrom->fDisconnect && it != pfrom->vRecvMsg.end()) {

This comment has been minimized.

Copy link
@theuni

theuni Dec 31, 2016

Author Member

Need to clarify in the commit message: It was almost always the case that only one message was processed in this while loop. See the break at line 2538.

This commit changes the behavior so that it's always one message processed per-loop. The only messages that show different behavior are the ones that used to "continue" the loop. I think it's perfectly reasonable to skip to the next node for processing in that case.

This comment has been minimized.

Copy link
@sipa

sipa Jan 5, 2017

Member

Can you clarify what the rationale is for switching from a ProcessMessages that processes multiple messages to just a single one? I'm not opposed to the change, but I'd like to understand what the reason for the change is.

This comment has been minimized.

Copy link
@TheBlueMatt

TheBlueMatt Jan 5, 2017

Contributor

Point is this is not a change. See #9441 (comment)

@dcousens

This comment has been minimized.

Copy link
Contributor

commented Jan 3, 2017

@theuni is this worth testing now?

@theuni

This comment has been minimized.

Copy link
Member Author

commented Jan 3, 2017

@dcousens Very much so!

src/net.cpp Outdated
int64_t nTime = GetTimeMicros();
LOCK(cs_vRecvMsg);
nRecvBytes += nBytes;
nLastRecv = nTime * 1000;

This comment has been minimized.

Copy link
@TheBlueMatt

TheBlueMatt Jan 3, 2017

Contributor

I think you meant /, not * here?

This comment has been minimized.

Copy link
@theuni

theuni Jan 3, 2017

Author Member

Heh, sure did

src/net.cpp Outdated

int64_t nTime = GetTimeMicros();
LOCK(cs_vRecvMsg);
nRecvBytes += nBytes;

This comment has been minimized.

Copy link
@TheBlueMatt

TheBlueMatt Jan 3, 2017

Contributor

It might be nice in the RPC for mapRecvBytesPerMsgCmd to add up to nRecvBytes (ie to increment this in the loop per-msg instead of after receiving (which would also mean no api change)).

This comment has been minimized.

Copy link
@theuni

theuni Jan 3, 2017

Author Member

Sure, that just means more locking. Though this is completely uncontested now, so that's not really a problem.

@TheBlueMatt

This comment has been minimized.

Copy link
Contributor

commented Jan 3, 2017

See IRC discussion:

<BlueMatt> cfields: hmmm, regarding #9441, do we really want to run the entire ProcessMessages loop (calling SendMessages potentially umpteen times) just to process multiple messages from the same node?
<BlueMatt> (ie remove the loop inside ProcessMessages and move it to ThreadProcessMessages)
<sipa> BlueMatt: i was wondering about that too
<sipa> BlueMatt: but there is hardly any (contentious) locking going on anymore in between
<BlueMatt> yea, but SendMessages.....
<sipa> Ah.
<sipa> i hadn't considered that
<sipa> that defeats the purpose
<sipa> hmm, i was about to say that it negatively impacts batching of invs and addrs
<sipa> but since we have explicit random delays for those, i don't think that's really an issue anymore
<BlueMatt> yea, I dont think it breaks anything
<BlueMatt> just repeatedly calls SendMessages to do nothing
<sipa> oh, it won't break anything
@theuni

This comment has been minimized.

Copy link
Member Author

commented Jan 3, 2017

@TheBlueMatt If I understand you correctly, as mentioned in the summary, that's the part that was cut out here for simplicity. The next set of changes would combine the loops and move them into net_processing. See here for how it looked in a previous iteration: theuni@1a6b10a#diff-eff7adeaec73a769788bb78858815c91R271

I'd be happy to add that here, but I'm afraid it adds a significant review burden to this PR.

@theuni

This comment has been minimized.

Copy link
Member Author

commented Jan 3, 2017

Also from IRC:

<BlueMatt> I assume you didnt touch cs_vSend due to https://github.com/bitcoin/bitcoin/pull/9419/commits/c214d120a363a05ba9afdccff6b4bda6e29ae7c4, cfields?
<cfields> BlueMatt: yes, cs_vSend wasn't touched because of your PR. I've just been operating under the assumption that the lock around SendMessages will be removed by one PR or another.
@theuni

This comment has been minimized.

Copy link
Member Author

commented Jan 3, 2017

Digging further into the IRC conversation above, I'd like to clarify that the interaction here between ProcessMessages and SendMessages is not changed substantially in this PR.

@TheBlueMatt had missed a subtle part of the current behavior, and maybe @sipa too, so I'd just like to explicitly point out the current break here: https://github.com/bitcoin/bitcoin/blob/master/src/net_processing.cpp#L2543, added in #3180

For the sake of processing in a round-robin fashion, we currently end up running through SendMessages multiple times while draining a node's received message queue. That behavior is far from ideal, and needs to be addressed, but is not changed here*. I have plans to work on this in next steps, but not for 0.14.

*It's only changed in the case of an invalid header, or bad message checksum.

@gmaxwell

This comment has been minimized.

Copy link
Member

commented Jan 4, 2017

cfields: hmmm, regarding #9441, do we really want to run the entire ProcessMessages loop (calling SendMessages potentially umpteen times) just to process multiple messages from the same node?
(ie remove the loop inside ProcessMessages and move it to ThreadProcessMessages)

Clearly people weren't reading PRs on this or you would have noticed that it only processes one at a time right now. I actually fixed it to process as many as it could until it encountered lock contention, then specifically pinged people for input on the effect. After no response, I unfixed it because of concern about the latency impact. Handling multiple messages at a time is good, if they're fast ones...

@sipa

This comment has been minimized.

Copy link
Member

commented Jan 4, 2017

theuni added some commits Dec 31, 2016

net: fix typo causing the wrong receive buffer size
Surprisingly this hasn't been causing me any issues while testing, probably
because it requires lots of large blocks to be flying around.

Send/Recv corks need tests!
net: make GetReceiveFloodSize public
This will be needed so that the message processor can cork incoming messages
net: only disconnect if fDisconnect has been set
These conditions are problematic to check without locking, and we shouldn't be
relying on the refcount to disconnect.
net: wait until the node is destroyed to delete its recv buffer
when vRecvMsg becomes a private buffer, it won't make sense to allow other
threads to mess with it anymore.
@sipa

This comment has been minimized.

Copy link
Member

commented Jan 4, 2017

Needs rebase.

net: set message deserialization version when it's actually time to d…
…eserialize

We'll soon no longer have access to vRecvMsg, and this is more intuitive anyway.
@morcos

This comment has been minimized.

Copy link
Member

commented Jan 12, 2017

utACK and ran on some nodes without error for several days ddac068
utACK 7b44e18

@TheBlueMatt

This comment has been minimized.

Copy link
Contributor

commented Jan 12, 2017

Dont think its actually possible due to the sizes involved, but, theoretically, PushMessage could increment nSendSize to be > nSendBufferMaxSize, set fPauseSend to true, and then call opportunistic SocketSendData, which flushed the entire buffer, but didnt reset fPauseSend. I'd suggest moving the fPauseSend update to right beside the nSendSize decrement as it is for the increment.

src/net.cpp Outdated
WakeMessageHandler();
completeMessages.splice(completeMessages.end(), pnode->vRecvMsg, pnode->vRecvMsg.begin(), it);
if (!QueueReceivedMessages(pnode, std::move(completeMessages), nSizeAdded))
pnode->fPauseRecv = true;

This comment has been minimized.

Copy link
@TheBlueMatt

TheBlueMatt Jan 12, 2017

Contributor

By moving the setting of fPauseRecv outside of the cs_vProcessMsg lock you've introduced a race where the message processing thread may have already taken the next message and set fPauseRecv to false before this line (seems pretty unlikely, but its not unheard of). I'd suggest simply dropping this commit.

This comment has been minimized.

Copy link
@theuni

theuni Jan 12, 2017

Author Member

Good catch!

@TheBlueMatt

This comment has been minimized.

Copy link
Contributor

commented Jan 12, 2017

utACK 7b44e18 if 975fc9d is reverted and the fPauseSend updates are all placed directly beside the nSendSize updates.

@TheBlueMatt

This comment has been minimized.

Copy link
Contributor

commented Jan 13, 2017

utACK 0168bea (+/- squash-needed).

theuni added some commits Dec 31, 2016

net: handle message accounting in ReceiveMsgBytes
This allows locking to be pushed down to only where it's needed

Also reuse the current time rather than checking multiple times.
net: Add a simple function for waking the message handler
This may be used publicly in the future
net: rework the way that the messagehandler sleeps
In order to sleep accurately, the message handler needs to know if _any_ node
has more processing that it should do before the entire thread sleeps.

Rather than returning a value that represents whether ProcessMessages
encountered a message that should trigger a disconnnect, interpret the return
value as whether or not that node has more work to do.

Also, use a global fProcessWake value that can be set by other threads,
which takes precedence (for one cycle) over the messagehandler's decision.

Note that the previous behavior was to only process one message per loop
(except in the case of a bad checksum or invalid header). That was changed in
PR #3180.

The only change here in that regard is that the current node now falls to the
back of the processing queue for the bad checksum/invalid header cases.
net: add a new message queue for the message processor
This separates the storage of messages from the net and queued messages for
processing, allowing the locks to be split.
net: add a flag to indicate when a node's process queue is full
Messages are dumped very quickly from the socket handler to the processor, so
it's the depth of the processing queue that's interesting.

The socket handler checks the process queue's size during the brief message
hand-off and pauses if necessary, and the processor possibly unpauses each time
a message is popped off of its queue.
net: add a flag to indicate when a node's send buffer is full
Similar to the recv flag, but this one indicates whether or not the net's send
buffer is full.

The socket handler checks the send queue when a new message is added and pauses
if necessary, and possibly unpauses after each message is drained from its buffer.
net: remove cs_vRecvMsg
vRecvMsg is now only touched by the socket handler thread.

The accounting vars (nRecvBytes/nLastRecv/mapRecvBytesPerMsgCmd) are also
only used by the socket handler thread, with the exception of queries from
rpc/gui. These accesses are not threadsafe, but they never were. This needs to
be addressed separately.

Also, update comment describing data flow
@theuni

This comment has been minimized.

Copy link
Member Author

commented Jan 13, 2017

Squashed, and I believe this is now merge-ready.

@sipa / @morcos See https://github.com/theuni/bitcoin/tree/connman-locks-tmp for the unsquashed version if you'd like. It's codewise-identical to e60360e.

@morcos

This comment has been minimized.

Copy link
Member

commented Jan 13, 2017

re-utACK e60360e

@TheBlueMatt

This comment has been minimized.

Copy link
Contributor

commented Jan 13, 2017

Confirmed there is no diff-tree to e60360e from my previous utACK

@sipa

This comment has been minimized.

Copy link
Member

commented Jan 13, 2017

utACK e60360e

@sipa sipa merged commit e60360e into bitcoin:master Jan 13, 2017

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details

sipa added a commit that referenced this pull request Jan 13, 2017

Merge #9441: Net: Massive speedup. Net locks overhaul
e60360e net: remove cs_vRecvMsg (Cory Fields)
991955e net: add a flag to indicate when a node's send buffer is full (Cory Fields)
c6e8a9b net: add a flag to indicate when a node's process queue is full (Cory Fields)
4d712e3 net: add a new message queue for the message processor (Cory Fields)
c5a8b1b net: rework the way that the messagehandler sleeps (Cory Fields)
c72cc88 net: remove useless comments (Cory Fields)
ef7b5ec net: Add a simple function for waking the message handler (Cory Fields)
f5c36d1 net: record bytes written before notifying the message processor (Cory Fields)
60befa3 net: handle message accounting in ReceiveMsgBytes (Cory Fields)
56212e2 net: set message deserialization version when it's actually time to deserialize (Cory Fields)
0e973d9 net: remove redundant max sendbuffer size check (Cory Fields)
6042587 net: wait until the node is destroyed to delete its recv buffer (Cory Fields)
f6315e0 net: only disconnect if fDisconnect has been set (Cory Fields)
5b4a8ac net: make GetReceiveFloodSize public (Cory Fields)
e5bcd9c net: make vRecvMsg a list so that we can use splice() (Cory Fields)
53ad9a1 net: fix typo causing the wrong receive buffer size (Cory Fields)

@fanquake fanquake moved this from In progress to Done in P2P refactor Mar 3, 2017

@dagurval dagurval referenced this pull request Jun 8, 2018

Merged

More net cherries #444

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.