Page MenuHomeWildfire Games

Remove the "lazy loading" feature in the VFS
Needs ReviewPublic

Authored by wraitii on Sep 2 2019, 9:31 PM.
This revision needs review, but there are no reviewers specified.

Details

Reviewers
None
Summary

The VFS is 0 A.D.'s abstraction of the file system.

It maintains a unique tree representation, combining any mods and folders we "attach" into it.
Any given "virtual path" (VfsPath) is represented by a list of VfsDirectory, and finally a VfsFile.

Example:
"simulation/components/Gate.js" -> VfsDirectory "", has a child VfsDirectory "simulation/", has a child VfsDirectory "components/", which itself has several files inside it, one at the path Gate.js`.

To generate this representation, 0 A.D. code "attaches" real directories (RealDirectory) into the VFS, at a given mount point.
For example, mods such as mod or public are loaded at mount point "" (root), screenshots are loaded from the user folder at screenshots/.

Each VfsDirectory keeps a link to a single RealDirectory (currently the latest mounted, see D2781).
To avoid IO slowdowns, a "lazy loading" feature is implemented. When attaching a folder, the VfsDirectory is created/updated, but the actual process of "looking for subdirectories and files, registering them in the VfsDirectory", which is called populate, is deferred until one of two things happen:

  • a vfsLookup call is made (i.e. somebody asked for the directory and/or a child of the directory)
  • another real directory is attached to the same mount point.

The 2nd case is where things kind of break down -> it's necessary or we would "forget" to populate the original directory, but it also means that in a multi-mod setup (like 0 A.D. is now), the population happens rather early.

The implementation of this lazy-loading is a CAS atomic call in a rather unexplicited manner in VfsAttach. I think it should be removed.

This diff fixes it by actually setting the real-directory right away, which leads to populating right away, but it should really be cleaned up more now that the problem is correctly understood.

Test Plan

Test loading with and without this. You may Notice "InitVfs" reports being much slower, but the game doesn't actually load much slower since it effectively loads the same data.

Event Timeline

wraitii created this revision.Sep 2 2019, 9:31 PM
Vulcan added a comment.Sep 2 2019, 9:33 PM

Successful build - Chance fights ever on the side of the prudent.

Link to build: https://jenkins.wildfiregames.com/job/vs2015-differential/54/display/redirect

Vulcan added a comment.Sep 2 2019, 9:38 PM

Successful build - Chance fights ever on the side of the prudent.

Linter detected issues:
Executing section Source...

source/lib/file/vfs/vfs_populate.cpp
|   1| /*·Copyright·(C)·2016·Wildfire·Games.
|    | [NORMAL] LicenseYearBear:
|    | License should have "2019" year instead of "2016"
Executing section JS...
Executing section cli...

Link to build: https://jenkins.wildfiregames.com/job/docker-differential/563/display/redirect

Submerged again in VFS for a while, and I'm certain that this change is correct. It makes no sense to populate without changing the real directory first, since that's what gets used for populating, and it leads to the above user.cfg issue (basically by being a data-race).

However I think mods shouldn't be able to run into conflicts with other 'namespaces', and thus all mods should be put in a child 'mods' directory or the VFS, not at root, to be safer.

The 'data-race' is the following.
First, some preliminaries:

  • vfs_Lookup returns a VFSDirectory. It may or may not skip populating. When Mount() is called, it does skip populating. There is a 1-1 correspondance between a Vfs Path (e.g. "config/") and a VfsDirectory. If you mount multiple real folders to a single VfsPath, then there is still only one directory that gets loaded with all these files. The whole point of vfsLookup is to ensure this 1-1 correspondence.
  • vfs_Populate calls vfs_attach on subdirectories.
  • vfs_Attach populates the Directory (== vfs Path), if needed, with the content of the current real directory, then updates the real directory and says that a 'populate' call is needed [NB: it should be noticed from reading this sentence that this is odd].
  1. Mount a mod A, containing a config folder, to L"".
    • At this point, the root directory points to the mod's directory, and nothing else exists in the VFS since it wasn't populated <=> L"" needs populating.
  2. Mount the user's config folder to L"config/".
    • A L"config/" VfsDirectory is created, unpopulated. It points to the user config. both L""and L"config/" need populating.
  3. Access some other file from the mod, such as L"simulation/components/Health.js".
    1. VFS_Lookup starts at L"", the root directory, and notices that it must be populated.
    2. Per the above, vfs_attach thus gets called for L"config/" with the mod config folder. It thus populates with the content of the real directory, which is still the user config folder. It updates L"config/" to point to the mod config folder. L"config/" needs populating and points to the mod config folder - which shouldn't happened.

If one swaps steps 3 and 2, then the mod A no longer needs populating, and `L"config/" correctly points to the user config. This is why I call this a 'data race'.

The patch above fixes this, because it makes vfs_attach populate with the updated real directory right away. This is fairly coincidental, but it makes much more sense for vfs_attach to do this than to populate with the content of the last-specified-real-directory.


On a separate note, I feel like this whole logic of sometimes populating and sometimes not in vfs_lookup is odd.

Last thing I wanted to check was whether this would slow down loading times, but we don't populate from archives anyways and we have to populate at some point if files are to exist, so that ought to be irrelevant.

wraitii retitled this revision from Change Vfs_Attach order so that mods can't overwrite config/ or screenshots/ paths to Remove the "lazy loading" feature in the VFS.EditedJun 1 2020, 10:09 PM
wraitii edited the summary of this revision. (Show Details)
wraitii edited the test plan for this revision. (Show Details)

Updated the revision given my better understanding of things at D2781.

The above comments were generally missing the mark (and rather incomprehensible)