multitheftauto
Element data optimizations
This PR is intended to improve the element data performance. The current implementation is far too inefficient because of a lot of string copying and the scourge of strlen invocations. There is also an excessive amount of access to std::map which makes it worse. Prior to the PR each setElementData call was producing at least 5 string copies on client and at least 7 copies(and a multiple of player count in worst case when using an element data subscription) on server side. There were also 14 strlen invocations(each has O(n) complexity).
Among the other improvements an additional ElementData's value check(newValue != oldValue) in method Packet_CustomData which can protect a server from PACKET_ID_CUSTOM_DATA packets spamming and the same for CElementRPCs::SetElementData method which also can protect a client from erroneous changes.
Simple(preliminary) measurements(local set/getElementData) show the performance gain nearly 2(in some cases 5, depends on an element data value) times.
Fun fact: 80% of the setElementData execution time is on(Client)ElementDataChange event processing with just one simple event handler.
Fun fact: 80% of the
setElementDataexecution time ison(Client)ElementDataChangeevent processing with just one simple event handler.
lol. I would understand it for an event that can be canceled, but not so much for an event that is not cancellable (like this one). Is there any way to optimize this? Just wondering..
Fun fact: 80% of the
setElementDataexecution time ison(Client)ElementDataChangeevent processing with just one simple event handler.lol. I would understand it for an event that can be canceled, but not so much for an event that is not cancellable (like this one). Is there any way to optimize this? Just wondering..
Yes, events can be slightly optimized. But to get a noticeable performance gain there would need a lot of work(probably, a full reengineering of the event system).
I think function setElementDataSilence(element, key, silent)(suggest the better name) can be added for blocking element data events. Because in most of the cases those events are not needed but they still consume a lot of CPU.
Apart from Lua events (see notes about their performance above), the element data is as fast as possible now.
In theory google::dense_hash_map may give even better results but I'm afraid it will consume too much memory
I think function
setElementDataSilence(element, key, silent)(suggest the better name) can be added for blocking element data events. Because in most of the cases those events are not needed but they still consume a lot of CPU.
better to just add argument to setElementData that tells if it should trigger an event
In theory
google::dense_hash_mapmay give even better results but I'm afraid it will consume too much memory
These days we have a lot of memory in our computers. 16GB is the minimum for new pcs in few last years, 8GB is still common. I think people can handle that really. Lets not be so paranoic. Main problem of element-datas are their CPU usage.
I think function
setElementDataSilence(element, key, silent)(suggest the better name) can be added for blocking element data events. Because in most of the cases those events are not needed but they still consume a lot of CPU.better to just add argument to setElementData that tells if it should trigger an event
This brings up a question: what should we do when a client sends data to a server? Send this flag as well and dictate whether server should invoke an event? It can produce the following circle: client -> server -> other clients. One of the clients may decide to silently change the data without even possibility for detecting it at endpoints. Or we shouldn't allow clients do that?
These days we have a lot of memory in our computers. 16GB is the minimum for new pcs in few last years, 8GB is still common. I think people can handle that really. Lets not be so paranoic. Main problem of element-datas are their CPU usage.
But you are aware that MTA/GTA is only 32 bit?
I think function
setElementDataSilence(element, key, silent)(suggest the better name) can be added for blocking element data events. Because in most of the cases those events are not needed but they still consume a lot of CPU.better to just add argument to setElementData that tells if it should trigger an event
This brings up a question: what should we do when a client sends data to a server? Send this flag as well and dictate whether server should invoke an event? It can produce the following circle: client -> server -> other clients. One of the clients may decide to silently change the data without even possibility for detecting it at endpoints. Or we shouldn't allow clients do that?
IMO this should definitely be an additional argument to setElementData instead of another function. It makes no sense to have it as a separate function (at least none that I can think of). The client should have this argument ignored when syncMode is set to true (synced with server) and same on the server logic - it should ignore such flag if sent from client.
One of the clients may decide to silently change the data without even possibility for detecting it at endpoints
This can be also solved by #3286 😎
newValue != oldValue doesn't work all that well.
Checking whenever 2 LuaArguments are the same isn't trivial, and the implementation is currently very shallow.
So keep that in mind.
Although this PR is fully functional I'm forced to admit that most of the new suggestions cannot be addressed without the C++20 support (see the discussion in comments above). A brief attempt to enable C++20 shows that a lot of things(even the singular deathmatch module) depend on previous standards. So there is many things that should be resolved before. I hope that work will be done in the foreseeable future.
Am i correct that we can save a lot of performance if we just delete all "onClientElementDataChange" event handlers in all running scripts? I use this event a lot in my old WW2 server because its very handy but the performance is terrible, even on a Ryzen 5900X.
Am i correct that we can save a lot of performance if we just delete all "onClientElementDataChange" event handlers in all running scripts? I use this event a lot in my old WW2 server because its very handy but the performance is terrible, even on a Ryzen 5900X.
Events are the heavier part of the element data. Thus, we can get a significant performance gain by disabling them. My original idea was to add setElementDataSilence(element, key, silent) function for this. If you're asking if you can get that gain right now, then yes. The less event handles you have the less they impact the performance.
Are there any updates or foreseeable plans to continue this? Element data optimizations would be huge for MTA. @tederis
Are there any updates or foreseeable plans to continue this? Element data optimizations would be huge for MTA. @tederis
As was already mentioned above, most of the suggestions cannot be addressed without the C++20 support. For example, one of the suggestions was using std::string instead of SString, but this would lead to the performance degradation without C++20. But there could be some workarounds that I'm going to consider. Perhaps, it would require an intermediate PR (for the string interning implementation). I'm also interested in the element data optimization, so I'm going to finish it this month.