Remove VFS cache
ClosedPublic
Actions

Authored by Sandarac on Jun 1 2017, 8:36 PM.

Details

Reviewers

echotangoecho
elexis

Commits

rP20639: Remove VFS cache, because it is less effective and less efficient than the OS…

Trac Tickets

#4072

Summary

As described in the trac ticket, the VFS file cache is not useful, mainly because the OS already caches files. The main changes for this diff are in the LoadFile method in vfs.cpp, which now loads files without adding them to the cache, or checking if they are already in the cache.

Removing the cache allows to remove two old functions from GameSetup.cpp that were only used for the VFS file cache, OperatingSystemFootprint and ChooseCacheSize. OperatingSystemFootprint is particularly dated, seems to return values arbitrarily (and only considers Windows), and hasn't been updated since rP8319 (and as such considers Windows XP a "newer Windows version" in a comment). Removing these functions allows to remove some unneeded includes from GameSetup.cpp.

Comments in vfs.cpp/.h were kept if they are still relevant.
This diff was compiled and run on Windows and Linux.

Test Plan

Read the code to check that the VFS works without the "in-house" cache. Compile and run the game.

Diff Detail

Repository

rP 0 A.D. Public Repository

Lint

Automatic diff as part of commit; lint not applicable.

Unit

Automatic diff as part of commit; unit tests not applicable.

Event Timeline

Sandarac created this revision.Jun 1 2017, 8:36 PM

Build is green

Updating workspaces.
Build (release)...
Build (debug)...
Running release tests...
Running cxxtest tests (306 tests)..................................................................................................................................................................................................................................................................................................................OK!
Running debug tests...
Running cxxtest tests (306 tests)..................................................................................................................................................................................................................................................................................................................OK!
Checking XML files...

http://jw:8080/job/phabricator/1440/ for more details.

Harbormaster completed remote builds in B2101: Diff 2369.Jun 2 2017, 3:22 AM

echotangoecho added a reviewer: echotangoecho.Jun 4 2017, 10:21 AM

Needs rebase (patch fails to apply currently), also, you can remove the now unused stats_cache() function and macro in lib/file/common/file_stats.*. Otherwise patch looks and works well.

This revision now requires changes to proceed.Jul 30 2017, 5:35 PM

elexis mentioned this in D1108: Convert simulation DataTemplateManager to globalscripts, remove json template serialization in simulation and AI.Dec 9 2017, 3:22 PM

So as Sandarac pointed out, we cache arbitrary amounts in general and 20 megabyte in some other cases.:
In source/lib/alignment.h:

static const size_t MiB = size_t(1) << 20;

A MiB is 1024 * 1024, i.e. a Mebibyte (slightly more than a megabyte).

Review:

Read the code to check that the VFS works without the "in-house" cache. Compile and run the game.

That is indeed the master question. We should clarify the things we talk about.

There is the HDD cache and the OS cache. Details are found at #4072. The most compelling argument to remove the VFS cache I found was this:

20:15 < Philip> Having our own file cache in the game means there will be less free RAM, so the OS won't be able to cache as much stuff itsef
20:15 < Philip> and the OS is smarter at caching than we are

The secondary argument:

20:16 < Philip> since there are loads of higher-level caches of templates, textures, etc

These are:

The DataTemplateManager (or whatever globalscript it becomes in D1108) caches the files on it's own, so the cache would be redundant even if the HDD has no cache.
The gamesetup.js has a cache for mapfiles (g_MapData).
The TemplateLoader.cpp in m_TemplateFileData.
Translated strings are cached in the dictionary of L10n.cpp.
I have no clue about a cache of art, audio or GUI or globalscripts files.

I have to test, but the rebase is trivial and the code seems to be proven correct.
As echotangoecho said some functions may have become obsolete too that can be removed in the same go.

source/graphics/tests/test_MeshManager.h
61 ↗	(On Diff #2369)	Dont know what it does, but it will run only for some seconds and has to load the files at least once, so reading them multiple times shouldn't be a problem in the worst case scenario.
source/graphics/tests/test_TextureConverter.h
38 ↗	(On Diff #2369)	Same
source/graphics/tests/test_TextureManager.h
39 ↗	(On Diff #2369)	(Same)
source/lib/file/vfs/vfs.cpp
316 ↗	(On Diff #2369)	(files should be checked for completeness)
source/main.cpp
509 ↗	(On Diff #2369)	That's the `isNonVisualReplay` case. Only accesses simulation templates during a game I hope, and those are already cached in the software stack as mentioned.
source/network/tests/test_Net.h
44 ↗	(On Diff #2369)	There is one disabled test that loads a map once, so goodbye cache here.
source/ps/ArchiveBuilder.cpp
34 ↗	(On Diff #2369)	I don't know if the same file will be accessed more than once. If so, 20MiB should be way too few, so would have to be dropped as even a low cache would not help measurably.
source/ps/GameSetup/GameSetup.cpp
391 ↗	(On Diff #2369)	Agree to bury.
source/ps/scripting/JSInterface_Mod.cpp
67 ↗	(On Diff #2369)	oh dear, good to remove that comment. `GetAvailableMods` should perform a new filescan each call, so no cache currently neede in any case. The JS GUI mod selectionpage / momod does cache and only loads files on init. No VFS cache should be needed in this file, it should be an cache in this file, in this layer if needed.
source/scriptinterface/tests/test_ScriptConversions.h
106 ↗	(On Diff #2369)	Caching the JS files doesn't matter because they are only executed once. There are 22 templates that arguably might be read in more than one test. But afaics that is called through the TemplateLoader which does cache on its own.
source/simulation2/components/tests/test_Pathfinder.h
37 ↗	(On Diff #2369)	Same, test runs for seconds and needs to read at least once.

In D587#29751, @echotangoecho wrote:

Needs rebase (patch fails to apply currently), also, you can remove the now unused stats_cache() function and macro in lib/file/common/file_stats.*. Otherwise patch looks and works well.

It seems there are more than that unused in these two files.
I'll propose a diff after this otherwise correct and complete patch.

stats_block_cache is unused since rP9350.
A lot of things seem completely unused there actually: ScopedIoMonitor, stats_cb_start, stats_cb_finish. stats_ab_connection, ....
I will just remove this one function where the last calls to it were removed.

Thanks for this patch Sandarac, good to have removed the outdated legacy code that seems to harm more than it helps.