perf(namekey): Remove superfluous AsciiString allocations for name key lookups #1917
Conversation
xezon
added
Minor
| /** | ||
| Find the Armor with the given name. If no such Armor exists, return null. | ||
| */ | ||
| const ArmorTemplate* findArmorTemplate(AsciiString name) const; |
There was a problem hiding this comment.
it would have been better to just alter these to pass by reference.
There was a problem hiding this comment.
I disagree, because these functions need no AsciiString and passing const char* is better, because it allows to pass string literals without AsciiString middle-man allocation.
There are a lot of call sites that pass string literals to these. For example in INI parse functions.
There was a problem hiding this comment.
The ascii string variant will be hit the most during runtime as the upgrade names in thing data will be an ascii string.
it could be handled with and overloaded function that passes an ascii string reference then calls the const char* implementation with the stored string.
There was a problem hiding this comment.
To me it looked 50/50. Some callsites come with AsciiString, others with C strings. The goal here is to discourage passing AsciiString to avoid unnecessary allocations where not necessary.
AsciiString can be passed on callsite with myAscii.str()
When you look through this refactor, you will see a lot of places where allocations have been eliminated.
There was a problem hiding this comment.
Yeah it should be fine in the end having looked back over it.
There was a problem hiding this comment.
After reviewing the NameKeyGenerator class one more time I realized this change is not optimal either.
What NameKeyGenerator lookups with AsciiString essentially did:
Take AsciiString, pass on into sub functions as C string, then eventually allocate a new AsciiString from that C string if it created a new NameKey entry, otherwise just use the C string for the lookup.
This duplicate AsciiString allocation can be avoided by passing the original AsciiString into the creation of the new NameKey entry. This was done with change #1959 and that should be optimal now.
Closing.
xezon
force-pushed
the
xezon/refactor-asciistring-unicodestring-instance-2
branch
from
f024f01 to
1df46e0
Compare
|
Rebased on main |
| Image *Enum(unsigned index) | ||
| { | ||
| for (std::map<unsigned,Image *>::iterator i=m_imageMap.begin();i!=m_imageMap.end();++i) | ||
| for (ImageMap::iterator i=m_imageMap.begin();i!=m_imageMap.end();++i) |
There was a problem hiding this comment.
This is one of those times when Auto would have been fine instead of a typedef :P but VC6 so not much we can do yet.
|
|
||
| const char * upgradeAlt = getCommandSetUpgradeModuleData()->m_triggerAlt.str(); | ||
| const UpgradeTemplate *upgradeTemplate = TheUpgradeCenter->findUpgrade( upgradeAlt ); | ||
| AsciiString upgradeAlt = getCommandSetUpgradeModuleData()->m_triggerAlt; |
There was a problem hiding this comment.
You could probably revert this one since it's going to cause the reference count to get touched.
xezon
force-pushed
the
xezon/refactor-asciistring-unicodestring-instance-2
branch
from
95cfbb7 to
a961400
Compare
2 participants
THE FIRST COMMIT IS FROM #1916This change removes superfluous
AsciiStringallocations for name key lookups. It was applied by hand.It cuts many allocations and can help loading and runtime performance a bit.
NAMEKEYandnameToKeyvariants forAsciiStringhave been deleted to discourage their use and to make the programmer think if he wants to pass a C string fromAsciiStringand if he really needed to allocate that.Adjacent to Name Key, some functions that only did Name Key lookups and needed no AsciiString have been changed to take
const char*instead ofAsciiString.The identified and affected functions are:
There are probably plenty more functions of this kind but this change is already large enough as is and this seemed to be all directly related to Name Key things.
TODO