Improve the Filesystem performance

[RaphaelIT7]

Details

So the filesystem is quite slow and it should be improved.

std::string usage

from what I noticed, std::strings are frequently used in the Addon filesystem and the file.* functions, and they can/do have a noticeable performance impact from always being created/destroyed.
I would suggest going through the usage of std::string to check if they are required or could be changed to a std::string_view for example.

Note

Look down in the "Linux DS 32x Test" of FixUpPath where you can see std:string in the images taking a good bit.

CBaseFileSystem::IsDirectory

I would also request vprof to be added to CBaseFileSystem::IsDirectory since that function is also quiet performance intensive, but it's missing it.

Lazy directory check

With a lazy directory check I mean that you get a convar like filesystem_lazydircheck that if enabled it will cause the filesystem to first check if the directory name has a . in the name and if it has we simply return false since we assume it's a directory.
Since normally the directory we check shouldn't have a . in the name this can be a good improvement.
And we made it toggle with a convar so that by default it will be disabled and won't change the behavior but can easily be enabled.

Changes required:

static ConVar filesystem_lazydircheck("holylib_filesystem_lazydircheck", "0", 0, "When checking if a path is a directory it will check if it has a . in the name after the last / and if so, it will assume it's a file extension and return false.");

static bool isFile(const char *path) {
	const char *last_slash = strrchr(path, '/');
	const char *last_dot = strrchr(path, '.');

	return last_dot != NULL && (last_slash == NULL || last_dot > last_slash);
}

bool CBaseFileSystem::IsDirectory(const char* pFileName, const char* pPathID)
{
    if (filesystem_lazydircheck.GetBool() && isFile(pFileName))
        return false;

    [...]
}

CBaseFileSystem::FixUpPath improvement

Another thing is to implement an improvement for CBaseFileSystem::FixUpPath which perfectly works and noticeably improves it.
The improvement works by only calling GetSearchPath("BASE_PATH") once instead for every call.
Example of the full implementation: https://github.com/RaphaelIT7/gmod-holylib/blob/main/source/modules/filesystem.cpp#L447-L488

Changes required:
CBaseFileSystem::FixUpPath

bool CBaseFileSystem::FixUpPath( ... )
    [...]
    
    }
    else 
    {
        if ( m_iBaseLength < 3 ) // If It's below 3 it's most likely empty. So we try again. (If I remember correctly GetSearchPath never returns 0?)
            m_iBaseLength = GetSearchPath( "BASE_PATH", true, m_pBaseDir, sizeof( m_pBaseDir ) );

        if ( m_iBaseLength > 3 )
        {
            if ( *m_pBaseDir && (m_iBaseLength+1 < V_strlen( pFixedUpFileName ) ) && (0 != V_strncmp( m_pBaseDir, pFixedUpFileName, m_iBaseLength ) )    )
            {
                V_strlower( &pFixedUpFileName[m_iBaseLength-1] );
            }
        }
    }

basefilesystem.h

abstract_class CBaseFileSystem
{
        [...]

private:
        char m_pBaseDir[MAX_PATH];
        int m_iBaseLength = 0;
}

Note

The following images are a bit older, but still should be accurate.
Without:

With:

HL2 Build Test

So here I tested the same thing on a hl2 build in debug where we try to find a file that doesn't exist.
Code: lua_run local a = SysTime() for k=1, 10000 do file.Exists("garrysmoood.ver", "GAME") end print(SysTime()-a)

Note

In my build file.Exists doesn't use IsDirectory unlike Gmod so it's basicly just file.Open() != nil

Unoptimized Version: 12.3706939

Optimized Version: 5.4124283

Linux DS 32x Test

Here we test it on Gmod's Linux DS dev branch.
Code: lua_run local a = SysTime() for k=1, 100000 do local f=file.Open("garrysmoood.ver", "r", "GAME") end print(SysTime()-a)

Unoptimized Version: 32.752237895

Optimized Version: 21.994974079
(perf didn't even manage to record it)

Searchcache

This would also be useful especially for Lua files since Gmod calls CBaseFileSystem::GetFileTime and then CBaseFileSystem::Open.
The improvement there would be to reuse the CSearchPath that CBaseFileSystem::GetFileTime found the file in and try to use that one to also open it which should work quiet well and should/will be a noticable improvement.

Note

Since this will be a lot more complex I would leave this out for now but I just wanted to also quickly bring that up here.

CBaseFileSystem::CSearchPathsIterator::CopySearchPaths

Important

This should only be considered if the Searchcache is implemented since this only really becomes a issue if you iterate a very low number of search paths.
I noticed this when the Searchcache found a file / used only a single search path was used.

So this function can be slow since it's copying the CUtlVector but it isn't too huge of a impact.
I didn't find a solution to this yet but it definetly should be noted here, since it would affect/improve many functions if this can be solved.

If were running in the main thread, do we even need to copy it?
Maybe this could be improved by instead holding a reference to the CBaseFileSystem's Vector?
It should always wait for all Async operations to finish if we modify the SearchPaths, so it should be safe to just directly use the vector of the filesystem itself?
The only requirement for that would probably be that the function itself doesn't delete any searchpaths when iterating using it. but no found should hopefully be doing that.

The Idea(Untested):
basefilesystem.h

class CSearchPathsIterator
{
private:
    [...]
    CUtlVector<CSearchPath>&     m_SearchPaths; // Added the & which should just work?
}

If I see this correctly we can also fully remove the m_SearchPathsMutex since it doesn't have any real purpose?
This should be the only change required:
basefilesystem.h

void CBaseFileSystem::RemoveAllSearchPaths( void )
{
    [[..]]
    AsyncFinishAll(); // Replace the mutex usage with this?
}

Autorefresh

When Autorefresh reloads a Lua file, it lets the filesystem search through all paths again to find and load it again.
Shouldn't it be possible to give it/let it use the absolute file path directly, which should be way faster?

Note

I didn't look into how Bootil does the file watching so I'm not sure if it even provides the full path but it should definetely be checked.

Searchpath order (legacy addons)

I would also take a look into the searchpath order to move often used paths to the head as another improvement.
I noticed that folders in addons/ that start with _ like _content are added after all other addons? This should probably be looked into.

Improve include

So from what I can tell, include will always go through all searchpaths to try to find a file.
If that file, for example, is now from addon addons/myaddon/lua/autorun/sv_loader.lua and it loads files that are in the addons/myaddon/lua/loader/ folder, it would unnecessarily loop through all other searchpaths first trying to find it.
My suggestion is to first check if the addons/myaddon folder itself has the file that should be included, which should be a good improvement.

List

Simple list to keep track of what was done and what's still waiting

• Checked std::string usage
• Added vprof to CBaseFileSystem::IsDirectory
• Added Lazy directory check
• CBaseFileSystem::FixUpPath improvement
• Implement a searchcache
• Better Autorefresh
• Improve include

[RaphaelIT7]

Just to document the results, I tested the latest dev version with the implemented CBaseFileSystem::FixUpPath change and it seems like on a default gmod server it improved it by ~7 seconds.

Tested command: lua_run local a = SysTime() for k=1, 100000 do file.Open("garrysmoood.ver", "rb", "GAME") end print(SysTime()-a)
Latest Dev:

12.838807993

Older Dev:

19.607373388