Skip to content

Move AssetServerStats from AssetInfos to AssetServer#24045

Open
Mysvac wants to merge 4 commits into
bevyengine:mainfrom
Mysvac:move_asset_server_stats
Open

Move AssetServerStats from AssetInfos to AssetServer#24045
Mysvac wants to merge 4 commits into
bevyengine:mainfrom
Mysvac:move_asset_server_stats

Conversation

@Mysvac
Copy link
Copy Markdown
Contributor

@Mysvac Mysvac commented Apr 30, 2026

Objective

bevy_asset: move AssetServerStats struct from AssetInfos to AssetServer.

In previous versions, adding stats.started_load_tasks required acquiring a write lock on AssetInfos, which was inefficient and prone to deadlocks (e.g., the deadlock fixed by #23980).

Now started_load_tasks has been changed to an atomic variable and moved into AssetServer, eliminating the deadlock issue entirely.

Note that acquiring read-write locks inherently requires more atomic operations, so the new implementation also achieves better performance.

Testing

  • cargo test bevy_asset, passed

Showcase

Click to view showcase
pub(crate) struct AssetInfos{
    // .....
    // stats: AssetServerStats, // removed
}

pub(crate) struct AssetServerData {
    infos: RwLock<AssetInfos>,
    // .....
    stats: AssetServerStats, // <- now
}

pub(crate) struct AssetServerStats {
    pub started_load_tasks: AtomicUsize,
}

@mnmaita mnmaita added A-Assets Load files from disk to use for things like images, models, and sounds C-Performance A change motivated by improving speed, memory usage or compile times D-Straightforward Simple bug fixes and API improvements, docs, test and examples S-Needs-Review Needs reviewer attention (from anyone!) to move forward labels Apr 30, 2026
@github-project-automation github-project-automation Bot moved this to Needs SME Triage in Assets Apr 30, 2026
@alice-i-cecile alice-i-cecile requested a review from andriyDev May 1, 2026 04:58
@alice-i-cecile alice-i-cecile added C-Bug An unexpected or incorrect behavior C-Code-Quality A section of code that is hard to understand or change X-Uncontroversial This work is generally agreed upon labels May 1, 2026
Copy link
Copy Markdown
Contributor

@andriyDev andriyDev left a comment

Choose a reason for hiding this comment

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

I'm not very excited about this change personally. It takes something simple (counting the number of loads), and makes it a whole atomics thing which is inherently more complex.

The additional mutex locks are a little sad, but they are also for "low traffic" APIs like load_folder and load_untyped_async, so I don't care that much about them.


pub(crate) fn add_started_load_tasks(&self) {
use core::sync::atomic::Ordering::Relaxed;
self.data.stats.started_load_tasks.fetch_add(1, Relaxed);
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Relaxed ordering makes me nervous. It means there's no guarantee afaik that we actually see this increment, whether in the tests or in an actual app.

@andriyDev andriyDev removed the X-Uncontroversial This work is generally agreed upon label May 3, 2026
@alice-i-cecile alice-i-cecile added X-Contentious There are nontrivial implications that should be thought through S-Waiting-on-Author The author needs to make changes or address concerns before this can be merged and removed S-Needs-Review Needs reviewer attention (from anyone!) to move forward labels May 3, 2026
@Mysvac
Copy link
Copy Markdown
Contributor Author

Mysvac commented May 4, 2026

@andriyDev

Visibility is unlikely to be a real problem. In single-threaded mode, there's no need to consider ordering — visibility is guaranteed. In multi-threaded mode, while Relaxed doesn't guarantee timely visibility, atomicity is still guaranteed: all add operations are effective and never conflict. Diagnostic functions using Relaxed may not always see the absolute latest value under high contention, but as diagnostic information, it's accurate enough. In multi-threaded environments, there might be slight delays due to visibility issues, but thanks to atomicity, no data will be lost.

Additionally, even if we change the add function to Release ordering and the diagnostic function to Acquire ordering to ensure visibility, the performance is still significantly faster than using a read-write lock. You can briefly check the implementation of a read-write lock — it uses a more complex atomic counter. In the original bevy_asset implementation, the read-write lock wasn't reused well — most modifications required Acquire + Release for locking plus a Release for unlocking — that's two atomic operations, plus some extra computation.

In fact, the overhead of atomic operations is negligible compared to I/O itself. The main goal here is diagnostic information — modifying it shouldn't involve complex issues like deadlocks.

If Relaxed ordering is not acceptable, we should use stricter ordering rather than falling back to a read-write lock.

@Mysvac
Copy link
Copy Markdown
Contributor Author

Mysvac commented May 4, 2026

Note, visibility in tests is guaranteed even with Relaxed.

First, App itself runs in a single thread — no ordering needed at all.

For internal multi-threaded functions like run_schedule and par_query, the TaskPool::scope implicitly provides synchronization. That means subsequent operations are guaranteed to see changes made by previous operations inside the scope. This ensures visibility across App statements (later operations always see earlier results) without requiring explicit memory ordering.


Normally, because of the TaskPool scope, the current frame's (or schedule stage's) diagnostic function can at least see the previous frame's (or stage's) results. (The after/before in Schedule also imply memory ordering.)

For systems without explicit ordering in the current frame, no extra guarantees are needed — they're already unordered by design.

@cart cart closed this May 5, 2026
@github-project-automation github-project-automation Bot moved this from Needs SME Triage to Done in Assets May 5, 2026
@cart cart reopened this May 5, 2026
@github-project-automation github-project-automation Bot moved this from Done to Needs SME Triage in Assets May 5, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

A-Assets Load files from disk to use for things like images, models, and sounds C-Bug An unexpected or incorrect behavior C-Code-Quality A section of code that is hard to understand or change C-Performance A change motivated by improving speed, memory usage or compile times D-Straightforward Simple bug fixes and API improvements, docs, test and examples S-Waiting-on-Author The author needs to make changes or address concerns before this can be merged X-Contentious There are nontrivial implications that should be thought through

Projects

Status: Needs SME Triage

Development

Successfully merging this pull request may close these issues.

5 participants