The PR branch HEAD was 99c1af5a8f at the time of this review club meeting.
Notes
CConnman
is the class in net that opens, maintains and closes connections to peers,
manages sockets, and reads/writes messages on the network.
Within CConnman, we maintain
vNodes,
a vector of connections to other nodes. That vector is updated and accessed
by various nodes, including:
the socket handler thread,
which is responsible for reading data from the sockets into receive
buffers, and also for accepting new incoming connections.
the open connections thread,
which is responsible for opening new connections to peers on the network.
the message handler thread,
which is responsible for reading messages from the receive buffer and passing them
up to net_processing.
Since the vector can be updated by multiple threads, it is guarded by a mutex
called cs_vNodes.
For operations that are done on each connection in turn (e.g. reading from each socket in
the socket handler thread, or passing messages to net_processing in the message handler
thread), the common pattern is to:
lock cs_vNodes
make a copy of the vNodes vector
for each CNode object, increment an internal
nRefCount
atomic counter.
release cs_vNodes
operate on each of the CNode objects in the vNodes copy in turn
decrement the nRefCount counter for each CNode
This PR proposes to extract that pattern into an
RAII object called
NodesSnapshot. It also changes the order of some logic in the socket
handler thread. The motivation is to reduce lock
contentions.
The CNode object contains a member called nRefCount. This is rather
similar to the reference counter in a shared
pointer. Why do you think
we use raw pointers to store CNode objects in vNodes instead of shared pointers?
The destructor of the NodesSnapshot class calls Release() on each CNode object
in turn. It does this without locking the cs_vNodes mutex. Is that ok? Why/why not?
The socket handler thread contains a while()
loop
that runs for the entire duration of bitcoind’s runtime (interruptNet is only
toggled to false during the shutdown sequence). For that loop not to be ‘busy’ and
peg the CPU, there should be a sleep somewhere in the loop. Where is that sleep
for the socket handler thread?
The second commit (net: don’t process newly accepted nodes in
SocketHandler()) moves the logic for accepting new connections below where
the nodes snapshot is taken. What are the consequences of doing that?
It has been claimed that this PR reduces lock contention. How would you
verify or test that?
<jnewbery> that's ok. I understand most people are doing this in their free time, so no worries if you didn't have time to review the code this week. Feel free to ask any questions as we go through it
<jnewbery> There are two parts to the PR. It's using RAII for copying the list of nodes in connman, and also changing the ordering of operations in the socket handler thread.
<larryruane> There are a bunch of places where we need to operate on every node, but while doing that (across multiple nodes), nodes can be added and deleted - this is why the pre-PR code makes a copy ... this PR makes making that copy safer and more systematic
<larryruane> thanks, so the very simple case of just declaring an object (as a local variable let's say), that's instantiating the resource (the object), and since its constructor runs, initializing it at the same time? Or is there more to it?
<jnewbery> larryruane: yes, that's exactly right. It uses the fact that in C++, whenever an object is declared, its constructor is called, and whenever that object goes out of scope, its destructor is called
<larryruane> What I've struggled with a bit at time is if there's a typedef to an `int64_t` let's say, it may not be clear from reading a declaration of a variable of that type whether it's being initialized (this is an object (class instance) versus just an int), without looking up the type
<jnewbery> the point is more about the release of the resource at the end of the object's lifetime. If we manually acquire the resource in a preocedural way, and then release the resource at the end of the function or whatever, we need to always check that there's no other way to leave that function (eg through an early exit or exception throw) that would miss that release and therefore leak the
<larryruane> I'm sorry to belabor the point, we can go on, but when I read `footype x;` I wonder if it should be `footype x{0}` or if the initialization is being taken care of
<jnewbery> The CNode object contains a member called nRefCount. This is rather similar to the reference counter in a shared pointer. Why do you think we use raw pointers to store CNode objects in vNodes instead of shared pointers?
<jnewbery> memory allocation would be very similar if it was a shared pointer and std::make_shared was used (there would also be a control block allocated, but that's not really important)
<jnewbery> and maybe 2.5: we could move to shared pointers, but it would require careful thinking about whether 2 is an issue, and there are higher priority things to work on
<jnewbery> The destructor of the NodesSnapshot class calls Release() on each CNode object in turn. It does this without locking the cs_vNodes mutex. Is that ok? Why/why not?
<jnewbery> ok, next question. The socket handler thread contains a while() loop that runs for the entire duration of bitcoind’s runtime (interruptNet is only toggled to false during the shutdown sequence). For that loop not to be ‘busy’ and peg the CPU, there should be a sleep somewhere in the loop. Where is that sleep for the socket handler thread?
<larryruane> hmm, the code at the first link (line 1386) doesn't look as expected -- it takes 3 sets as arguments, but seems to be called with `snap.Nodes()` as the first argument
<jnewbery> Next question: The second commit (net: don’t process newly accepted nodes in SocketHandler()) moves the logic for accepting new connections below where the nodes snapshot is taken. What are the consequences of doing that?
<jnewbery> I had to review this quite carefully to convince myself that it was ok. It's a little difficult to get your head around the ordering of things in the socket handler loop, and when the thread is sleeping/waiting for events on the socket
<larryruane> If the condition is false, then I expect that later WHEN the refcount decrements to zero, we would be doing these same things (the `remove` and `DeleteNode()`)
<jnewbery> larryruane: right, when the refcount drops to zero, we delete the object. If not, then we continue and try again on the next loop of thread socket handler