Skip to content

Element data optimizations #3287

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

Closed
wants to merge 14 commits into from
Closed

Element data optimizations #3287

Show file tree
Hide file tree
Changes from 3 commits
Commits
Show all changes
14 commits
Select commit Hold shift + click to select a range
c50fd5f
Element data optimizations
tederis Jan 10, 2024
cbfed48
Fix misprint
tederis Jan 10, 2024
206d49e
Merge branch 'master' into element_data_optimize
tederis Jan 10, 2024
347f9cc
Further optimizations
tederis Jan 12, 2024
69b5960
Revert "Further optimizations"
tederis Jan 12, 2024
02c8fd9
Merge branch 'element_data_optimize' of https://github.com/tederis/mt…
tederis Jan 12, 2024
5ee515a
Revert "Revert "Further optimizations""
tederis Jan 12, 2024
cd554b6
Remove CStaticFunctionDefinitions::GetElementData
tederis Jan 12, 2024
b22af37
Use weak argument for the data deletion
tederis Jan 12, 2024
9c560a9
Merge branch 'master' into element_data_optimize
Dutchman101 Jan 13, 2024
5ccc413
Merge branch 'master' into element_data_optimize
Dutchman101 Jan 18, 2024
0ce4587
Merge branch 'master' into element_data_optimize
tederis Jan 20, 2024
0e257a0
Merge branch 'master' into element_data_optimize
tederis Jun 19, 2024
14eafb4
Merge branch 'master' into element_data_optimize
Dutchman101 Jun 30, 2024
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
79 changes: 32 additions & 47 deletions Client/mods/deathmatch/logic/CClientEntity.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -279,12 +279,10 @@ void CClientEntity::SetID(ElementID ID)
}
}

CLuaArgument* CClientEntity::GetCustomData(const char* szName, bool bInheritData, bool* pbIsSynced)
CLuaArgument* CClientEntity::GetCustomData(const SString& strName, bool bInheritData, bool* pbIsSynced)
Copy link
Member

Choose a reason for hiding this comment

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

We are truing to not use SString in new code. Discussion

Copy link
Member Author

@tederis tederis Jan 11, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

Yes, I know. But in this partucular casу SString was chosen for the reason that it was already used in almost all associative containers. By using lets say std::string you would get an additional string copy (see SString(const std::string& strText) constructor) and potential problems with key mathching. So the usage of std::string would exceed the boundaries of this PR.

Copy link
Contributor

@Pirulax Pirulax Jan 14, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

Why not use std::string_view all the way, and only in the end (when inserting) create the SString?
This way for the getter you can get away without creating a string at all.
Also, since the code limits the string length to 128, you could use some kind of a fixed-length string and store the data directly inside it (So Caching works better).

Copy link
Member

Choose a reason for hiding this comment

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

std::string_view uses 8 bytes.
Pointers and (I believe) references use 4 bytes.
So function use less registers. And the code can be optimized better.

Copy link
Contributor

Choose a reason for hiding this comment

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

I think not allocating any memory until absolutely necessary far outweighs using 2 registers instead of 1.......

Copy link
Member Author

@tederis tederis Jan 19, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

Why are you so against string_view I don't understand.

Im not totally against string_view. If you can pass simpler value, you should use it. Basic string compare function doesn't require string length. It's similar to removing unused data from function arguments.

std::hash struct requires string len. That is std::unordered_map will have to construct a new string from the characters buffer(const char*) which is obviously not the best behaviour. std::string_view does not have this drawback.

If you use std::map you can use take advantage of transparent lookup. Sadly unordered_map doesn't support this. Another way of doing this would be to use a fixed length string buffer as the key to avoid heap allocation (and by using an unodered_map we could increase cache locality)

Heterogeneous lookups for unordered maps should be available since C++20. Regretfully we're still with C++17.

As you must be aware we are using both (unordered hash and tree-based maps) in MTA. Thus the solution must be selected according to the specifics of the both. Fixed length strings cannot be efficient here due to the unpredictable length of such strings.

