Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Improve multithreading support #13

Open
dnwerner opened this issue Dec 5, 2021 · 4 comments
Open

Improve multithreading support #13

dnwerner opened this issue Dec 5, 2021 · 4 comments
Assignees

Comments

@dnwerner
Copy link

dnwerner commented Dec 5, 2021

Last year Paradox developer Mathieu Ropert published a blog entry pointing to some potential enhancements in PhysFS regarding multithreading ( https://mropert.github.io/2020/07/26/threading_with_physfs/ ).

We are unaffiliated with Paradox or any Paradox products, but we encounter comparable performance problems in one of our projects when loading resources through PhysicsFS. Since modern APIs like Vulcan support multithreaded model-loading and since many machines now feature four, eight or even more cores and with ever-increasing SSD speeds, it is unfortunate that when using PhysicsFS, loading can effectively only be done on one thread at a time.

Is there any chance to see this bottleneck in PhysicsFS addressed in the future?

@icculus
Copy link
Owner

icculus commented Dec 9, 2021

I've had that blog post open in a tab for months, but haven't had a chance to work on this yet. But yes, I'd like to resolve this issue and will try to find time to do so soon.

@icculus icculus self-assigned this Dec 9, 2021
@Underewarrr
Copy link

waiting for it

@icculus
Copy link
Owner

icculus commented Dec 19, 2021

Okay, so I looked at this, and reading from a PHYSFS_File does not hold the state lock as far as I can tell, which is where most of your time would be spent (either waiting on file i/o or decompressing). We've always said that it was safe to read from handles from any thread, as long as you don't read from the same handle on two threads at the same time, and this still seems to be the case: PHYSFS_read() and friends don't grab the big global lock when you call into them.

However, PHYSFS_enumerate() does, which is to say: if you've set up your threaded loader to look like...

void myloader_on_a_thread(const char *dir_to_read_from)
{
    PHYSFS_enumerate(dir_to_read_from, load_an_enumerated_file, NULL);
}

...each thread is going to block all the others until the entire directory is enumerated, because that does hold the state lock, and if the app's enumeration callback actually loads each file during enumeration, that will effectively serialize the whole effort. While this can be worked around by the app, what I'm describing is definitely a PhysicsFS deficiency.

Going to look further into this; we can very likely make the locking more fine-grained and solve this problem without changing the API or changing thread safety guarantees in unreasonable ways.

@icculus
Copy link
Owner

icculus commented Dec 19, 2021

Confirmed with Mathieu that this is happening during enumeration, so I'll figure out how to fix that.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants