-
-
Notifications
You must be signed in to change notification settings - Fork 3.7k
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
fix: limit the number of concurrent load_internal calls to avoid exce… #17377
base: main
Are you sure you want to change the base?
Changes from all commits
d7d1508
7b7cc1d
6789825
fc58681
98c9bb0
2e71d14
f61e0ea
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -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; | ||
|
@@ -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, | ||
} | ||
|
||
/// The "asset mode" the server is currently in. | ||
|
@@ -112,6 +116,32 @@ impl AssetServer { | |
let (asset_event_sender, asset_event_receiver) = crossbeam_channel::unbounded(); | ||
let mut infos = AssetInfos::default(); | ||
infos.watching_for_changes = watching_for_changes; | ||
|
||
//Normal limits are cut in half to allow for .meta files and sub 1 for headroom | ||
#[cfg(target_os = "ios")] | ||
/* | ||
https://forum.vizrt.com/index.php?threads/ios-too-many-open-files-with-little-number-of-sources-receivers.250906/#:~:text=The%20number%20of%20sockets%20quickly,iOS%20and%20crashes%20the%20application. | ||
Documentation is fairly scarce on the actual limit, there is no documentation that I've been able to find from apple | ||
*/ | ||
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) | ||
|
||
/* | ||
https://krypted.com/mac-os-x/maximum-files-in-mac-os-x/ | ||
Running `ulimit -n` on a MBP M3-Max yields 2560. In empirical testing when using the exact limit | ||
some failures would still squeak through. This also leaves a small amount of headroom for direct | ||
std::fs calls by the client application | ||
*/ | ||
#[cfg(target_os = "macos")] | ||
let file_limit = 1279; | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. why are these not just CONST? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. this limit is at 256 on my mac (m4 max). There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Ah! That's weird and disappointing. If there are no objections, I'll go ahead and implement this idea, and add 256 as the default limit for osx/ios if that package.metadata field isn't present. I think long term we likely want to have some level of retry logic, and it would be good to find a reliable way to identify the descriptor limit at runtime (in a perfect world). For my app we worked around this by using multiple loading states, which works, even if inelegant. |
||
|
||
/* | ||
https://docs.pingidentity.com/pingdirectory/latest/installing_the_pingdirectory_suite_of_products/pd_ds_config_file_descriptor_limits.html#:~:text=Many%20Linux%20distributions%20have%20a,large%20number%20of%20concurrent%20connections. | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. not a fan of this source of truth, but couldn't find a better one... There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I searched around for about an hour trying to find some reasonably 'verified' information, but came to the conclusion that it's so configurable by distro/os etc. it really can be all over the place. Identifying the limit at runtime is probably the only way to be reliable. |
||
Setting this as a 'sensible' default in lieu of a cross platform way to determine file descriptor limits. For OSX/Linux we could potentially run ulimit at runtime, but client applications could also chunk their calls to asset_server | ||
as a workaround. Apps that exceed this limit would be fairly exceptional. | ||
*/ | ||
#[cfg(all(not(target_os = "macos"), not(target_os = "ios")))] | ||
let file_limit = 511; | ||
|
||
Self { | ||
data: Arc::new(AssetServerData { | ||
sources, | ||
|
@@ -121,6 +151,7 @@ impl AssetServer { | |
asset_event_receiver, | ||
loaders, | ||
infos: RwLock::new(infos), | ||
asset_counter: Semaphore::new(file_limit), | ||
}), | ||
} | ||
} | ||
|
@@ -547,6 +578,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(); | ||
|
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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:
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.