-
Notifications
You must be signed in to change notification settings - Fork 202
perf(gamemessage): Optimize argument list of GameMessage by replacing linked list with vector #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.
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
base: main
Are you sure you want to change the base?
Changes from all commits
0bf542d
572f477
f627bba
84d7681
51bd443
8e5eda3
6ca8832
be4126e
763f611
4e7c038
b06b824
2d3dc31
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -33,6 +33,7 @@ | |
| #include "GameNetwork/NetPacketStructs.h" | ||
| #include "Common/UnicodeString.h" | ||
|
|
||
| class GameMessageArgument; | ||
| class NetCommandRef; | ||
|
|
||
| //----------------------------------------------------------------------------- | ||
|
|
@@ -117,10 +118,9 @@ class NetGameCommandMsg : public NetCommandMsgT<NetPacketGameCommand, SmallNetPa | |
| virtual Select getSmallNetPacketSelect() const override; | ||
|
|
||
| protected: | ||
| Int m_numArgs; | ||
| Int m_argSize; | ||
| // TheSuperHackers @refactor RiQQ 11/05/2026 Replaced linked list with std::vector for argument storage | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think this comment can be removed because it is not useful for the next reader. |
||
| GameMessage::Type m_type; | ||
| GameMessageArgument *m_argList, *m_argTail; | ||
| std::vector<const GameMessageArgument*> m_argList; | ||
| }; | ||
|
|
||
| //----------------------------------------------------------------------------- | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -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; | ||
| } | ||
|
|
||
| /** | ||
|
|
@@ -102,23 +98,23 @@ 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 std::vector<const GameMessageArgument*>& args = msg->getArguments(); | ||
| m_argList.reserve(args.size()); | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. If you inline code of There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. How is that better? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Because There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Fair enough. I did some benchmark tests and it's tied to the cost of default construction and the vector size. It'd appear to be the better choice in this context, though. |
||
|
|
||
| for (size_t i = 0; i < args.size(); ++i) { | ||
|
Caball009 marked this conversation as resolved.
|
||
| const GameMessageArgument* arg = args[i]; | ||
| addArgument(arg->m_type, arg->m_data); | ||
| } | ||
| } | ||
|
|
||
| /** | ||
| * 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(const_cast<GameMessageArgument*>(m_argList[i])); | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Instead of const cast, make the vector element non-const. |
||
| } | ||
| } | ||
|
|
||
|
|
@@ -127,21 +123,10 @@ NetGameCommandMsg::~NetGameCommandMsg() { | |
| */ | ||
| void NetGameCommandMsg::addArgument(const GameMessageArgumentDataType type, GameMessageArgumentType arg) | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Since this function is small and only called once, you can also inline it in the constructor, which reduces code complexity. |
||
| { | ||
| 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 | ||
|
|
@@ -171,9 +156,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: | ||
|
|
@@ -211,8 +195,6 @@ GameMessage *NetGameCommandMsg::constructGameMessage() const | |
| break; | ||
|
|
||
| } | ||
|
|
||
| arg = arg->m_next; | ||
| } | ||
| return retval; | ||
| } | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -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. | ||
| }; | ||
|
|
@@ -605,7 +604,16 @@ 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 | ||
| // TheSuperHackers @refactor RiQQ 12/05/2026 Return argument count from vector size | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Remove superfluous comment |
||
| /// @todo If a GameMessage needs more than 255 arguments, it needs to be split up into multiple GameMessage's. | ||
| UnsignedByte getArgumentCount() const { | ||
| DEBUG_ASSERTCRASH( | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This assert looks misplaced in this function. One would expect this assert at the time of argument creation, not when querying the size of it. |
||
| m_argList.size() <= 255, | ||
| ("GameMessage has more than 255 arguments") | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
|
||
| ); | ||
| return static_cast<UnsignedByte>(m_argList.size()); | ||
| } | ||
| const std::vector<const GameMessageArgument*>& getArguments() const { return m_argList ; } | ||
|
|
||
| const char *getCommandAsString() const; ///< returns a string representation of the command type. | ||
| static const char *getCommandTypeAsString(GameMessage::Type t); | ||
|
|
@@ -628,7 +636,6 @@ class GameMessage : public MemoryPoolObject | |
|
|
||
| /** | ||
| * Return the given argument union. | ||
| * @todo This should be a more list-like interface. Very inefficient. | ||
| */ | ||
| const GameMessageArgumentType *getArgument( Int argIndex ) const; | ||
| GameMessageArgumentDataType getArgumentDataType( Int argIndex ) const; | ||
|
|
@@ -649,10 +656,8 @@ 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 | ||
| // TheSuperHackers @refactor RiQQ 12/05/2026 Replaced linked list with std::vector for argument storage | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think this comment can be removed because it is not useful for the next reader. |
||
| std::vector<const GameMessageArgument*> m_argList; ///< This message's arguments | ||
|
|
||
| /// allocate a new argument, add it to list, return pointer to its data | ||
| GameMessageArgument *allocArg(); | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -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. | ||
| }; | ||
|
|
@@ -639,7 +638,16 @@ 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 | ||
| // TheSuperHackers @refactor RiQQ 11/05/2026 Return argument count from vector size | ||
| /// @todo If a GameMessage needs more than 255 arguments, it needs to be split up into multiple GameMessage's. | ||
| UnsignedByte getArgumentCount() const { | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Better move this function next to |
||
| DEBUG_ASSERTCRASH( | ||
| m_argList.size() <= 255, | ||
| ("GameMessage has more than 255 arguments") | ||
| ); | ||
| return static_cast<UnsignedByte>(m_argList.size()); | ||
| } | ||
| const std::vector<const GameMessageArgument*>& getArguments() const { return m_argList ; } | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Instead of giving this function that returns internal data structure, use established |
||
|
|
||
| const char *getCommandAsString() const; ///< returns a string representation of the command type. | ||
| static const char *getCommandTypeAsString(GameMessage::Type t); | ||
|
|
@@ -662,7 +670,6 @@ class GameMessage : public MemoryPoolObject | |
|
|
||
| /** | ||
| * Return the given argument union. | ||
| * @todo This should be a more list-like interface. Very inefficient. | ||
| */ | ||
| const GameMessageArgumentType *getArgument( Int argIndex ) const; | ||
| GameMessageArgumentDataType getArgumentDataType( Int argIndex ) const; | ||
|
|
@@ -683,10 +690,8 @@ 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 | ||
| // TheSuperHackers @refactor RiQQ 11/05/2026 Replaced linked list with std::vector for argument storage | ||
| std::vector<const GameMessageArgument*> m_argList; ///< This message's arguments | ||
|
|
||
| /// allocate a new argument, add it to list, return pointer to its data | ||
| GameMessageArgument *allocArg(); | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -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; | ||
| } | ||
|
|
||
|
|
@@ -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(const_cast<GameMessageArgument*>(m_argList[i])); | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Instead of const cast, make the vector element non-const. |
||
| } | ||
|
|
||
| // detach message from list | ||
|
|
@@ -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 }; | ||
|
|
@@ -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; | ||
| } | ||
|
|
||
|
|
@@ -124,21 +106,7 @@ GameMessageArgument *GameMessage::allocArg() | |
| { | ||
| // allocate a new argument | ||
| GameMessageArgument *arg = newInstance(GameMessageArgument); | ||
|
|
||
| // 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++; | ||
|
|
||
| m_argList.push_back(arg); | ||
| return arg; | ||
| } | ||
|
|
||
|
|
||
Uh oh!
There was an error while loading. Please reload this page.