Copy link
Contributor

Choose a reason for hiding this comment

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

As you must be aware we are using both (unordered hash and tree-based maps) in MTA. Thus the solution must be selected according to the specifics of the both. Fixed length strings cannot be efficient here due to the unpredictable length of such strings.

See MAX_CUSTOMDATA_NAME_LENGTH.
So no name can be longer than 128 chars, so a 128 + 1 byte buffer would be enough.
A cache line is typically 64B, so keep that in mind. (But I think 128 + 1 for the char buffer, and uint8 for the length should be enough, this way the whole struct would be 132 bytes (I think, or 136 on 64b).
I'm unsure why you think fixed length strings would be inefficient. Tree based maps have extreme memory overhead for small values (std::string + CLuaArgument is barely a few bytes).
[Also, memory footprint can be lowered by merging my CLuaArgument refactor that uses std::variant (Thus the final size is quite a bit less)]

Heterogeneous lookups for unordered maps should be available since C++20. Regretfully we're still with C++17

So we can upgrade to c++20 (For the server/client dm project at least).

Copy link
Contributor

Choose a reason for hiding this comment

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

Also, we could experiment with custom hash functions (that generate less collisions for our use-case), if we decide to use a hashmap.

Copy link
Member Author

@tederis tederis Jan 20, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

See MAX_CUSTOMDATA_NAME_LENGTH.
So no name can be longer than 128 chars, so a 128 + 1 byte buffer would be enough.
A cache line is typically 64B, so keep that in mind. (But I think 128 + 1 for the char buffer, and uint8 for the length should be enough, this way the whole struct would be 132 bytes (I think, or 136 on 64b).
I'm unsure why you think fixed length strings would be inefficient. Tree based maps have extreme memory overhead for small values (std::string + CLuaArgument is barely a few bytes).
[Also, memory footprint can be lowered by merging my CLuaArgument refactor that uses std::variant (Thus the final size is quite a bit less)]

128 bytes is still a lot actually. On large servers such keys can eat up to 10 megabytes(or even more) of the ram. You might say that 10 mb does not deserve attention, but on client side it may be significant (especially on high-load custom maps).

Personally I would prefer the StringName(as like in Godot Engine, or FName in UE) approach that implies just one buffer for each unique string. This way we only need a unique number that can be used as a key within the maps.

Copy link
Contributor

@Pirulax Pirulax Jan 20, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

That is an awesome idea, never thought of that, you're a genius!
So we could both save RAM, and CPU!
Just have to have a string interning system (That is global to all elements).
We also need to use some kind of reference counting (shared_ptr perhaps? Though the control block is allocated separately, that's kinda a waste of memory in our case) - To free unused strings.

Not sure how good of a key the serial index of the string would be, in terms of load factor (we want to avoid list traversals of course).

Not sure how efficient the same interning would be for CLuaArgument.

Though, I do have a concern, we currently use case-sensitive strings, right? FName's are case-insensitive (which makes sense, less variation)

{
assert(szName);

// Grab it and return a pointer to the variable
SCustomData* pData = m_pCustomData->Get(szName);
SCustomData* pData = m_pCustomData->Get(strName);
if (pData)
{
if (pbIsSynced)
Expand All @@ -295,22 +293,22 @@ CLuaArgument* CClientEntity::GetCustomData(const char* szName, bool bInheritData
// If none, try returning parent's custom data
if (bInheritData && m_pParent)
{
return m_pParent->GetCustomData(szName, true, pbIsSynced);
return m_pParent->GetCustomData(strName, true, pbIsSynced);
}

// None available
return NULL;
return {};
}

CLuaArguments* CClientEntity::GetAllCustomData(CLuaArguments* table)
{
assert(table);

for (auto it = m_pCustomData->IterBegin(); it != m_pCustomData->IterEnd(); it++)
for (const auto& [key, data] : m_pCustomData->GetData())
{
table->PushString(it->first); // key
table->PushArgument(it->second.Variable); // value
}
table->PushString(key); // key
table->PushArgument(data.Variable); // value
}

return table;
}
Expand Down Expand Up @@ -470,54 +468,41 @@ bool CClientEntity::GetCustomDataBool(const char* szName, bool& bOut, bool bInhe
return false;
}

void CClientEntity::SetCustomData(const char* szName, const CLuaArgument& Variable, bool bSynchronized)
bool CClientEntity::SetCustomData(const SString& strName, const CLuaArgument& Variable, bool bSynchronized)
{
assert(szName);
if (strlen(szName) > MAX_CUSTOMDATA_NAME_LENGTH)
{
// Don't allow it to be set if the name is too long
CLogger::ErrorPrintf("Custom data name too long (%s)", *SStringX(szName).Left(MAX_CUSTOMDATA_NAME_LENGTH + 1));
return;
}
// SCustomData is 64(88 in debug) bytes long. A static storage can be more efficient.
static SCustomData oldData;

// Grab the old variable
CLuaArgument oldVariable;
SCustomData* pData = m_pCustomData->Get(szName);
if (pData)
if (m_pCustomData->Set(strName, Variable, bSynchronized, &oldData))
{
oldVariable = pData->Variable;
}

// Set the new data
m_pCustomData->Set(szName, Variable, bSynchronized);
// Trigger the onClientElementDataChange event on us
CLuaArguments Arguments;
Arguments.PushString(strName);
Arguments.PushArgument(oldData.Variable);
Arguments.PushArgument(Variable);
CallEvent("onClientElementDataChange", Arguments, true);

// Trigger the onClientElementDataChange event on us
CLuaArguments Arguments;
Arguments.PushString(szName);
Arguments.PushArgument(oldVariable);
Arguments.PushArgument(Variable);
CallEvent("onClientElementDataChange", Arguments, true);
return true;
}

return false;
}

void CClientEntity::DeleteCustomData(const char* szName)
void CClientEntity::DeleteCustomData(const SString& strName)
{
// Grab the old variable
SCustomData* pData = m_pCustomData->Get(szName);
if (pData)
{
CLuaArgument oldVariable;
oldVariable = pData->Variable;

// Delete the custom data
m_pCustomData->Delete(szName);
// SCustomData is 64(88 in debug) bytes long. A static storage can be more efficient.
static SCustomData oldData;

// Delete the custom data
if (m_pCustomData->Delete(strName, &oldData))
{
// Trigger the onClientElementDataChange event on us
CLuaArguments Arguments;
Arguments.PushString(szName);
Arguments.PushArgument(oldVariable);
Arguments.PushArgument(CLuaArgument()); // Use nil as the new value to indicate the data has been removed
Arguments.PushString(strName);
Arguments.PushArgument(oldData.Variable);
Arguments.PushArgument(CLuaArgument{}); // Use nil as the new value to indicate the data has been removed
CallEvent("onClientElementDataChange", Arguments, true);
}
}
}

bool CClientEntity::GetMatrix(CMatrix& matrix) const
Expand Down
6 changes: 3 additions & 3 deletions Client/mods/deathmatch/logic/CClientEntity.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -199,14 +199,14 @@ class CClientEntity : public CClientEntityBase
void SetID(ElementID ID);

CCustomData* GetCustomDataPointer() { return m_pCustomData; }
CLuaArgument* GetCustomData(const char* szName, bool bInheritData, bool* pbIsSynced = nullptr);
CLuaArgument* GetCustomData(const SString& strName, bool bInheritData, bool* pbIsSynced = nullptr);
CLuaArguments* GetAllCustomData(CLuaArguments* table);
bool GetCustomDataString(const char* szKey, SString& strOut, bool bInheritData);
bool GetCustomDataFloat(const char* szKey, float& fOut, bool bInheritData);
bool GetCustomDataInt(const char* szKey, int& iOut, bool bInheritData);
bool GetCustomDataBool(const char* szKey, bool& bOut, bool bInheritData);
void SetCustomData(const char* szName, const CLuaArgument& Variable, bool bSynchronized = true);
void DeleteCustomData(const char* szName);
bool SetCustomData(const SString& strName, const CLuaArgument& Variable, bool bSynchronized = true);
void DeleteCustomData(const SString& strName);

virtual bool GetMatrix(CMatrix& matrix) const;
virtual bool SetMatrix(const CMatrix& matrix);
Expand Down
64 changes: 35 additions & 29 deletions Client/mods/deathmatch/logic/CCustomData.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -14,52 +14,58 @@

void CCustomData::Copy(CCustomData* pCustomData)
{
std::map<std::string, SCustomData>::const_iterator iter = pCustomData->IterBegin();
for (; iter != pCustomData->IterEnd(); iter++)
{
Set(iter->first.c_str(), iter->second.Variable);
}
for (const auto& [key, data] : pCustomData->GetData())
Set(key, data.Variable);
}

SCustomData* CCustomData::Get(const char* szName)
SCustomData* CCustomData::Get(const SString& strName, bool bCreate)
{
assert(szName);

std::map<std::string, SCustomData>::iterator it = m_Data.find(szName);
if (it != m_Data.end())
if (auto it = m_Data.find(strName); it != m_Data.end())
return &it->second;

return NULL;
if (bCreate)
return &m_Data[strName];

return {};
}

void CCustomData::Set(const char* szName, const CLuaArgument& Variable, bool bSynchronized)
bool CCustomData::Set(const SString& strName, const CLuaArgument& Variable, bool bSynchronized, SCustomData* pOldData)
{
assert(szName);

// Grab the item with the given name
SCustomData* pData = Get(szName);
if (pData)
if (strName.length() > MAX_CUSTOMDATA_NAME_LENGTH)
{
// Update existing
pData->Variable = Variable;
pData->bSynchronized = bSynchronized;
// Don't allow it to be set if the name is too long
CLogger::ErrorPrintf("Custom data name too long (%s)", *strName.Left(MAX_CUSTOMDATA_NAME_LENGTH + 1));
return false;
}
else

SCustomData* pCurrentVariable = Get(strName, true);
assert(pCurrentVariable);

if (pCurrentVariable->Variable.IsEmpty() || pCurrentVariable->Variable != Variable || pCurrentVariable->bSynchronized != bSynchronized)
{
// Add new
SCustomData newData;
newData.Variable = Variable;
newData.bSynchronized = bSynchronized;
m_Data[szName] = newData;
}
// Save the old variable
if (pOldData)
*pOldData = *pCurrentVariable;

// Set the new data
pCurrentVariable->Variable = Variable;
pCurrentVariable->bSynchronized = bSynchronized;

return true;
}

return false;
}

bool CCustomData::Delete(const char* szName)
bool CCustomData::Delete(const SString& strName, SCustomData* pOldData)
{
// Find the item and delete it
std::map<std::string, SCustomData>::iterator it = m_Data.find(szName);
auto it = m_Data.find(strName);
if (it != m_Data.end())
{
if (pOldData)
*pOldData = it->second;

m_Data.erase(it);
return true;
}
Expand Down
13 changes: 6 additions & 7 deletions Client/mods/deathmatch/logic/CCustomData.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -17,22 +17,21 @@
struct SCustomData
{
CLuaArgument Variable;
bool bSynchronized;
bool bSynchronized{true};
};

class CCustomData
{
public:
void Copy(CCustomData* pCustomData);

SCustomData* Get(const char* szName);
void Set(const char* szName, const CLuaArgument& Variable, bool bSynchronized = true);
SCustomData* Get(const SString& strName, bool bCreate = false);
bool Set(const SString& strName, const CLuaArgument& Variable, bool bSynchronized = true, SCustomData* pOldData = {});

bool Delete(const char* szName);
bool Delete(const SString& strName, SCustomData* pOldData = {});

std::map<std::string, SCustomData>::const_iterator IterBegin() { return m_Data.begin(); }
std::map<std::string, SCustomData>::const_iterator IterEnd() { return m_Data.end(); }
const std::unordered_map<SString, SCustomData>& GetData() const { return m_Data; }

private:
std::map<std::string, SCustomData> m_Data;
std::unordered_map<SString, SCustomData> m_Data;
};
17 changes: 5 additions & 12 deletions Client/mods/deathmatch/logic/CStaticFunctionDefinitions.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -993,34 +993,27 @@ bool CStaticFunctionDefinitions::SetElementID(CClientEntity& Entity, const char*
return false;
}

bool CStaticFunctionDefinitions::SetElementData(CClientEntity& Entity, const char* szName, CLuaArgument& Variable, bool bSynchronize)
bool CStaticFunctionDefinitions::SetElementData(CClientEntity& Entity, const SString& strName, CLuaArgument& Variable, bool bSynchronize)
{
assert(szName);
assert(strlen(szName) <= MAX_CUSTOMDATA_NAME_LENGTH);

bool bIsSynced;
CLuaArgument* pCurrentVariable = Entity.GetCustomData(szName, false, &bIsSynced);
if (!pCurrentVariable || Variable != *pCurrentVariable || bIsSynced != bSynchronize)
if (Entity.SetCustomData(strName, Variable, bSynchronize))
{
if (bSynchronize && !Entity.IsLocalEntity())
{
NetBitStreamInterface* pBitStream = g_pNet->AllocateNetBitStream();
// Write element ID, name length and the name. Also write the variable.
pBitStream->Write(Entity.GetID());
unsigned short usNameLength = static_cast<unsigned short>(strlen(szName));
const unsigned short usNameLength = static_cast<unsigned short>(strName.length());
pBitStream->WriteCompressed(usNameLength);
pBitStream->Write(szName, usNameLength);
pBitStream->Write(strName.c_str(), usNameLength);
Variable.WriteToBitStream(*pBitStream);

// Send the packet and deallocate
g_pNet->SendPacket(PACKET_ID_CUSTOM_DATA, pBitStream, PACKET_PRIORITY_HIGH, PACKET_RELIABILITY_RELIABLE_ORDERED);
g_pNet->DeallocateNetBitStream(pBitStream);
}

// Set its custom data
Entity.SetCustomData(szName, Variable, bSynchronize);
return true;
}
}

return false;
}
Expand Down
2 changes: 1 addition & 1 deletion Client/mods/deathmatch/logic/CStaticFunctionDefinitions.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -81,7 +81,7 @@ class CStaticFunctionDefinitions
static CClientDummy* CreateElement(CResource& Resource, const char* szTypeName, const char* szID);
static bool DestroyElement(CClientEntity& Entity);
static bool SetElementID(CClientEntity& Entity, const char* szID);
static bool SetElementData(CClientEntity& Entity, const char* szName, CLuaArgument& Variable, bool bSynchronize);
static bool SetElementData(CClientEntity& Entity, const SString& strName, CLuaArgument& Variable, bool bSynchronize);
static bool RemoveElementData(CClientEntity& Entity, const char* szName);
static bool SetElementMatrix(CClientEntity& Entity, const CMatrix& matrix);
static bool SetElementPosition(CClientEntity& Entity, const CVector& vecPosition, bool bWarp = true);
Expand Down
2 changes: 2 additions & 0 deletions Client/mods/deathmatch/logic/lua/CLuaArgument.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -51,6 +51,8 @@ class CLuaArgument
int GetType() const { return m_iType; };
int GetIndex() const { return m_iIndex; };

bool IsEmpty() const { return m_iType == LUA_TNIL; }

bool GetBoolean() const { return m_bBoolean; };
lua_Number GetNumber() const { return m_Number; };
const SString& GetString() { return m_strString; };
Expand Down
Loading