Skip to content

Expose a file_list API + VfsPath::as_filesystem #73

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

Open
wants to merge 15 commits into
base: master
Choose a base branch
from

Conversation

cheesycod
Copy link

file_list is really useful for my usecase of updating an in memory filesystem from a hashmap and may be useful for other cases as well. For now, this is only implemented in in-memory FS where it can be done quickly (and could be optimized even further in future). VfsPath::as_filesystem is useful for extracting out the filesystem from a VfsPath

@cheesycod
Copy link
Author

cheesycod commented Apr 28, 2025

Have also fixed up the RwLock unwrap a bit by adding a new error variant VfsErrorKind::LockPoisoned and adds a non-panicking new to overlayfs and a try_new_owned to avoid allocating a new vector every time

@cheesycod
Copy link
Author

cheesycod commented Apr 28, 2025

Have also applied a few optimizations with a new Filesystem trait method read_to_string and read_to_bytes with a default impl from VfsPath. MemoryFS then overrides this default impl to avoid buffering / extra copying/extra RwLock calls etc where not needed [errors to be fixed soon along with access time related things]

@manuel-woelker
Copy link
Owner

Thanks for the PR. I would really like to better understand the use cases for things like file_list() and as_filesystem().

What specific use cases do you have in mind?

@cheesycod
Copy link
Author

cheesycod commented Apr 28, 2025

Thanks for the PR. I would really like to better understand the use cases for things like file_list() and as_filesystem().

What specific use cases do you have in mind?

file_list allows for listing all files and folders in the filesystem which is useful when you want to quickly list the files of an in memory filesystem to a user or for diffing a in memory filesystem for updating only changed files in a lua require cache (this is a biggy for me, my product is a discord bot which uses an in memory filesystem to allow running luau scripts that are potentially multiple files require'd together). This is also why I added the replace and take filesystem API's along with the read_to_bytes filesystem trait method as I use MemoryFS in hot-path code so perf there matters a lot now.

as_filesystem allows for me to pull out the inner filesystem from a vfspath which is a big hurdle i've faced internally (leading to an uneeded Arc<Box> to work around this fact). This is basically needed for luau require trait

Another API I'll prob be adding to vfspath (at least in my fork) is the ability to just set the path with fs (e.g. VfsPath::from_path(&str, Filesystem)) which is another pretty important API that I've been working around the lack of so far.

@manuel-woelker
Copy link
Owner

Hi again, thanks for your reply!
I have to admit I probably still haven't quite grasped how these use cases require the new functions.

In general, I'd like to keep the API surface quite focused and minimal.

Regarding file_list - this should easily be implementable as a standalone function with simple using read_dir+map+collect in client space.

Using the VfsPath read to string/bytes should not be significantly slower, ReadableFile does not do any locking AFAICT. read() is just copying arrays.
Do you have any performance benchmarks?

Hiding the actual filesystem is a deliberate choice, to clearly and strongly separate the consumer facing API (VfsPath) and the Implementor side (trait FileSystem), so that they can evolve independently.

Maybe you could show some concrete example in the lua integration code, to help me better understand the underlying use cases and figure out what the options are.

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

Successfully merging this pull request may close these issues.

2 participants