1
0
mirror of https://github.com/TerryCavanagh/VVVVVV.git synced 2024-06-02 02:53:32 +02:00

Refactor level dir listing to not use STL data marshalling

Note that level dir listing still uses plenty of STL (including the end
product - the `LevelMetaData` struct - which, for the purposes of 2.3,
is okay enough (2.4 should remove STL usage entirely)); it's just that
the initial act of iterating over the levels directory no longer takes
four or SIX(!!!) heap allocations (not counting reallocations and other
heap allocations this patch does not remove), and no longer does any
data marshalling.

Like text splitting, and binary blob extra indice grabbing, the current
approach that FILESYSTEM_getLevelDirFileNames() uses is a temporary
std::vector of std::strings as a middleman to store all the filenames,
and the game iterates over that std::vector to grab each level metadata.
Except, it's even worse in this case, because PHYSFS_enumerateFiles()
ALREADY does a heap allocation. Oh, and
FILESYSTEM_getLevelDirFileNames() gets called two or three times. Yeah,
let me explain:

1. FILESYSTEM_getLevelDirFileNames() calls PHYSFS_enumerateFiles().

2. PHYSFS_enumerateFiles() allocates an array of pointers to arrays of
   chars on the heap. For each filename, it will:

   a. Allocate an array of chars for the filename.

   b. Reallocate the array of pointers to add the pointer to the above
      char array.

      (In this step, it also inserts the filename in alphabetically -
      without any further allocations, as far as I know - but this is a
      COMPLETELY unnecessary step, because we are going to sort the list
      of levels by ourselves via the metadata title in the end anyways.)

3. FILESYSTEM_getLevelDirFileNames() iterates over the PhysFS list, and
   allocates an std::vector on the heap to shove the list into. Then,
   for each filename, it will:

   a. Allocate an std::string, initialized to "levels/".

   b. Append the filename to the std::string above. This will most
      likely require a re-allocation.

   c. Duplicate the std::string - which requires allocating more memory
      again - to put it into the std::vector.

      (Compared to the PhysFS list above, the std::vector does less
      reallocations; it however will still end up reallocating a certain
      amount of times in the end.)

4. FILESYSTEM_getLevelDirFileNames() will free the PhysFS list.

5. Then to get the std::vector<std::string> back to the caller, we end
   up having to reallocate the std::vector again - reallocating every
   single std::string inside it, too - to give it back to the caller.

And to top it all off, FILESYSTEM_getLevelDirFileNames() is guaranteed
to either be called two times, or three times. This is because
editorclass::getDirectoryData() will call editorclass::loadZips(), which
will unconditionally call FILESYSTEM_getLevelDirFileNames(), then call
it AGAIN if a zip was found. Then once the function returns,
getDirectoryData() will still unconditionally call
FILESYSTEM_getLevelDirFileNames(). This smells like someone bolting
something on without regard for the whole picture of the system, but
whatever; I can clean up their mess just fine.

So, what do I do about this? Well, just like I did with text splitting
and binary blob extras, make the final for-loop - the one that does the
actual metadata parsing - more immediate.

So how do I do that? Well, PhysFS has a function named
PHYSFS_enumerate(). PHYSFS_enumerateFiles(), in fact, uses this function
internally, and is basically just a wrapper with some allocation and
alphabetization.

PHYSFS_enumerate() takes in a pointer to a function, which it will call
for every single entry that it iterates over. It also lets you pass in
another arbitrary pointer that it leaves alone, which I use to pass
through a function pointer that is the actual callback.

So to clarify, there are two callbacks - one callback is passed through
into another callback that gets passed through to PHYSFS_enumerate().

The callback that gets passed to PHYSFS_enumerate() is always the same,
but the callback that gets passed through the callback can be different
(if you look at the calling code, you can see that one caller passes
through a normal level metadata callback; the other passes through a zip
file callback).

Furthermore, I've also cleaned it up so that if editorclass::loadZips()
finds a zip file, it won't iterate over all the files in the levels
directory a third time. Instead, the level directory only gets iterated
over twice - once to check for zips, and another to load every level
plus all zips; the second time is when all the heap allocations happen.

And with that, level list loading now uses less STL templated stuff and
much less heap allocations.

Also, ed.directoryList basically has no reason to exist other than being
a temporary std::vector, so I've removed it. This further decreases
memory usage, depending on how many levels you have in your levels
folder (I know that I usually have a lot and don't really ever clean it
up, lol).

Lastly, in the callback passed to PhysFS, `builtLocation` is actually no
longer hardcoded to just the `levels` directory, since instead we now
use the `origdir` variable that PhysFS passes us. So that's good, too.
This commit is contained in:
Misa 2021-02-18 18:42:13 -08:00 committed by Ethan Lee
parent 532dbee4fd
commit 83976016c7
5 changed files with 53 additions and 49 deletions

View File

