PhysFS performance, a story of threading and locking · Mathieu Ropert
26 comments · 93% upvoted · source · other discussions
comments
TheThiefMaster
TryingT0Wr1t3
I don't remember the PhysFS license, doesn't it requires sharing modifications of the source code?
mropert [OP]
Nope, it's zlib license.
And as I mentioned, my employer wouldn't be against publishing changes upstream (we've done it in the past for other libraries) but I really don't think they'd be interested in merging them as our version makes it a C++ library (with a C API, but still...).
qwertyuiop924
Even if icculus won't upstream this, at least release your changes provided Paradox permits it?
A lot of people could benefit from this work and not releasing it is just... greedy.
[deleted]
[deleted]
mropert [OP]
I would definitely like to have a C++ virtual FS API if I could. But it's like always, there's always something more pressing that rewriting something that kinda does the job :)
60hzcherryMXram
Dumb question: what makes a file system virtual?
8ad762515de8665ec9a1
In this context it typically means something implemented entirely in userspace, not the OS-provided filesystem. Of course, all filesystems are virtual.
WellMakeItSomehow
Being able to store the data not in files on disk, but somewhere else: an archive, a database, an S3-like object store or HTTP(S) server, on FTP, on a remote machine over SSH etc.
[deleted]
[deleted]
WellMakeItSomehow
Sure, I think this is a domain where there's no "one size fits all" design. There's a lot of approaches you can take and each has its own trade-offs.
I guess it's also why I hate TAR archives :-) -- no file index and if it's compressed you need to uncompress all of it just to list the files.
The older version was streaming API which was 32 bit friendly as with memory mapping you can run out of address space easily so that's uncool.
It makes things much harder and you probably know this, but you can also map only a part (or parts) of a large file, which makes it work better on 32-bit platforms, but probably limits the amount of data you can access at once.
we could write a page handler where we could decompress the block at the failed address and cache the decompressed blocks / pages, but that would require kernel support so it's out of play scenario
I think you can use SEH (or signals) to catch the fault and retrieve the data, but.. yeah. I don't want to implement that.
Also, thanks for the DirectStorage pointer. It's the second time I hear about it, I'll take a look.
mjklaim
I remember discussing this at some point in Boost mailing list, but there was some changes necessary (in the standard too - someone made a first proposal then vanished) that would make the path type usable whatever the implementation, and the implementation would have been a variable. It's probably a breaking change, and mostly only game developers need this kind of abstraction (most of us don't use boost - I do).
turol
Which games will this ship in and when?
mropert [OP]
Crusader Kings 3 should benefit from it at release. Imperator should include it next patch (can't be 100% sure but they share engine code so it should).
Older titles will require a backport so it'll be on a case by case basis. Experiments so far have shown promising results on Stellaris but not much gain on EU4.
GerwazyMiod
Nice article! I was surprised about linked lists being used in xxi century but I guess it's more common that we would like to admit, right?
mropert [OP]
Not every code in production today was written this decade (or even this century) ;)
PhysFS has been around for some time now.
GerwazyMiod
True :)
Not a read/write lock? That would seem perfect for this.
mropert [OP] 5 points · Jul 2020
That could have worked all things considered, but it would have required to include a r/W lock implementation (I don't have access to C++17's std::shared_mutex) as I was trying to keep the dependencies of that lib to a minimum. It might have been able to get away with shared_timed_mutex but not everywhere.
In the end a toggle to read only worked and only requires checking an atomic bool to synchronize whereas most rwlock implementation need at least one mutex or condition variable.
acwaters 1 points · Jul 2020
Can you expand on this? Where wouldn't you have been able to get away with it? My understanding is that being a timed mutex costs nothing extra on non-Windows platforms, and even on Windows the overhead is negligible for most purposes...
mropert [OP] 5 points · Jul 2020
Some older titles haven't migrated to C++14 yet, so they wouldn't be able to use shared_timed_mutex. Also to be fair I completely forgot it existed until yesterday.
rysto32 2 points · Jul 2020
A lot of places where a reader/writer lock seems to be perfect wind up being incredibly bad places for reader/writer locks in practice. With most reader/writer lock implementations all of the readers have to contend over a single cache line to increment the reader count and the resulting performance is frequently deeply disappointing. Although to be fair, if each of these calls winds up performing I/O the cacheline contention cost may well end up buried in the noise. On the other hand, if this API is doing any kind of caching then having a heavily contended cache line in the fast path could cost you a lot of potential performance.
TheThiefMaster 1 points · Jul 2020 · C++latest fanatic (and game dev)
Op mentioned they used an atomic to track the read-only state - I'm not 100% on my cache behaviour, but it sounds to me like they'd already be fighting.
rysto32 1 points · Jul 2020
Atomic reads on x86 don’t have cache contention issues.
TheThiefMaster 2 points · Jul 2020 · C++latest fanatic (and game dev)
Reading it again, it sounds like they removed the lock entirely at the library level and just rely on the user to not try to mount/unmount a filesystem while another thread might be doing file things.
The atomic just seems to be to detect some basic logic errors, as it's impossible to write a nontrivial function that is threadsafe with only an atomic read on a guard variable. What happens in their implementation to enforce all read accesses are finished before their RAII accessor sets the "freeze" flag to writable?
/u/mropert ?
rysto32 2 points · Jul 2020
I took it that the mount path is only updated at very specific points in the game (initial load and when adding a mod), and that they simply had to ensure that any threads that might access that API were quiesced during those operations.
mropert [OP] 2 points · Jul 2020
Yeah that's basically the point, the atomic is there to have a cheap way to switch to a no lock implementation and an assert when a logic error would happen.
In our case, we certainly do expect the configuration to be set before we start the heavy load operation, as if it was not we would end up potentially missing some files in a very unpredictable way.