MagicBeanDev/Hookshot

Archived

Watch

Fork 0

perf(os): wake WaitAny only for objects a multi-wait watches #1353

Merged

klimovich008 merged 6 commits from host-multiwait-targeted-wake into main

2026-11-10 04:25:39 +00:00

klimovich008 commented

2026-11-09 22:42:19 +00:00

(Migrated from github.com)

What this fixes

In offline OoT, unrelated event, timer, semaphore and queue changes repeatedly wake online-service threads in WaitAny, consuming CPU while their events remain idle.

Why merge

Filters only changes to objects no multi-wait watches. State updates and individual notifications stay unchanged.
Counts follow guest addresses across reset and teardown; Link/Unlink avoid target lookups.
Keeps the 5 ms recheck. Allocation failure restores broadcasts; HOS_MW_WAKE=all forces the old policy.

Change

host/nn_os_sync.c: track watched addresses under mw_lock and maintain holder counts.
tests/test_multiwait_wake.py: check lifetimes, notification filtering and allocation failure.
tests/run_all.py: register the check.

Tested

python -I -B tests/test_multiwait_wake.py --cxx clang++ compiles the real table, poke and holder operations with counting stubs and injected allocation failure. Fails on the old code, passes now.
v2.0.11 Hyrule, 32:9, three alternating runs per arm, same executable, HOS_MW_WAKE=all control: CPU ms/frame −6.4%; median fps 55.5 → 57.3, every pair higher.
Windows, RX 9070 XT: Hyrule/Kokiri/Temple, 96–97 seconds each; expected geometry, original saves unchanged, no fatal markers. Existing VUID-VkImageCreateInfo-imageCreateMaxMipLevels-02251 remains.

Not covered

Tested on AMD Windows only; Linux and smaller CPUs untested.
Native scene pass precedes the allocation fallback. No save-write/reload cycle or long soak.

## What this fixes In offline OoT, unrelated event, timer, semaphore and queue changes repeatedly wake online-service threads in WaitAny, consuming CPU while their events remain idle. ## Why merge - Filters only changes to objects no multi-wait watches. State updates and individual notifications stay unchanged. - Counts follow guest addresses across reset and teardown; Link/Unlink avoid target lookups. - Keeps the 5 ms recheck. Allocation failure restores broadcasts; `HOS_MW_WAKE=all` forces the old policy. ## Change - `host/nn_os_sync.c`: track watched addresses under `mw_lock` and maintain holder counts. - `tests/test_multiwait_wake.py`: check lifetimes, notification filtering and allocation failure. - `tests/run_all.py`: register the check. ## Tested - `python -I -B tests/test_multiwait_wake.py --cxx clang++` compiles the real table, poke and holder operations with counting stubs and injected allocation failure. Fails on the old code, passes now. - v2.0.11 Hyrule, 32:9, three alternating runs per arm, same executable, `HOS_MW_WAKE=all` control: CPU ms/frame −6.4%; median fps 55.5 → 57.3, every pair higher. - Windows, RX 9070 XT: Hyrule/Kokiri/Temple, 96–97 seconds each; expected geometry, original saves unchanged, no fatal markers. Existing `VUID-VkImageCreateInfo-imageCreateMaxMipLevels-02251` remains. ## Not covered - Tested on AMD Windows only; Linux and smaller CPUs untested. - Native scene pass precedes the allocation fallback. No save-write/reload cycle or long soak.

hckrman101 (Migrated from github.com) reviewed

2026-11-09 23:25:09 +00:00

hckrman101 (Migrated from github.com) left a comment

Nice find on the npln wakeups. I went through the ordering and the main path looks right. A signal stores under e->lk and then does a full-barrier read of mw_linked, and Link increments under mw_lock before any WaitAny scans, so signal-vs-link can't drop a wakeup. Linking while a WaitAny is already asleep behaves the same as before (neither version pokes on link). MoveAll keeps the counts. CI is green.

Two things around object lifetime, both inline:

The count lives on the target Obj, so re-initializing the target zeroes it (the memset in obj_lookup's same-address tombstone path) while holders still link that address. Several HLE getters re-run event_init on every call, so this can happen in practice. After that, signals on a watched object skip the poke and WaitAny only notices on its 5 ms recheck. It's added latency, not a hang.
holder_target uses obj_lookup, which creates objects. Unlink/UnlinkAll now looks up the target at teardown, often after it was finalized. That brings the tombstone back as a live object, and if the address has since been reused for another kind it wipes that live object.

Tests: both fixtures are single-threaded with stubbed lookups. They check the poke filter and the link/unlink counts but no races, and no re-init or finalize-before-unlink. A case that re-initializes a linked event and then signals it would cover (1).

Nit: the comment and the checkpoints_locked check say obj must never run under mw_lock, but wait_any → holder_ready already calls obj() under it. That's fine as-is, the wording just overstates it.

Nice find on the npln wakeups. I went through the ordering and the main path looks right. A signal stores under `e->lk` and then does a full-barrier read of `mw_linked`, and Link increments under `mw_lock` before any WaitAny scans, so signal-vs-link can't drop a wakeup. Linking while a WaitAny is already asleep behaves the same as before (neither version pokes on link). MoveAll keeps the counts. CI is green. Two things around object lifetime, both inline: 1. The count lives on the target `Obj`, so re-initializing the target zeroes it (the `memset` in `obj_lookup`'s same-address tombstone path) while holders still link that address. Several HLE getters re-run `event_init` on every call, so this can happen in practice. After that, signals on a watched object skip the poke and WaitAny only notices on its 5 ms recheck. It's added latency, not a hang. 2. `holder_target` uses `obj_lookup`, which creates objects. Unlink/UnlinkAll now looks up the target at teardown, often after it was finalized. That brings the tombstone back as a live object, and if the address has since been reused for another kind it wipes that live object. Tests: both fixtures are single-threaded with stubbed lookups. They check the poke filter and the link/unlink counts but no races, and no re-init or finalize-before-unlink. A case that re-initializes a linked event and then signals it would cover (1). Nit: the comment and the `checkpoints_locked` check say `obj` must never run under `mw_lock`, but `wait_any` → `holder_ready` already calls `obj()` under it. That's fine as-is, the wording just overstates it.

