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.

Sign up for GitHub

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

fix: limit the number of concurrent load_internal calls to avoid exce… #17377

Open
wants to merge 9 commits into
base: main
Choose a base branch
from
+34 −0
Open

fix: limit the number of concurrent load_internal calls to avoid exce… #17377

Changes from 1 commit
Commits
Show all changes
9 commits
Select commit Hold shift + click to select a range
d7d1508
fix: limit the number of concurrent load_internal calls to avoid exce…
Braymatter Jan 15, 2025
7b7cc1d
fix: Added macos file limit based on ulimit -n
Braymatter Jan 15, 2025
6789825
fix: lower default load_internal semaphore value
Braymatter Jan 15, 2025
fc58681
Merge branch 'main' into main
Braymatter Jan 15, 2025
98c9bb0
chore: Doc and update limits based on doc
Braymatter Jan 15, 2025
2e71d14
Merge branch 'main' of https://github.com/Braymatter/bevy_filecount_fix
Braymatter Jan 15, 2025
f61e0ea
fmt
Braymatter Jan 15, 2025
9b15738
Merge branch 'main' of https://github.com/bevyengine/bevy
Braymatter Mar 20, 2025
f634315
fix: import statement
Braymatter Mar 20, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
15 changes: 15 additions & 0 deletions crates/bevy_asset/src/server/mod.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -18,6 +18,7 @@ use crate::{
UntypedAssetLoadFailedEvent, UntypedHandle,
};
use alloc::sync::Arc;
use async_lock::Semaphore;
use atomicow::CowArc;
use bevy_ecs::prelude::*;
use bevy_tasks::IoTaskPool;
Expand Down Expand Up @@ -61,6 +62,9 @@ pub(crate) struct AssetServerData {
sources: AssetSources,
mode: AssetServerMode,
meta_check: AssetMetaCheck,

///Used to ensure the `asset_server` does not try to acquire more loaders (and thus `file_handles`) than the OS allows
asset_counter: Semaphore,
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This approach looks good to me, but I'm thinking we should scope this to FileAssetReader instead, as this is a "file descriptor" limitation protection? We wouldn't want this limit on an in-memory asset source

Copy link
Contributor Author

@Braymatter Braymatter Jan 15, 2025 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

My initial instinct was to keep in in file_asset as well, but I couldn't think of a way to detect when the file was actually closed/dropped, as the AssetReader returns the File back to the caller.

If I were to add the Semaphore to the AssetReader impl in file_asset.rs it would release the permit as soon as it loaded the file and returned, and not after it was finished being read/processed etc. My understanding is that the host OS will keep the file descriptor allocated until the process closes (or in Rust's case drops) the file.

I could definitely be misreading/misunderstanding something so open to suggestions.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Some previous discussion here:
https://discord.com/channels/691052431525675048/749332104487108618/1328572829930881066

We could read the limit from a package.metadata parameter and leave it to the client application developers to set, while defaulting to u32::Max or something sensible.

It would then be opt-in, and wouldn't affect network/memory etc. AssetReaders by default. The downside being client app developers would have to use the lowest limit of any of their target platforms.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In theory we could implement the Reader trait for a wrapper containing both a semaphore handle and the File, then return that.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Idk if the async_lock semaphore impl will meet the send / sync requirements there.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I took a stab at it here: Braymatter@a415213

It... compiles but the Semaphores are no longer preventing too many file handles from being opened. Interestingly it seems like the app is provisioning two FileAssetReaders based on this screenshot / log showing up twice in the xcode debugger:

image

I think this would actually be a problem anyway because you may want to load assets from the normal assets/imported_assets AND load downloaded assets from (in my case) the App sandbox fs.

There's also the issue of the AssetWriter also potentially reserving file descriptors.

We could do some kind of global/shared semaphore, but I could also be misunderstanding how some of this fits together.

Open to more suggestions - I'm unsure why this isn't working; I would expect that halving the amount of Semaphore permits would resolve the issue with two asset loaders but it doesn't seem to be the case.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think a shared semaphore between anything file-related that needs it (FileAssetReader / FileAssetWriter) makes the most sense to me. I'd like to avoid leaking internal filesystem-specific constraints into the high level asset server.

}

/// The "asset mode" the server is currently in.
Expand Down Expand Up @@ -112,6 +116,13 @@ impl AssetServer {
let (asset_event_sender, asset_event_receiver) = crossbeam_channel::unbounded();
let mut infos = AssetInfos::default();
infos.watching_for_changes = watching_for_changes;

#[cfg(target_os = "ios")]
let file_limit = 127; // The normal limit is 256, cut in half for .meta files and sub 1 because 128 still throws the occasional error (3 failed files out of 1500)

#[cfg(not(target_os = "ios"))]
let file_limit = 16000;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Source?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not much:
https://stackoverflow.com/questions/57007894/permanently-increase-file-descriptor-ulimit-in-android-without-being-root

Seemed like a reasonable limit, considering it wasn't limited at all before - it's going to vary with platform OS and I didn't find exhaustive documentation on specific limits for every OS and distro.

I'm not very concerned as I've never run into this problem on a desktop machine and my projects are bigger than most.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In terms of correctness, these values seem very magic and arbitrary. They seem like they could change at any time or even by configurable by the gamer. Is there a way to instead detect whether the limit has been reached when performing the operation instead of assuming what the limit is at compile-time?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

These values should have a primary source linked in a comment. If there is an authoritative resource, maaaayybe we can rely on them.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In general I seriously doubt that anyone is changing the file descriptor limit on ios. The ios limit of 256 is somewhat well documented online, but can always change. I ran ulimit -n on my macos machine and got 2560, which is where the other value came from.

16000 as a limit was a wild-guess.

I was unable to find a cross platform, exhaustive way to detect file system limits. As you mentioned they can change arbitrarily. If anything I would set it to 127 for every platform as most apps aren't even going to have that many assets, and the ones that do will likely have a negligible/user undetectable increase in load times.


Self {
data: Arc::new(AssetServerData {
sources,
Expand All @@ -121,6 +132,7 @@ impl AssetServer {
asset_event_receiver,
loaders,
infos: RwLock::new(infos),
asset_counter: Semaphore::new(file_limit),
}),
}
}
Expand Down Expand Up @@ -547,6 +559,9 @@ impl AssetServer {
force: bool,
meta_transform: Option<MetaTransform>,
) -> Result<UntypedHandle, AssetLoadError> {
//Wait to acquire asset permit so we don't overload the file io for the os
let _guard = self.data.asset_counter.acquire().await;

let asset_type_id = input_handle.as_ref().map(UntypedHandle::type_id);

let path = path.into_owned();
Expand Down
Loading