Skip to content

Wrap LOADED_MODS in a std Mutex#76

Merged
jmquigs merged 1 commit into
masterfrom
claude/mutex-loaded-mods-vD4NQ
May 7, 2026
Merged

Wrap LOADED_MODS in a std Mutex#76
jmquigs merged 1 commit into
masterfrom
claude/mutex-loaded-mods-vD4NQ

Conversation

@jmquigs
Copy link
Copy Markdown
Owner

@jmquigs jmquigs commented May 7, 2026

This is part of my attempts to fix the "static mut" issue. https://doc.rust-lang.org/edition-guide/rust-2024/static-mut-references.html

LoadedMods was a prime suspect for the kind of problems that can occur with that, and while I've never seen one in practice, it's probably just a matter of time. So previously I pulled it out into its own static and now it is a mutex.

For a long time I worried about the cost of locking in draw calls. However, when framerate is high it doesn't even show up in the benchmark scenes I tried.

My worst-case test showed little measurable regression, but it may not have been sensitive enough. For that test I used a
draw heavy ~4 minute event. There I measured 35fps median without locking (using a build based on an earlier commit that just has the static mut without mutex). Then I measured it again with this Mutex code and got 34.4, 32.0, and 33.1 median fps. But a confounder is that more players were joining in the later tests. The 95th (22-24 fps), and 99th (18-20), were bad in all cases. Its difficult to construct a true repeatable test that can possibly show a problem with this with the tools I have.

Future work:

If I wish to revisit this in the future I could replace the Mutex type with a shell that implements the same interface but just hands out static mut references - good enough for benchmarking at least.

There is also an optimization that could be done where the initial quick check consults a thread local cache view of the mod database (or perhaps, the entire mod database is just one-time copied into a thread local any time it changes). Would be better in theory because the vast majority of stuff is not modded. I don't think it's worth it right now though.

And now Claude describes some details:

Replaces the static mut LOADED_MODS: Option<LoadedModState> with static LOADED_MODS: Mutex<Option<LoadedModState>> so that concurrent access from the render thread and the deferred load thread is no longer UB. This addresses the torn-write concern previously called out in load_thread::load_resource, since both readers and the writer now go through the same lock.

LoadedModState contains raw d3d resource pointers which are not Send by default; an explicit unsafe impl Send is added (mirroring the pattern already used for LoadMsg in the loader thread).

Callers were updated to lock briefly (preselect / variant selection / mod stats) or to hold the guard for the span of the iteration (load_deferred_mods and the DIP path in check_and_render_mod).

Lock poisoning is handled at every runtime call site with a match that logs via write_log_file and falls back to a safe behavior:

  • DIP / preselect: treat as no mods to render.
  • variant select / load thread writeback / mod stats: log and skip.
  • clear / setup / load_deferred_mods: log and early-return.

Test code in mod_stats keeps unwrap since a panic on poison there is the desired test failure.

This is part of my attempts to fix the "static mut" issue.
https://doc.rust-lang.org/edition-guide/rust-2024/static-mut-references.html

LoadedMods was a prime suspect for the kind of problems that can occur with that,
and while I've never seen one in practice, it's probably just a matter of time.
So previously I pulled it out into its own static and now it is a mutex.

For a long time I worried about the cost of locking in draw calls.  However, when
framerate is high it doesn't even show up in the benchmark scenes I tried.

My worst-case test showed little measurable regression, but it may not have been
sensitive enough.  For that test I used a
draw heavy ~4 minute event.  There I measured 35fps median without locking (using a build
based on an earlier commit that just has the static mut without mutex).  Then I measured
it again with this Mutex code and got 34.4, 32.0, and 33.1 median fps.  But a confounder
is that more players were joining in the later tests.  The 95th (22-24 fps),
and 99th (18-20), were bad in all cases.  Its difficult to construct a true repeatable
test that can possibly show a problem with this with the tools I have.

Future work:

If I wish to revisit this in the future I could replace the Mutex type with a shell
that implements the same interface but just hands out static mut references - good
enough for benchmarking at least.

There is also an optimization that could be done where the initial quick check
consults a thread local cache view of the mod database (or perhaps, the entire mod
database is just one-time copied into a thread local any time it changes).  Would
be better in theory because the vast majority of stuff is not modded.  I don't
think it's worth it right now though.

And now Claude describes some details:

Replaces the `static mut LOADED_MODS: Option<LoadedModState>` with
`static LOADED_MODS: Mutex<Option<LoadedModState>>` so that concurrent
access from the render thread and the deferred load thread is no longer
UB. This addresses the torn-write concern previously called out in
`load_thread::load_resource`, since both readers and the writer now go
through the same lock.

`LoadedModState` contains raw d3d resource pointers which are not
`Send` by default; an explicit `unsafe impl Send` is added (mirroring
the pattern already used for `LoadMsg` in the loader thread).

Callers were updated to lock briefly (preselect / variant selection /
mod stats) or to hold the guard for the span of the iteration
(`load_deferred_mods` and the DIP path in `check_and_render_mod`).

Lock poisoning is handled at every runtime call site with a `match`
that logs via `write_log_file` and falls back to a safe behavior:
- DIP / preselect: treat as no mods to render.
- variant select / load thread writeback / mod stats: log and skip.
- clear / setup / load_deferred_mods: log and early-return.

Test code in mod_stats keeps `unwrap` since a panic on poison there is
the desired test failure.
@jmquigs jmquigs merged commit 14115ca into master May 7, 2026
1 check passed
@jmquigs jmquigs deleted the claude/mutex-loaded-mods-vD4NQ branch May 7, 2026 19:59
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