Skip to content

Commit

Permalink
Browse files Browse the repository at this point in the history
Various network performance improvements (#8125)
* Optimize packet construction functions

Some of the functions that construct packets in
connection.cpp are using a const reference to get the raw
packet data to package and others use a value passed
parameter to do that. The ones that use the value passed
parameter suffer from performance hit as the rather bulky
packet data gets a temporary copy when the parameter is
passed before it lands at its final destination inside the
newly constructed packet. The unnecessary temporary copy
hurts quite badly as the underlying class (SharedBuffer)
actually allocates the space for the data in the heap.

Fix the performance hit by converting all of these value
passed parameters to const references. I believe that this
is what the author of the relevant code actually intended
to do as there is a couple of packet construction helper
functions that already use a const reference to get the
raw data.

* Optimize packet sender thread class

Most of the data sending methods of the packet sender thread
class use a value passed parameter for the packet data to be
sent. This causes the rather bulky data to be allocated on
the heap and copied, slowing the packet sending down. Convert
these parameters to const references to avoid the performance
hit.

* Optimize packet receiver thread class

The packet receiver and processor thread class has many
methods (mostly packet handlers) that receive the packed data
by value. This causes a performance hit that is actually
worse than the one caused by the packet sender methods
because the packet is first handed to the processPacket
method which looks at the packet type stored in the header
and then delegates the actual handling to one of the
handlers. Both, processPacket and all the handlers get the
packet data by value, leading to at least two unnecessary
copies of the data (with malloc and all the slow bells and
whistles of bulky classes).

As there already is a few methods that use a const reference
parameter for the packet data, convert all this value passed
packets to const references.
  • Loading branch information
osjc authored and paramat committed Apr 14, 2019
1 parent 4d2ad7c commit 007ce24
Show file tree
Hide file tree
Showing 4 changed files with 28 additions and 27 deletions.
6 changes: 3 additions & 3 deletions src/network/connection.cpp
Expand Up @@ -56,7 +56,7 @@ std::mutex log_message_mutex;

#define PING_TIMEOUT 5.0

BufferedPacket makePacket(Address &address, SharedBuffer<u8> data,
BufferedPacket makePacket(Address &address, const SharedBuffer<u8> &data,
u32 protocol_id, session_t sender_peer_id, u8 channel)
{
u32 packet_size = data.getSize() + BASE_HEADER_SIZE;
Expand Down Expand Up @@ -126,7 +126,7 @@ void makeSplitPacket(const SharedBuffer<u8> &data, u32 chunksize_max, u16 seqnum
}
}

void makeAutoSplitPacket(SharedBuffer<u8> data, u32 chunksize_max,
void makeAutoSplitPacket(const SharedBuffer<u8> &data, u32 chunksize_max,
u16 &split_seqnum, std::list<SharedBuffer<u8>> *list)
{
u32 original_header_size = 1;
Expand All @@ -140,7 +140,7 @@ void makeAutoSplitPacket(SharedBuffer<u8> data, u32 chunksize_max,
list->push_back(makeOriginalPacket(data));
}

SharedBuffer<u8> makeReliablePacket(SharedBuffer<u8> data, u16 seqnum)
SharedBuffer<u8> makeReliablePacket(const SharedBuffer<u8> &data, u16 seqnum)
{
u32 header_size = 3;
u32 packet_size = data.getSize() + header_size;
Expand Down
6 changes: 3 additions & 3 deletions src/network/connection.h
Expand Up @@ -103,16 +103,16 @@ struct BufferedPacket
};

// This adds the base headers to the data and makes a packet out of it
BufferedPacket makePacket(Address &address, SharedBuffer<u8> data,
BufferedPacket makePacket(Address &address, const SharedBuffer<u8> &data,
u32 protocol_id, session_t sender_peer_id, u8 channel);

// Depending on size, make a TYPE_ORIGINAL or TYPE_SPLIT packet
// Increments split_seqnum if a split packet is made
void makeAutoSplitPacket(SharedBuffer<u8> data, u32 chunksize_max,
void makeAutoSplitPacket(const SharedBuffer<u8> &data, u32 chunksize_max,
u16 &split_seqnum, std::list<SharedBuffer<u8>> *list);

// Add the TYPE_RELIABLE header to the data
SharedBuffer<u8> makeReliablePacket(SharedBuffer<u8> data, u16 seqnum);
SharedBuffer<u8> makeReliablePacket(const SharedBuffer<u8> &data, u16 seqnum);

struct IncomingSplitPacket
{
Expand Down
18 changes: 9 additions & 9 deletions src/network/connectionthreads.cpp
Expand Up @@ -327,7 +327,7 @@ void ConnectionSendThread::sendAsPacketReliable(BufferedPacket &p, Channel *chan
}

bool ConnectionSendThread::rawSendAsPacket(session_t peer_id, u8 channelnum,
SharedBuffer<u8> data, bool reliable)
const SharedBuffer<u8> &data, bool reliable)
{
PeerHelper peer = m_connection->getPeerNoEx(peer_id);
if (!peer) {
Expand Down Expand Up @@ -575,7 +575,7 @@ void ConnectionSendThread::disconnect_peer(session_t peer_id)
}

void ConnectionSendThread::send(session_t peer_id, u8 channelnum,
SharedBuffer<u8> data)
const SharedBuffer<u8> &data)
{
assert(channelnum < CHANNEL_COUNT); // Pre-condition

Expand Down Expand Up @@ -615,7 +615,7 @@ void ConnectionSendThread::sendReliable(ConnectionCommand &c)
peer->PutReliableSendCommand(c, m_max_packet_size);
}

void ConnectionSendThread::sendToAll(u8 channelnum, SharedBuffer<u8> data)
void ConnectionSendThread::sendToAll(u8 channelnum, const SharedBuffer<u8> &data)
{
std::list<session_t> peerids = m_connection->getPeerIDs();

Expand Down Expand Up @@ -776,7 +776,7 @@ void ConnectionSendThread::sendPackets(float dtime)
}

void ConnectionSendThread::sendAsPacket(session_t peer_id, u8 channelnum,
SharedBuffer<u8> data, bool ack)
const SharedBuffer<u8> &data, bool ack)
{
OutgoingPacket packet(peer_id, channelnum, data, false, ack);
m_outgoing_queue.push(packet);
Expand Down Expand Up @@ -1086,7 +1086,7 @@ bool ConnectionReceiveThread::checkIncomingBuffers(Channel *channel,
}

SharedBuffer<u8> ConnectionReceiveThread::processPacket(Channel *channel,
SharedBuffer<u8> packetdata, session_t peer_id, u8 channelnum, bool reliable)
const SharedBuffer<u8> &packetdata, session_t peer_id, u8 channelnum, bool reliable)
{
PeerHelper peer = m_connection->getPeerNoEx(peer_id);

Expand Down Expand Up @@ -1125,7 +1125,7 @@ const ConnectionReceiveThread::PacketTypeHandler
};

SharedBuffer<u8> ConnectionReceiveThread::handlePacketType_Control(Channel *channel,
SharedBuffer<u8> packetdata, Peer *peer, u8 channelnum, bool reliable)
const SharedBuffer<u8> &packetdata, Peer *peer, u8 channelnum, bool reliable)
{
if (packetdata.getSize() < 2)
throw InvalidIncomingDataException("packetdata.getSize() < 2");
Expand Down Expand Up @@ -1224,7 +1224,7 @@ SharedBuffer<u8> ConnectionReceiveThread::handlePacketType_Control(Channel *chan
}

SharedBuffer<u8> ConnectionReceiveThread::handlePacketType_Original(Channel *channel,
SharedBuffer<u8> packetdata, Peer *peer, u8 channelnum, bool reliable)
const SharedBuffer<u8> &packetdata, Peer *peer, u8 channelnum, bool reliable)
{
if (packetdata.getSize() <= ORIGINAL_HEADER_SIZE)
throw InvalidIncomingDataException
Expand All @@ -1238,7 +1238,7 @@ SharedBuffer<u8> ConnectionReceiveThread::handlePacketType_Original(Channel *cha
}

SharedBuffer<u8> ConnectionReceiveThread::handlePacketType_Split(Channel *channel,
SharedBuffer<u8> packetdata, Peer *peer, u8 channelnum, bool reliable)
const SharedBuffer<u8> &packetdata, Peer *peer, u8 channelnum, bool reliable)
{
Address peer_address;

Expand Down Expand Up @@ -1269,7 +1269,7 @@ SharedBuffer<u8> ConnectionReceiveThread::handlePacketType_Split(Channel *channe
}

SharedBuffer<u8> ConnectionReceiveThread::handlePacketType_Reliable(Channel *channel,
SharedBuffer<u8> packetdata, Peer *peer, u8 channelnum, bool reliable)
const SharedBuffer<u8> &packetdata, Peer *peer, u8 channelnum, bool reliable)
{
assert(channel != NULL);

Expand Down
25 changes: 13 additions & 12 deletions src/network/connectionthreads.h
Expand Up @@ -52,23 +52,23 @@ class ConnectionSendThread : public Thread
private:
void runTimeouts(float dtime);
void rawSend(const BufferedPacket &packet);
bool rawSendAsPacket(session_t peer_id, u8 channelnum, SharedBuffer<u8> data,
bool reliable);
bool rawSendAsPacket(session_t peer_id, u8 channelnum,
const SharedBuffer<u8> &data, bool reliable);

void processReliableCommand(ConnectionCommand &c);
void processNonReliableCommand(ConnectionCommand &c);
void serve(Address bind_address);
void connect(Address address);
void disconnect();
void disconnect_peer(session_t peer_id);
void send(session_t peer_id, u8 channelnum, SharedBuffer<u8> data);
void send(session_t peer_id, u8 channelnum, const SharedBuffer<u8> &data);
void sendReliable(ConnectionCommand &c);
void sendToAll(u8 channelnum, SharedBuffer<u8> data);
void sendToAll(u8 channelnum, const SharedBuffer<u8> &data);
void sendToAllReliable(ConnectionCommand &c);

void sendPackets(float dtime);

void sendAsPacket(session_t peer_id, u8 channelnum, SharedBuffer<u8> data,
void sendAsPacket(session_t peer_id, u8 channelnum, const SharedBuffer<u8> &data,
bool ack = false);

void sendAsPacketReliable(BufferedPacket &p, Channel *channel);
Expand Down Expand Up @@ -119,26 +119,27 @@ class ConnectionReceiveThread : public Thread
channelnum: channel on which the packet was sent
reliable: true if recursing into a reliable packet
*/
SharedBuffer<u8> processPacket(Channel *channel, SharedBuffer<u8> packetdata,
session_t peer_id, u8 channelnum, bool reliable);
SharedBuffer<u8> processPacket(Channel *channel,
const SharedBuffer<u8> &packetdata, session_t peer_id,
u8 channelnum, bool reliable);

SharedBuffer<u8> handlePacketType_Control(Channel *channel,
SharedBuffer<u8> packetdata, Peer *peer, u8 channelnum,
const SharedBuffer<u8> &packetdata, Peer *peer, u8 channelnum,
bool reliable);
SharedBuffer<u8> handlePacketType_Original(Channel *channel,
SharedBuffer<u8> packetdata, Peer *peer, u8 channelnum,
const SharedBuffer<u8> &packetdata, Peer *peer, u8 channelnum,
bool reliable);
SharedBuffer<u8> handlePacketType_Split(Channel *channel,
SharedBuffer<u8> packetdata, Peer *peer, u8 channelnum,
const SharedBuffer<u8> &packetdata, Peer *peer, u8 channelnum,
bool reliable);
SharedBuffer<u8> handlePacketType_Reliable(Channel *channel,
SharedBuffer<u8> packetdata, Peer *peer, u8 channelnum,
const SharedBuffer<u8> &packetdata, Peer *peer, u8 channelnum,
bool reliable);

struct PacketTypeHandler
{
SharedBuffer<u8> (ConnectionReceiveThread::*handler)(Channel *channel,
SharedBuffer<u8> packet, Peer *peer, u8 channelnum,
const SharedBuffer<u8> &packet, Peer *peer, u8 channelnum,
bool reliable);
};

Expand Down

0 comments on commit 007ce24

Please sign in to comment.