host/nn_os_sync.c

This field is after owner, so the re-init path in obj_lookup zeroes it: fresh() sets kind to 0, then find_dead_locked reuses the tombstone and does memset(&o->owner, ...). Holders link by guest address, so after a re-init the count says 0 while holders are still linked. For example, GetDisplayVsyncEvent, NvOsDrvQueryEvent, prepo GetCompletionEvent and AcquireAudioDeviceNotificationForOutput all call hos_system_event_init → fresh on every call. Call one of them again while a holder for that SystemEvent is linked, and later signals skip poke_multiwait, so WaitAny only notices on its 5 ms recheck. The next Unlink then takes it to -1, which pokes forever. That direction is harmless but it's wrong. Since the count is per address, the simplest fix is probably to carry mw_linked across the same-key tombstone reuse in obj_lookup (save before the memset, restore after). The spare/new-address path should keep resetting it.

This field is after `owner`, so the re-init path in `obj_lookup` zeroes it: `fresh()` sets kind to 0, then `find_dead_locked` reuses the tombstone and does `memset(&o->owner, ...)`. Holders link by guest address, so after a re-init the count says 0 while holders are still linked. For example, `GetDisplayVsyncEvent`, `NvOsDrvQueryEvent`, prepo `GetCompletionEvent` and `AcquireAudioDeviceNotificationForOutput` all call `hos_system_event_init` → `fresh` on every call. Call one of them again while a holder for that SystemEvent is linked, and later signals skip `poke_multiwait`, so WaitAny only notices on its 5 ms recheck. The next Unlink then takes it to -1, which pokes forever. That direction is harmless but it's wrong. Since the count is per address, the simplest fix is probably to carry `mw_linked` across the same-key tombstone reuse in `obj_lookup` (save before the memset, restore after). The spare/new-address path should keep resetting it.

host/nn_os_sync.c

obj_lookup creates objects, so it's risky in Unlink/UnlinkAll, which usually run at teardown. If the event was already finalized, this brings its tombstone back as a live K_EVENT, which is never buried, and the count goes to -1. If the memory now holds a different kind of object (say a mutex), obj_lookup sets that live object's kind to 0 and memsets it into an event. Before this PR, Unlink never touched the target. On the unlink side I'd use a non-creating lookup, e.g. find_unlocked(h->event, kind), and skip when it returns NULL. holder_ready has the same create-on-miss behaviour, but only while a WaitAny is actually scanning.

`obj_lookup` creates objects, so it's risky in Unlink/UnlinkAll, which usually run at teardown. If the event was already finalized, this brings its tombstone back as a live `K_EVENT`, which is never buried, and the count goes to -1. If the memory now holds a different kind of object (say a mutex), `obj_lookup` sets that live object's kind to 0 and memsets it into an event. Before this PR, Unlink never touched the target. On the unlink side I'd use a non-creating lookup, e.g. `find_unlocked(h->event, kind)`, and skip when it returns NULL. `holder_ready` has the same create-on-miss behaviour, but only while a WaitAny is actually scanning.

klimovich008 commented

2026-11-09 23:48:30 +00:00

(Migrated from github.com)

Thanks, both lifetime issues were real. Fixed in f0fe694 by moving the count off the object.

Re-init: the watched addresses and their holder counts now live in a small table under mw_lock, keyed by the guest address the holder links. fresh() and the tombstone memset no longer touch the count. The fixture re-initializes a linked event, signals it, and checks that it still wakes.
Lookups at teardown: Link, Unlink and UnlinkAll no longer look the target up at all; they use the holder's address. poke_multiwait checks the table under mw_lock, which it already took before broadcasting, and only when a WaitAny is waiting. A link before the check is seen; a link after it is followed by a scan that sees the stored state. The fixture finalizes an event before unlinking it and checks that nothing is resurrected and that no object lookup happens in link, unlink or poke.
Nit: right, wait_any / holder_ready already call obj under mw_lock. That's pre-existing, and this PR no longer adds any lookup under the lock or claims there are none.

