perf(mapcache): Simplify and improve implementation of MapCache to prevent expensive reoccurring redundant map cache reads #1775
Conversation
…event expensive reoccurring redundant map cache reads
xezon
added
GUI
Mauller
left a comment
Mauller
left a comment
There was a problem hiding this comment.
Looks good overall, only a few points to help clear up the logic.
|
|
||
| MapCache::iterator it = begin(); | ||
| MapMetaData md; | ||
| for (; it != end(); ++it) |
There was a problem hiding this comment.
Would be clearer and more standardised to make all of the iterator based for loops into while loops instead.
There was a problem hiding this comment.
I intentionally did not do that because while loops should be the exception. Because it is far easier to create infinite loops by placing a continue in them, when not accompanied with iterator change.
| TheFileSystem->createDirectory(mapDir); | ||
|
|
||
| filepath.concat(m_mapCacheName); | ||
| FILE *fp = fopen(filepath.str(), "w"); |
There was a problem hiding this comment.
If we are rewriting this, should we make use of the game file system like we did with the recorder class instead of raw C file handling? It might make some of the file handling a bit cleaner.
could be someting for a followup PR at some point.
There was a problem hiding this comment.
Maybe yes. I did not specifically touch the file handling stuff, just nearby code.
| if (loadUserMaps()) | ||
| // Create the standard map cache if required. Is only relevant for Mod developers. | ||
| // TheSuperHackers @tweak This step is done before loading any other map caches to not poison the cached state. | ||
| if (!m_hasTriedCreatingStandardMapCacheINI) |
There was a problem hiding this comment.
Could be more concise and easier to understand the logic in places to name these variables with the action instead of past tense of the action so;
m_CreateStandardMapCache
m_LoadUserMapCache
m_LoadStandardMapCache
Then just invert the logic. Could also drop the INI from the naming of things since we could make the map cache binary data in future.
With the current naming it makes logic look strange in places.
| } | ||
|
|
||
| // Load standard maps from map cache last. | ||
| // This overwrites matching user maps to prevent munkees getting rowdy :) |
| { | ||
| // not seen in the dir - clear it out. | ||
| erase(mapName); | ||
| erase(it); |
There was a problem hiding this comment.
Is kind of annoying that stlport map does not support returning an interator from the erase function.
| } | ||
|
|
||
| Bool MapCache::loadUserMaps() | ||
| Bool MapCache::loadMapsFromDisk( const AsciiString &mapDir, Bool isOfficial, Bool filterByAllowedMaps ) |
There was a problem hiding this comment.
The nesting in this function was filth, nice to see that flattened out and making more sense.
|
|
||
| Bool exists = false; | ||
| AsciiString munkee = worldDict.getAsciiString(TheKey_mapName, &exists); | ||
| md.m_nameLookupTag = munkee; |
There was a problem hiding this comment.
Should we keep the munkee or give it a more appropriate name?
2 participants
This change simplifies and improves the implementation of MapCache to prevent expensive reoccurring redundant map cache reads. It also removes DEBUG_LOG in
buildMapListForNumPlayersbecause it is a spammy log that adds stalling.Originally, the implementation in
MapCache::updateCachewas a total munkee disaster. Very complicated and inefficient. This change turned out to be half refactor, half performance optimization.Problems in MapCache were:
MapCache::updateCache, even if already loadedMapCache::loadUserMapsTheGlobalData->m_buildMapCachestd::map<AsciiString, Bool> m_seenAll this is fixed.
Performance measurement
Measured in optimized vs2022 build, with RTS_DEBUG.
TODO