@ -337,23 +337,31 @@ bool FILESYSTEM_loadTiXml2Document(const char *name, tinyxml2::XMLDocument& doc)
return true;
}
std::vector<std::string> FILESYSTEM_getLevelDirFileNames()
{
std::vector<std::string> list;
char **fileList = PHYSFS_enumerateFiles("/levels");
char **i;
std::string builtLocation;
static PHYSFS_EnumerateCallbackResult enumerateCallback(
void* data,
const char* origdir,
const char* filename
) {
void (*callback)(const char*) = (void (*)(const char*)) data;
char builtLocation[MAX_PATH];
for (i = fileList; *i != NULL; i++)
{
builtLocation = "levels/";
builtLocation += *i;
list.push_back(builtLocation);
}
SDL_snprintf(
builtLocation,
sizeof(builtLocation),
"%s/%s",
origdir,
filename
);
PHYSFS_freeList(fileList);
callback(builtLocation);
return list;
return PHYSFS_ENUM_OK;
}
void FILESYSTEM_enumerateLevelDirFileNames(
void (*callback)(const char* filename)
) {
PHYSFS_enumerate("levels", enumerateCallback, (void*) callback);
}
static void PLATFORM_getOSDirectory(char* output)

View File

@ -1,8 +1,7 @@
#ifndef FILESYSTEMUTILS_H
#define FILESYSTEMUTILS_H
#include <string>
#include <vector>
#include <stddef.h>
// Forward declaration, including the entirety of tinyxml2.h across all files this file is included in is unnecessary
namespace tinyxml2 { class XMLDocument; }
@ -25,7 +24,7 @@ void FILESYSTEM_freeMemory(unsigned char **mem);
bool FILESYSTEM_saveTiXml2Document(const char *name, tinyxml2::XMLDocument& doc);
bool FILESYSTEM_loadTiXml2Document(const char *name, tinyxml2::XMLDocument& doc);
std::vector<std::string> FILESYSTEM_getLevelDirFileNames();
void FILESYSTEM_enumerateLevelDirFileNames(void (*callback)(const char* filename));
bool FILESYSTEM_openDirectoryEnabled();
bool FILESYSTEM_openDirectory(const char *dname);

View File

@ -77,28 +77,28 @@ static bool compare_nocase (std::string first, std::string second)
return false;
}
void editorclass::loadZips()
static void levelZipCallback(const char* filename)
{
directoryList = FILESYSTEM_getLevelDirFileNames();
bool needsReload = false;
std::string filename_ = filename;
for(size_t i = 0; i < directoryList.size(); i++)
if (endsWith(filename_, ".zip"))
{
if (endsWith(directoryList[i], ".zip")) {
PHYSFS_File* zip = PHYSFS_openRead(directoryList[i].c_str());
if (!PHYSFS_mountHandle(zip, directoryList[i].c_str(), "levels", 1)) {
printf(
"Could not mount %s: %s\n",
filename_.c_str(),
PHYSFS_getErrorByCode(PHYSFS_getLastErrorCode())
);
} else {
needsReload = true;
}
PHYSFS_File* zip = PHYSFS_openRead(filename_.c_str());
if (!PHYSFS_mountHandle(zip, filename_.c_str(), "levels", 1))
{
printf(
"Could not mount %s: %s\n",
filename_.c_str(),
PHYSFS_getErrorByCode(PHYSFS_getLastErrorCode())
);
}
}
}
if (needsReload) directoryList = FILESYSTEM_getLevelDirFileNames();
void editorclass::loadZips()
{
FILESYSTEM_enumerateLevelDirFileNames(levelZipCallback);
}
static void replace_all(std::string& str, const std::string& from, const std::string& to)
@ -206,24 +206,26 @@ TAG_FINDER(find_website, "website");
#undef TAG_FINDER
static void levelMetaDataCallback(const char* filename)
{
extern editorclass ed;
LevelMetaData temp;
std::string filename_ = filename;
if (ed.getLevelMetaData(filename_, temp))
{
ed.ListOfMetaData.push_back(temp);
}
}
void editorclass::getDirectoryData()
{
ListOfMetaData.clear();
directoryList.clear();
loadZips();
directoryList = FILESYSTEM_getLevelDirFileNames();
for(size_t i = 0; i < directoryList.size(); i++)
{
LevelMetaData temp;
if (getLevelMetaData( directoryList[i], temp))
{
ListOfMetaData.push_back(temp);
}
}
FILESYSTEM_enumerateLevelDirFileNames(levelMetaDataCallback);
for(size_t i = 0; i < ListOfMetaData.size(); i++)
{
@ -232,7 +234,6 @@ void editorclass::getDirectoryData()
if(compare_nocase(ListOfMetaData[i].title, ListOfMetaData[k].title ))
{
std::swap(ListOfMetaData[i] , ListOfMetaData[k]);
std::swap(directoryList[i], directoryList[k]);
}
}
}

View File

@ -110,7 +110,6 @@ class editorclass{
std::string Desc3;
std::string website;
std::vector<std::string> directoryList;
std::vector<LevelMetaData> ListOfMetaData;
void loadZips();

View File

@ -298,9 +298,6 @@ int main(int argc, char *argv[])
game.playassets = playassets;
game.menustart = true;
ed.directoryList.clear();
ed.directoryList.push_back(playtestname);
LevelMetaData meta;
if (ed.getLevelMetaData(playtestname, meta)) {
ed.ListOfMetaData.clear();