Skip to content

perf(gamemessage): Replace GameMessageArgument linked lists with vectors #2700

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
RikuAnt wants to merge 2 commits into
base: main
Choose a base branch
from
+53 −141
Open

perf(gamemessage): Replace GameMessageArgument linked lists with vectors #2700

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
6c8bf63
refactor: migrate GameMessage arguments from linked list to vector (#…
RikuAnt Jun 4, 2026
e09c51c
refactor: mirror Zero Hour changes to generals (#2630)
RikuAnt Jun 4, 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
5 changes: 2 additions & 3 deletions Core/GameEngine/Include/GameNetwork/NetCommandMsg.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -33,6 +33,7 @@
#include "GameNetwork/NetPacketStructs.h"
#include "Common/UnicodeString.h"

class GameMessageArgument;
Comment thread
Caball009 marked this conversation as resolved.
class NetCommandRef;

//-----------------------------------------------------------------------------
Expand Down Expand Up @@ -179,10 +180,8 @@ class NetGameCommandMsg : public NetCommandMsgT<NetPacketGameCommand, SmallNetPa
virtual Select getSmallNetPacketSelect() const override;

protected:
Int m_numArgs;
Int m_argSize;
GameMessage::Type m_type;
GameMessageArgument *m_argList, *m_argTail;
std::vector<GameMessageArgument*> m_argList;
};

//-----------------------------------------------------------------------------
Expand Down
43 changes: 13 additions & 30 deletions Core/GameEngine/Source/GameNetwork/NetCommandMsg.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -88,12 +88,8 @@ Int NetCommandMsg::getSortNumber() const {
* Constructor with no argument, sets everything to default values.
*/
NetGameCommandMsg::NetGameCommandMsg() {
m_argSize = 0;
m_numArgs = 0;
m_type = (GameMessage::Type)0;
m_commandType = NETCOMMANDTYPE_GAMECOMMAND;
m_argList = nullptr;
m_argTail = nullptr;
}

/**
Expand All @@ -102,23 +98,24 @@ NetGameCommandMsg::NetGameCommandMsg() {
*/
NetGameCommandMsg::NetGameCommandMsg(GameMessage *msg) {
m_commandType = NETCOMMANDTYPE_GAMECOMMAND;

m_type = msg->getType();
Int count = msg->getArgumentCount();
for (Int i = 0; i < count; ++i) {
addArgument(msg->getArgumentDataType(i), *(msg->getArgument(i)));

const size_t argsCount = msg->getArgumentCount();
m_argList.reserve(argsCount);

for (size_t i = 0; i < argsCount; ++i) {
GameMessageArgumentDataType argType = msg->getArgumentDataType(i);
const GameMessageArgumentType* arg = msg->getArgument(i);
addArgument(argType, *arg);
}
}

/**
* Destructor
*/
NetGameCommandMsg::~NetGameCommandMsg() {
GameMessageArgument *arg = m_argList;
while (arg != nullptr) {
m_argList = m_argList->m_next;
deleteInstance(arg);
arg = m_argList;
for (size_t i = 0; i < m_argList.size(); ++i) {
deleteInstance(m_argList[i]);
}
}

Expand All @@ -127,21 +124,10 @@ NetGameCommandMsg::~NetGameCommandMsg() {
*/
void NetGameCommandMsg::addArgument(const GameMessageArgumentDataType type, GameMessageArgumentType arg)
Comment thread
xezon marked this conversation as resolved.
{
if (m_argTail == nullptr) {
m_argList = newInstance(GameMessageArgument);
m_argTail = m_argList;
m_argList->m_data = arg;
m_argList->m_type = type;
m_argList->m_next = nullptr;
return;
}

GameMessageArgument *newArg = newInstance(GameMessageArgument);
newArg->m_data = arg;
newArg->m_type = type;
newArg->m_next = nullptr;
m_argTail->m_next = newArg;
m_argTail = newArg;
m_argList.push_back(newArg);
}

// here's where we figure out which slot corresponds to which player
Expand Down Expand Up @@ -171,9 +157,8 @@ GameMessage *NetGameCommandMsg::constructGameMessage() const
name.format("player%d", getPlayerID());
retval->friend_setPlayerIndex( ThePlayerList->findPlayerWithNameKey(TheNameKeyGenerator->nameToKey(name))->getPlayerIndex());

GameMessageArgument *arg = m_argList;
while (arg != nullptr) {

for (size_t i = 0; i < m_argList.size(); ++i) {
const GameMessageArgument* arg = m_argList[i];
switch (arg->m_type) {

case ARGUMENTDATATYPE_INTEGER:
Expand Down Expand Up @@ -211,8 +196,6 @@ GameMessage *NetGameCommandMsg::constructGameMessage() const
break;

}

arg = arg->m_next;
}
return retval;
}
Expand Down
9 changes: 2 additions & 7 deletions Generals/Code/GameEngine/Include/Common/MessageStream.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -82,7 +82,6 @@ class GameMessageArgument : public MemoryPoolObject
{
MEMORY_POOL_GLUE_WITH_USERLOOKUP_CREATE(GameMessageArgument, "GameMessageArgument")
public:
GameMessageArgument* m_next; ///< The next argument
GameMessageArgumentType m_data; ///< The data storage of an argument
GameMessageArgumentDataType m_type; ///< The type of the argument.
};
Expand Down Expand Up @@ -637,7 +636,6 @@ class GameMessage : public MemoryPoolObject
GameMessage *prev() { return m_prev; } ///< Return prev message in the stream

Type getType() const { return m_type; } ///< Return the message type
UnsignedByte getArgumentCount() const { return m_argCount; } ///< Return the number of arguments for this msg

const char *getCommandAsString() const; ///< returns a string representation of the command type.
static const char *getCommandTypeAsString(GameMessage::Type t);
Expand All @@ -660,8 +658,8 @@ class GameMessage : public MemoryPoolObject

/**
* Return the given argument union.
* @todo This should be a more list-like interface. Very inefficient.
*/
UnsignedByte getArgumentCount() const { return static_cast<UnsignedByte>(m_argList.size()); }
Copy link
Copy Markdown

@xezon xezon Jun 5, 2026

Choose a reason for hiding this comment

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

Bot is complaining about the 255 limit assert. Fix:

return std::min<UnsignedByte>(255, m_argList.size());

Copy link
Copy Markdown
Author

@RikuAnt RikuAnt Jun 5, 2026

Choose a reason for hiding this comment

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

The bot is hallucinating, this is the exact behaviour as it was before (UnsignedByte counter that would wrap back to 0), but with actual debug crash if it actually can happen.

Using min logic would potentially mask the issue more when it happens, the current logic would keep it more obvious. The current functionality is broken in any case, as we don't handle the more than 255 arguments case at all.

Also do we want to keep retail compatibility? I think this would cause mismatch with retail as the message args would end up differing

Copy link
Copy Markdown

@xezon xezon Jun 5, 2026

Choose a reason for hiding this comment

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

There is no message that has anywhere near 255 arguments. So it is impossible to break retail compat with this.

I think the complaint is valid. If the value wraps, it better not lose visibility to all arguments, but simply be unable to use the excess ones beyond 255. The assert will highlight that too many arguments are added.

Copy link
Copy Markdown
Author

@RikuAnt RikuAnt Jun 5, 2026

Choose a reason for hiding this comment

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

Given that no existing message type can approach 255 arguments, the min clamping would never actually do anything. The debug crash in allocArg() already covers the case if that assumption ever breaks during development, so I think that's sufficient. The min doesn't fix the root cause either - if too many args were ever added, the message would still be corrupt, just silently. I wouldn't make this change.

Copy link
Copy Markdown

@xezon xezon Jun 5, 2026

Choose a reason for hiding this comment

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

ok

const GameMessageArgumentType *getArgument( Int argIndex ) const;
GameMessageArgumentDataType getArgumentDataType( Int argIndex ) const;

Expand All @@ -681,10 +679,7 @@ class GameMessage : public MemoryPoolObject

Int m_playerIndex; ///< The Player who issued the command

/// @todo If a GameMessage needs more than 255 arguments, it needs to be split up into multiple GameMessage's.
UnsignedByte m_argCount; ///< The number of arguments of this message

GameMessageArgument *m_argList, *m_argTail; ///< This message's arguments
std::vector<GameMessageArgument*> m_argList; ///< This message's arguments

/// allocate a new argument, add it to list, return pointer to its data
GameMessageArgument *allocArg();
Expand Down
49 changes: 11 additions & 38 deletions Generals/Code/GameEngine/Source/Common/MessageStream.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -56,9 +56,6 @@ GameMessage::GameMessage( GameMessage::Type type )
{
m_playerIndex = ThePlayerList->getLocalPlayer()->getPlayerIndex();
m_type = type;
m_argList = nullptr;
m_argTail = nullptr;
m_argCount = 0;
m_list = nullptr;
}

Expand All @@ -69,12 +66,8 @@ GameMessage::GameMessage( GameMessage::Type type )
GameMessage::~GameMessage()
{
// free all arguments
GameMessageArgument *arg, *nextArg;

for( arg = m_argList; arg; arg=nextArg )
{
nextArg = arg->m_next;
deleteInstance(arg);
for( size_t i = 0; i < m_argList.size(); ++i ) {
deleteInstance(m_argList[i]);
}

// detach message from list
Expand All @@ -84,14 +77,11 @@ GameMessage::~GameMessage()

/**
* Return the given argument union.
* @todo This should be a more list-like interface. Very inefficient.
*/
const GameMessageArgumentType *GameMessage::getArgument( Int argIndex ) const
{
int i=0;
for( GameMessageArgument *a = m_argList; a; a=a->m_next, i++ )
if (i == argIndex)
return &a->m_data;
if (static_cast<size_t>(argIndex) < m_argList.size())
return &m_argList[argIndex]->m_data;

DEBUG_CRASH(("argument not found"));
static const GameMessageArgumentType zero = { 0 };
Expand All @@ -103,17 +93,9 @@ const GameMessageArgumentType *GameMessage::getArgument( Int argIndex ) const
*/
GameMessageArgumentDataType GameMessage::getArgumentDataType( Int argIndex ) const
{
if (argIndex >= m_argCount) {
return ARGUMENTDATATYPE_UNKNOWN;
}
int i=0;
GameMessageArgument *a = m_argList;
for (; a && (i < argIndex); a=a->m_next, ++i );
if (static_cast<size_t>(argIndex) < m_argList.size())
return m_argList[argIndex]->m_type;

if (a != nullptr)
{
return a->m_type;
}
return ARGUMENTDATATYPE_UNKNOWN;
}

Expand All @@ -124,21 +106,12 @@ GameMessageArgument *GameMessage::allocArg()
{
// allocate a new argument
GameMessageArgument *arg = newInstance(GameMessageArgument);
m_argList.push_back(arg);

// add to end of argument list
if (m_argTail)
m_argTail->m_next = arg;
else
{
m_argList = arg;
m_argTail = arg;
}

arg->m_next = nullptr;
m_argTail = arg;

m_argCount++;

DEBUG_ASSERTCRASH(
m_argList.size() <= 255,
("If a GameMessage needs more than 255 arguments, it needs to be split up into multiple GameMessage's.")
);
return arg;
}

Expand Down
15 changes: 6 additions & 9 deletions Generals/Code/GameEngine/Source/Common/Recorder.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -785,15 +785,12 @@ void RecorderClass::writeToFile(GameMessage * msg) {
argType = argType->getNext();
}

// UnsignedByte lasttype = (UnsignedByte)ARGUMENTDATATYPE_UNKNOWN;
Int numArgs = msg->getArgumentCount();
for (Int i = 0; i < numArgs; ++i) {
// UnsignedByte type = (UnsignedByte)(msg->getArgumentDataType(i));
// if (lasttype != type) {
// fwrite(&type, sizeof(type), 1, m_file);
// lasttype = type;
// }
writeArgument(msg->getArgumentDataType(i), *(msg->getArgument(i)));
const size_t argsCount = msg->getArgumentCount();

for (size_t i = 0; i < argsCount; ++i) {
GameMessageArgumentDataType argType = msg->getArgumentDataType(i);
const GameMessageArgumentType* arg = msg->getArgument(i);
writeArgument(argType, *arg);
}

deleteInstance(parser);
Expand Down
9 changes: 2 additions & 7 deletions GeneralsMD/Code/GameEngine/Include/Common/MessageStream.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -82,7 +82,6 @@ class GameMessageArgument : public MemoryPoolObject
{
MEMORY_POOL_GLUE_WITH_USERLOOKUP_CREATE(GameMessageArgument, "GameMessageArgument")
public:
GameMessageArgument* m_next; ///< The next argument
GameMessageArgumentType m_data; ///< The data storage of an argument
GameMessageArgumentDataType m_type; ///< The type of the argument.
};
Expand Down Expand Up @@ -637,7 +636,6 @@ class GameMessage : public MemoryPoolObject
GameMessage *prev() { return m_prev; } ///< Return prev message in the stream

Type getType() const { return m_type; } ///< Return the message type
UnsignedByte getArgumentCount() const { return m_argCount; } ///< Return the number of arguments for this msg

const char *getCommandAsString() const; ///< returns a string representation of the command type.
static const char *getCommandTypeAsString(GameMessage::Type t);
Expand All @@ -660,8 +658,8 @@ class GameMessage : public MemoryPoolObject

/**
* Return the given argument union.
* @todo This should be a more list-like interface. Very inefficient.
*/
UnsignedByte getArgumentCount() const { return static_cast<UnsignedByte>(m_argList.size()); }
const GameMessageArgumentType *getArgument( Int argIndex ) const;
GameMessageArgumentDataType getArgumentDataType( Int argIndex ) const;

Expand All @@ -681,10 +679,7 @@ class GameMessage : public MemoryPoolObject

Int m_playerIndex; ///< The Player who issued the command

/// @todo If a GameMessage needs more than 255 arguments, it needs to be split up into multiple GameMessage's.
UnsignedByte m_argCount; ///< The number of arguments of this message

GameMessageArgument *m_argList, *m_argTail; ///< This message's arguments
std::vector<GameMessageArgument*> m_argList; ///< This message's arguments

/// allocate a new argument, add it to list, return pointer to its data
GameMessageArgument *allocArg();
Expand Down
49 changes: 11 additions & 38 deletions GeneralsMD/Code/GameEngine/Source/Common/MessageStream.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -56,9 +56,6 @@ GameMessage::GameMessage( GameMessage::Type type )
{
m_playerIndex = ThePlayerList->getLocalPlayer()->getPlayerIndex();
m_type = type;
m_argList = nullptr;
m_argTail = nullptr;
m_argCount = 0;
m_list = nullptr;
}

Expand All @@ -69,12 +66,8 @@ GameMessage::GameMessage( GameMessage::Type type )
GameMessage::~GameMessage()
{
// free all arguments
GameMessageArgument *arg, *nextArg;

for( arg = m_argList; arg; arg=nextArg )
{
nextArg = arg->m_next;
deleteInstance(arg);
for( size_t i = 0; i < m_argList.size(); ++i ) {
deleteInstance(m_argList[i]);
}

// detach message from list
Expand All @@ -84,14 +77,11 @@ GameMessage::~GameMessage()

/**
* Return the given argument union.
* @todo This should be a more list-like interface. Very inefficient.
*/
const GameMessageArgumentType *GameMessage::getArgument( Int argIndex ) const
{
int i=0;
for( GameMessageArgument *a = m_argList; a; a=a->m_next, i++ )
if (i == argIndex)
return &a->m_data;
if (static_cast<size_t>(argIndex) < m_argList.size())
return &m_argList[argIndex]->m_data;

DEBUG_CRASH(("argument not found"));
static const GameMessageArgumentType zero = { 0 };
Expand All @@ -103,17 +93,9 @@ const GameMessageArgumentType *GameMessage::getArgument( Int argIndex ) const
*/
GameMessageArgumentDataType GameMessage::getArgumentDataType( Int argIndex ) const
{
if (argIndex >= m_argCount) {
return ARGUMENTDATATYPE_UNKNOWN;
}
int i=0;
GameMessageArgument *a = m_argList;
for (; a && (i < argIndex); a=a->m_next, ++i );
if (static_cast<size_t>(argIndex) < m_argList.size())
return m_argList[argIndex]->m_type;

if (a != nullptr)
{
return a->m_type;
}
return ARGUMENTDATATYPE_UNKNOWN;
}

Expand All @@ -124,21 +106,12 @@ GameMessageArgument *GameMessage::allocArg()
{
// allocate a new argument
GameMessageArgument *arg = newInstance(GameMessageArgument);
m_argList.push_back(arg);

// add to end of argument list
if (m_argTail)
m_argTail->m_next = arg;
else
{
m_argList = arg;
m_argTail = arg;
}

arg->m_next = nullptr;
m_argTail = arg;

m_argCount++;

DEBUG_ASSERTCRASH(
m_argList.size() <= 255,
("If a GameMessage needs more than 255 arguments, it needs to be split up into multiple GameMessage's.")
);
return arg;
}

Expand Down
Loading
Loading