Skip to content

refactor(network): Remove dynamic allocations for NetPacket #2866

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.

Sign up for GitHub

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

Open
Caball009 wants to merge 4 commits into
base: main
Choose a base branch
from
Open

refactor(network): Remove dynamic allocations for NetPacket #2866

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
94cf9ae
Refactored 'ConstructBigCommandPacketList'.
Caball009 Jul 13, 2026
5c00a6c
Removed memory pool for NetPacket.
Caball009 Jul 24, 2026
ad51ba1
refactor: moved the initialization of 'commandList'.
Caball009 Jul 24, 2026
27c0487
Added nullptr check.
Caball009 Jul 24, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
14 changes: 4 additions & 10 deletions Core/GameEngine/Include/GameNetwork/NetPacket.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -40,18 +40,12 @@ be specialized code.
#include "Common/MessageStream.h"
#include "Common/GameMemory.h"

class NetPacket;

typedef std::list<NetPacket *> NetPacketList;
typedef std::list<NetPacket *>::iterator NetPacketListIter;

class NetPacket : public MemoryPoolObject
class NetPacket
{
MEMORY_POOL_GLUE_WITH_USERLOOKUP_CREATE(NetPacket, "NetPacket")
public:
NetPacket();
NetPacket(TransportMessage *msg);
//virtual ~NetPacket();
NetPacket(const TransportMessage& msg);
~NetPacket();

void init();
void reset();
Expand All @@ -62,7 +56,7 @@ class NetPacket : public MemoryPoolObject
NetCommandList *getCommandList();

static NetCommandRef *ConstructNetCommandMsgFromRawData(const UnsignedByte *data, UnsignedInt dataLength);
static NetPacketList ConstructBigCommandPacketList(NetCommandRef *ref);
static NetCommandList *ConstructBigCommandList(NetCommandRef *ref);

UnsignedByte *getData();
Int getLength();
Expand Down
1 change: 0 additions & 1 deletion Core/GameEngine/Source/Common/System/GameMemoryInitPools_Generals.inl
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -522,7 +522,6 @@ static PoolSizeRec PoolSizes[] =
{ "Campaign", 32, 32 },
{ "Mission", 32, 32 },
{ "ModalWindow", 32, 32 },
{ "NetPacket", 32, 32 },
{ "AISideInfo", 32, 32 },
{ "AISideBuildList", 32, 32 },
{ "MetaMapRec", 256, 32 },
Expand Down
1 change: 0 additions & 1 deletion Core/GameEngine/Source/Common/System/GameMemoryInitPools_GeneralsMD.inl
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -518,7 +518,6 @@ static PoolSizeRec PoolSizes[] =
{ "Campaign", 32, 32 },
{ "Mission", 88, 32 },
{ "ModalWindow", 32, 32 },
{ "NetPacket", 32, 32 },
{ "AISideInfo", 32, 32 },
{ "AISideBuildList", 32, 32 },
{ "MetaMapRec", 256, 32 },
Expand Down
62 changes: 19 additions & 43 deletions Core/GameEngine/Source/GameNetwork/Connection.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -131,59 +131,38 @@ User * Connection::getUser() {
* The relay mostly has to do with the packet router.
*/
void Connection::sendNetCommandMsg(NetCommandMsg *msg, UnsignedByte relay) {
static NetPacket *packet = nullptr;

// this is done so we don't have to allocate and delete a packet every time we send a message.
if (packet == nullptr) {
packet = newInstance(NetPacket);
}


if (m_isQuitting)
return;

if (m_netCommandList != nullptr) {
NetPacket packet;

// check to see if this command will fit in a packet. If not, we need to split it up.
// we are splitting up the command here so that the retry logic will not try to
// resend the ENTIRE command (i.e. multiple packets work of data) and only do the retry
// one wrapper command at a time.
packet->reset();

NetCommandRef *tempref = NEW_NETCOMMANDREF(msg);
if (packet.addCommand(tempref)) {
deleteInstance(tempref);
tempref = nullptr;
} else {
tempref->setRelay(relay);

Bool msgFits = packet->addCommand(tempref);
deleteInstance(tempref); // delete the temporary reference.
tempref = nullptr;

if (!msgFits) {
NetCommandRef *origref = NEW_NETCOMMANDREF(msg);
origref->setRelay(relay);
// the message doesn't fit in a single packet, need to split it up.
NetPacketList packetList = NetPacket::ConstructBigCommandPacketList(origref);
NetPacketListIter tempPacketPtr = packetList.begin();

while (tempPacketPtr != packetList.end()) {
NetPacket *tempPacket = (*tempPacketPtr);

NetCommandList *list = tempPacket->getCommandList();
NetCommandRef *ref1 = list->getFirstMessage();
while (ref1 != nullptr) {
NetCommandRef *ref2 = m_netCommandList->addMessage(ref1->getCommand());
ref2->setRelay(relay);

ref1 = ref1->getNext();
if (NetCommandList* list = NetPacket::ConstructBigCommandList(tempref)) {
for (NetCommandRef* ref1 = list->getFirstMessage(); ref1 != nullptr; ref1 = ref1->getNext()) {
if (NetCommandRef* ref2 = m_netCommandList->addMessage(ref1->getCommand())) {
ref2->setRelay(relay);
}
}
Comment thread
greptile-apps[bot] marked this conversation as resolved.

deleteInstance(tempPacket);
tempPacket = nullptr;
++tempPacketPtr;

deleteInstance(list);
list = nullptr;
}

deleteInstance(origref);
origref = nullptr;
deleteInstance(tempref);
tempref = nullptr;

return;
}
Expand Down Expand Up @@ -270,9 +249,8 @@ UnsignedInt Connection::doSend() {
NetCommandRef *msg = m_netCommandList->getFirstMessage();

while ((msg != nullptr) && couldQueue) {
NetPacket *packet = newInstance(NetPacket);
packet->init();
packet->setAddress(m_user->GetIPAddr(), m_user->GetPort());
NetPacket packet;
packet.setAddress(m_user->GetIPAddr(), m_user->GetPort());

Bool notDone = TRUE;

Expand All @@ -283,7 +261,7 @@ UnsignedInt Connection::doSend() {
time_t timeLastSent = msg->getTimeLastSent();

if (((curtime - timeLastSent) > m_retryTime) || (timeLastSent == -1)) {
notDone = packet->addCommand(msg);
notDone = packet.addCommand(msg);
if (notDone) {
// the msg command was added to the packet.
if (CommandRequiresAck(msg->getCommand())) {
Expand All @@ -308,14 +286,12 @@ UnsignedInt Connection::doSend() {
++numpackets;

/// @todo Make the act of giving the transport object a packet to send more efficient. Make the transport take a NetPacket object rather than the raw data, thus avoiding an extra memcpy.
if (packet->getNumCommands() > 0) {
if (packet.getNumCommands() > 0) {
// If the packet actually has any information to give, give it to the transport object
// for transmission.
couldQueue = m_transport->queueSend(packet->getAddr(), packet->getPort(), packet->getData(), packet->getLength());
couldQueue = m_transport->queueSend(packet.getAddr(), packet.getPort(), packet.getData(), packet.getLength());
m_lastTimeSent = curtime;
}

deleteInstance(packet); // delete the packet now that we're done with it.
}

return numpackets;
Expand Down
18 changes: 4 additions & 14 deletions Core/GameEngine/Source/GameNetwork/ConnectionManager.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -459,20 +459,18 @@ void ConnectionManager::destroyGameMessages() {
* assumption that a command will only be relayed once.
*/
void ConnectionManager::doRelay() {
NetPacket *packet = nullptr;

for (size_t i = 0; i < ARRAY_SIZE(m_transport->m_inBuffer); ++i) {
if (m_transport->m_inBuffer[i].length > 0) {
// This transport buffer has yet to be processed.

// make a NetPacket out of this data so it can be broken up into individual commands.
packet = newInstance(NetPacket)(&(m_transport->m_inBuffer[i]));
NetPacket packet(m_transport->m_inBuffer[i]);

//DEBUG_LOG(("ConnectionManager::doRelay() - got a packet with %d commands", packet->getNumCommands()));
//LOGBUFFER( packet->getData(), packet->getLength() );
//DEBUG_LOG(("ConnectionManager::doRelay() - got a packet with %d commands", packet.getNumCommands()));
//LOGBUFFER( packet.getData(), packet.getLength() );

// Get the command list from the packet.
NetCommandList *cmdList = packet->getCommandList();
NetCommandList *cmdList = packet.getCommandList();

// Iterate through the commands in this packet and send them to the proper connections.
for (NetCommandRef* cmd = cmdList->getFirstMessage(); cmd; cmd = cmd->getNext()) {
Expand All @@ -487,10 +485,6 @@ void ConnectionManager::doRelay() {
}
}

// Delete this packet since we won't be needing it anymore.
deleteInstance(packet);
packet = nullptr;

deleteInstance(cmdList);
cmdList = nullptr;

Expand All @@ -511,10 +505,6 @@ void ConnectionManager::doRelay() {
}
}

// Delete this packet since we won't be needing it anymore.
deleteInstance(packet);
packet = nullptr;

deleteInstance(cmdList);
cmdList = nullptr;
}
Expand Down
62 changes: 28 additions & 34 deletions Core/GameEngine/Source/GameNetwork/NetPacket.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -71,25 +71,22 @@ NetCommandRef *NetPacket::ConstructNetCommandMsgFromRawData(const UnsignedByte *
return ref;
}

NetPacketList NetPacket::ConstructBigCommandPacketList(NetCommandRef *ref) {
NetCommandList *NetPacket::ConstructBigCommandList(NetCommandRef *ref)
{
// if we don't have a unique command ID, then the wrapped command cannot
// be identified. Therefore don't allow commands without a unique ID to
// be wrapped.
NetCommandMsg *msg = ref->getCommand();

if (!DoesCommandRequireACommandID(msg->getNetCommandType())) {
DEBUG_CRASH(("Trying to wrap a command that doesn't have a unique command ID"));
return NetPacketList();
return nullptr;
}

UnsignedInt bufferSize = GetBufferSizeNeededForCommand(msg); // need to implement. I have a drinking problem.
UnsignedByte *bigPacketData = nullptr;

NetPacketList packetList;
UnsignedByte* bigPacketData = NEW UnsignedByte[bufferSize];

// create the buffer for the huge message and fill the buffer with that message.
UnsignedInt bigPacketCurrentOffset = 0;
bigPacketData = NEW UnsignedByte[bufferSize];
ref->getCommand()->copyBytesForNetPacket(bigPacketData, *ref);

// create the wrapper command message we'll be using.
Expand All @@ -102,55 +99,51 @@ NetPacketList NetPacket::ConstructBigCommandPacketList(NetCommandRef *ref) {
if ((bufferSize % commandSizePerPacket) > 0) {
++numChunks;
}

NetCommandList* commandList = newInstance(NetCommandList);

UnsignedInt currentChunk = 0;
UnsignedInt bigPacketCurrentOffset = 0;

// create the packets and the wrapper messages.
while (currentChunk < numChunks) {
NetPacket *packet = newInstance(NetPacket);

UnsignedInt dataSizeThisPacket = commandSizePerPacket;
if ((bufferSize - bigPacketCurrentOffset) < dataSizeThisPacket) {
dataSizeThisPacket = bufferSize - bigPacketCurrentOffset;
UnsignedInt dataSizeThisChunk = commandSizePerPacket;
if ((bufferSize - bigPacketCurrentOffset) < dataSizeThisChunk) {
dataSizeThisChunk = bufferSize - bigPacketCurrentOffset;
}
NetCommandDataChunk bigPacket(dataSizeThisPacket);
memcpy(bigPacket.data(), bigPacketData + bigPacketCurrentOffset, bigPacket.size());

NetCommandDataChunk chunkData(dataSizeThisChunk);
memcpy(chunkData.data(), bigPacketData + bigPacketCurrentOffset, chunkData.size());

if (DoesCommandRequireACommandID(wrapperMsg->getNetCommandType())) {
wrapperMsg->setID(GenerateNextCommandID());
}

wrapperMsg->setPlayerID(msg->getPlayerID());
wrapperMsg->setExecutionFrame(msg->getExecutionFrame());

wrapperMsg->setChunkNumber(currentChunk);
wrapperMsg->setNumChunks(numChunks);
wrapperMsg->setDataOffset(bigPacketCurrentOffset);
wrapperMsg->setData(bigPacket);
wrapperMsg->setData(chunkData);
wrapperMsg->setTotalDataLength(bufferSize);
wrapperMsg->setWrappedCommandID(msg->getID());

bigPacketCurrentOffset += dataSizeThisPacket;

NetCommandRef *ref = NEW_NETCOMMANDREF(wrapperMsg);
ref->setRelay(ref->getRelay());

if (packet->addCommand(ref) == FALSE) {
DEBUG_LOG_LEVEL(DEBUG_LEVEL_NET, ("NetPacket::BeginBigCommandPacketList - failed to add a wrapper command to the packet")); // I still have a drinking problem.
}
bigPacketCurrentOffset += dataSizeThisChunk;

packetList.push_back(packet);

deleteInstance(ref);
ref = nullptr;
NetCommandRef* chunkRef = NEW_NETCOMMANDREF(wrapperMsg);
chunkRef->setRelay(ref->getRelay());

commandList->addMessage(chunkRef);
++currentChunk;
}
Comment on lines 108 to 138

@greptile-apps greptile-apps Bot Jul 24, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P1 All chunk refs share the same mutable wrapperMsg — big commands silently truncated to one chunk

Every chunkRef created in the loop points to the same wrapperMsg object. Because wrapperMsg is mutated in place on each iteration (new command ID, new chunk number, new data), all refs that were already inserted into commandList immediately observe the new state. The deduplication logic in addMessage(NetCommandRef*&) compares m_lastMessageInserted->getCommand() and msg->getCommand(), but both are literally the same pointer, so every chunk after the first is discarded as a "duplicate" and its chunkRef is deleted. The list ends up with a single entry holding the last chunk's data and the last generated command ID.

The old code worked because addCommand serialized each chunk's state into a raw byte buffer inside a per-chunk NetPacket, and the caller then called getCommandList() to deserialize independent NetCommandMsg objects from those bytes. The new path eliminates that serialization round-trip, but must instead allocate a distinct NetWrapperCommandMsg per chunk (so each ref owns its own snapshot of the state) to preserve correctness.

Any game command large enough to span two or more network chunks will be silently dropped to its last piece, breaking reassembly on the receiver.

Prompt To Fix With AI 
This is a comment left during a code review.
Path: Core/GameEngine/Source/GameNetwork/NetPacket.cpp
Line: 108-138

Comment:
**All chunk refs share the same mutable `wrapperMsg` — big commands silently truncated to one chunk**

Every `chunkRef` created in the loop points to the *same* `wrapperMsg` object. Because `wrapperMsg` is mutated in place on each iteration (new command ID, new chunk number, new data), all refs that were already inserted into `commandList` immediately observe the new state. The deduplication logic in `addMessage(NetCommandRef*&)` compares `m_lastMessageInserted->getCommand()` and `msg->getCommand()`, but both are literally the same pointer, so every chunk after the first is discarded as a "duplicate" and its `chunkRef` is deleted. The list ends up with a single entry holding the last chunk's data and the last generated command ID.

The old code worked because `addCommand` serialized each chunk's state into a raw byte buffer inside a per-chunk `NetPacket`, and the caller then called `getCommandList()` to deserialize independent `NetCommandMsg` objects from those bytes. The new path eliminates that serialization round-trip, but must instead allocate a distinct `NetWrapperCommandMsg` per chunk (so each ref owns its own snapshot of the state) to preserve correctness.

Any game command large enough to span two or more network chunks will be silently dropped to its last piece, breaking reassembly on the receiver.

How can I resolve this? If you propose a fix, please make it concise.


wrapperMsg->detach();
wrapperMsg = nullptr;

delete[] bigPacketData;
bigPacketData = nullptr;

return packetList;
return commandList;
Comment thread
Caball009 marked this conversation as resolved.
}

UnsignedInt NetPacket::GetBufferSizeNeededForCommand(NetCommandMsg *msg) {
Expand All @@ -173,13 +166,14 @@ NetPacket::NetPacket() {
/**
* Constructor given raw transport data.
*/
NetPacket::NetPacket(TransportMessage *msg) {
NetPacket::NetPacket(const TransportMessage& msg) {
init();
m_packetLen = msg->length;
memcpy(m_packet, msg->data, MAX_PACKET_SIZE);

m_packetLen = msg.length;
memcpy(m_packet, msg.data, MAX_PACKET_SIZE);
m_numCommands = -1;
m_addr = msg->addr;
m_port = msg->port;
m_addr = msg.addr;
m_port = msg.port;
}

/**
Expand Down
Loading