The fixture is still single-threaded. The cross-thread argument is that every read and write of the table happens under mw_lock.

Thanks, both lifetime issues were real. Fixed in f0fe694 by moving the count off the object. - **Re-init:** the watched addresses and their holder counts now live in a small table under `mw_lock`, keyed by the guest address the holder links. `fresh()` and the tombstone `memset` no longer touch the count. The fixture re-initializes a linked event, signals it, and checks that it still wakes. - **Lookups at teardown:** `Link`, `Unlink` and `UnlinkAll` no longer look the target up at all; they use the holder's address. `poke_multiwait` checks the table under `mw_lock`, which it already took before broadcasting, and only when a WaitAny is waiting. A link before the check is seen; a link after it is followed by a scan that sees the stored state. The fixture finalizes an event before unlinking it and checks that nothing is resurrected and that no object lookup happens in link, unlink or poke. - **Nit:** right, `wait_any` / `holder_ready` already call `obj` under `mw_lock`. That's pre-existing, and this PR no longer adds any lookup under the lock or claims there are none. The fixture is still single-threaded. The cross-thread argument is that every read and write of the table happens under `mw_lock`.

NotRexed commented

2026-11-10 02:24:45 +00:00

(Migrated from github.com)

This landed in the v2.0.12 branch (merged there for testing; it reaches main with that release), so closing. Thanks!

This landed in the v2.0.12 branch (merged there for testing; it reaches `main` with that release), so closing. Thanks!

NotRexed commented

2026-11-10 03:54:25 +00:00

(Migrated from github.com)

Reopened: closing this was a mistake on my side. It was tested in the v2.0.12 branch, and it will be merged into main here, so it shows as merged. Sorry for the noise.

hckrman101 (Migrated from github.com) reviewed

2026-11-10 04:14:03 +00:00

host/nn_os_sync.c

Fixed in f0fe694. The count now lives in mw_watched, keyed by guest address, so fresh() and the tombstone memset no longer touch it, and mw_watch_remove_locked drops the entry at zero instead of going to -1. The fixture covers re-init then signal.

Fixed in f0fe694. The count now lives in `mw_watched`, keyed by guest address, so `fresh()` and the tombstone memset no longer touch it, and `mw_watch_remove_locked` drops the entry at zero instead of going to -1. The fixture covers re-init then signal.

hckrman101 (Migrated from github.com) reviewed

2026-11-10 04:14:12 +00:00

host/nn_os_sync.c

Fixed. Link/Unlink/UnlinkAll now go through holder_watch(h), which only reads h->event, so nothing on the unlink path looks the target up or recreates it. The finalize-before-unlink case is in the fixture.

Fixed. Link/Unlink/UnlinkAll now go through `holder_watch(h)`, which only reads `h->event`, so nothing on the unlink path looks the target up or recreates it. The finalize-before-unlink case is in the fixture.

hckrman101 (Migrated from github.com) approved these changes

2026-11-10 04:15:20 +00:00

hckrman101 (Migrated from github.com) left a comment

Re-reviewed f0fe694..7b9f02f. Both lifetime issues are fixed. The watcher count is now a per-address table under mw_lock, so re-initializing the target no longer zeroes it, and Link/Unlink/UnlinkAll use holder_watch(h) (just h->event), so nothing on the unlink path recreates a finalized object. The ordering argument still holds. poke_multiwait reads the table under mw_lock, the same lock Link and the WaitAny scan take, and the mw_waiting fast path is the same as before. The sticky mw_watch_incomplete fallback handles the allocation-failure undercount correctly. The fixture now covers re-init, finalize-before-unlink and OOM, and it's registered in run_all.py.

Minor, not blocking: if a holder is re-initialized or finalized while it's still linked, its watch entry leaks, because fresh() clears h->mw and Unlink then skips the remove. That only means extra wakes, i.e. the old behaviour for that address, and the dangling link itself was already there before this PR.

Looks good to me. The two Python/SPIR-V CI jobs were still pending when I checked, and everything else is green.

Re-reviewed f0fe694..7b9f02f. Both lifetime issues are fixed. The watcher count is now a per-address table under `mw_lock`, so re-initializing the target no longer zeroes it, and Link/Unlink/UnlinkAll use `holder_watch(h)` (just `h->event`), so nothing on the unlink path recreates a finalized object. The ordering argument still holds. `poke_multiwait` reads the table under `mw_lock`, the same lock Link and the WaitAny scan take, and the `mw_waiting` fast path is the same as before. The sticky `mw_watch_incomplete` fallback handles the allocation-failure undercount correctly. The fixture now covers re-init, finalize-before-unlink and OOM, and it's registered in run_all.py. Minor, not blocking: if a holder is re-initialized or finalized while it's still linked, its watch entry leaks, because `fresh()` clears `h->mw` and Unlink then skips the remove. That only means extra wakes, i.e. the old behaviour for that address, and the dangling link itself was already there before this PR. Looks good to me. The two Python/SPIR-V CI jobs were still pending when I checked, and everything else is green.

Commenting is not possible because the repository is archived.

No reviewers

hckrman101