WIP: fix(host): BroadcastConditionVariable and the timed semaphore acquires reach the HLE's objects #157

klimovich008 · 2026-11-13T07:42:47Z

klimovich008 commented

2026-11-13 07:42:47 +00:00

Ported from Hookshot: https://github.com/NotRexed/Hookshot/pull/2765 (same commits, rebased onto Longshot main with the Longshot names).

Review there: Joey-Magnus at 563a184f, "The code looks right", asking for an in-game run with a movie before merging (the run is under Measurements): https://github.com/NotRexed/Hookshot/pull/2765#pullrequestreview-5876249386

Why

The movie framework's reply and buffer queues and NVIDIA's OS layer wait on condition variables that only BroadcastConditionVariable wakes, and the HLE never saw that Broadcast. Their waits are untimed, so a waiter got out only if some other Signal reached the same condition variable. This could explain movie hangs; it is not reproduced here, and no scene tested so far reaches these callers (Measurements):
this is hardening of those paths, not a fix of an observed hang.

What changed

The HLE replaces the nn::os condition variable and semaphore functions with host objects, but a few siblings were left to the game's own SDK code. That code acts on guest words the HLE never writes, and signals kernel keys that no HLE waiter sleeps on.

Broadcast (host/nn_os_sync.c):
- The SDK's version signals the kernel key at cv+4 (svcSignalProcessWideKey), which no HLE waiter sleeps on. The HLE's WaitConditionVariable sleeps on its host object until the object's generation moves.
- The new HLE moves the generation under cv->lk and wakes all waiters.
- In Horizon a broadcast moves every waiter onto the mutex. So each waiter that is not yet owed the mutex is owed it here, the same hand-off a Signal makes (cv_reserve): one mutex for all of them, at most 8 owed. HOS_NO_HANDOFF=1 turns it off, as for Signal.
TimedAcquireSemaphore, TimedAcquireLightSemaphore, TryAcquireLightSemaphore:
- The timed acquire never saw an HLE release. The C library's timed semaphore wait and NVIDIA's OS layer call it; the light-semaphore ones have the same gap but no direct callers.
- All three now take the HLE's count through sema_acquire_for, which waits up to the timeout. A timeout of 0 or less tries once.
wait_until: sleeps in steps of at most 0x7FFFFFFF ms, because a finite wait of 0xFFFFFFFF ms is INFINITE on Windows. Its callers already wait again until their deadline.
The timed acquire's waits are noted for HOS_CHAIN, like sema_acquire's. Its deadline saturates for a huge TimeSpan.

How these were found: the image's exports were compared with the HLE's list, per sync object type. Their callers were found
statically, in the image: the import slots holding each function's address, the stubs that load from them, and the calls to
those stubs. All are in the SDK, none in the game's own code: for Broadcast, the movie framework's message reply and its live
data source's buffer and end-of-stream queues, and NVIDIA's OS layer; for the timed acquire, nlib's semaphore wait, the C
library's timed semaphore wait and NVIDIA's OS layer. Siblings still left to the SDK that have direct callers: StopTimerEvent (the C library's timers) and RegisterSystemWorkerHandler. The HLE timer has no separate "signalled" state, so a correct Stop needs a model change; it is left for a follow-up.

Flags and settings

None new. HOS_NO_HANDOFF=1 (existing) now also turns off the Broadcast's hand-off.

Measurements

A fix: the proof of the code is the game-free test, which fails on main and passes here. In the game, the question is whether
play reaches the changed code at all, so each run below counted every call: a local log line for each Broadcast and each timed
or try acquire, added to the measured build only (not part of this PR).

main: 839e85ff (2026-11-13)
this PR: fix/cv-broadcast-hle @ d98acf80 (measured as this PR's three commits on 839e85ff, as a dev_packs pack, plus the local log line)
Hardware: RX 9070 XT, AMD 26.10.2, Ryzen 7 5700X3D, 64 GB, Windows 11
Scene: each scene from its save, 16:9, frame rate unlocked, the fixed Custom profile; one run each, about 55 s from the start
(the save loads at about 29 s, then 20+ s of play)
Method: counting runs, not an A/B: the log lines counted per run
Baseline: this PR @ d98acf80 + log line: Broadcast calls (a count needs no other arm: main has no Broadcast HLE, so main makes
no such call through the HLE by construction). The counts hold on main 514591a3: the merge changes neither of this PR's files.

scene	this PR @ `d98acf80` + log line: Broadcast calls	this PR @ `d98acf80` + log line: timed or try acquires
Hyrule Field (the standard test's save, camera turning)	0	0
Kokiri Forest (the default save)	0	0
The Temple of Time route (the Castle Town save)	0	0
The prologue movie from a new game (Hookshot `46780503`, the same change, earlier)	0	not counted

No scene tested reaches the changed code. So nothing here can show the movie-hang link, and nothing here can show a cost: with
no calls, the change does no work in these scenes.

Testing

Tested: this branch (first on Longshot main 39647e1a and 1be180bb; merged with main 514591a3 since, e76dd95e, with the checks run again there), Windows 11, Ryzen 7 5700X3D:

tests/test_os_lifted_siblings.py (new) runs the real code cut from the source (cv_enter, cv_leave, cv_reserve, the Broadcast HLE, sema_acquire_for) over counting stubs and a fake clock.
- Broadcast: with no waiters, three, one already owed plus two, ten (8 owed), and mixed mutexes. Each case is run with HOS_NO_HANDOFF unset, 1 and 0.
- Semaphore: a count already there, a zero or negative timeout, a release inside the timeout, no release, a release after the deadline, a timeout so large the deadline would wrap, and the HOS_CHAIN notes.
- Broken variants must fail: one wake instead of all, no generation move, one reservation, a single wait, taking a count of 0, no decrement.
- A line check pins the three acquire HLEs to sema_acquire_for, and wait_until's clamp below INFINITE.
- It fails on main, which has no Broadcast HLE.
tests/test_multiwait_wake.py passes.
python -I -B tests/run_all.py on this branch merged with main 514591a3 (LONGSHOT_MINGW = llvm-mingw 20261104): 496 passed, 4 failed, 34 skipped.
The failures fail the same way on unmodified main: nx2/test_nup_paths.py, tests/test_check_third_party.py, tests/test_menu_i18n.py (fixed by #273), tests/test_release.py.
Syntax compile of every host/*.c (llvm-mingw clang, against a build's generated runtime headers): the changed files compile. 5 files stop at library headers this PC lacks (dav1d, zstd, opus, libavcodec), the same on main.
On Hookshot: the in-game run above, and a gpt-6.1-sol review (three rounds, the last clean).

Not tested: a scene that reaches Broadcast in the game (none of the four above does), so whether this fixes a movie hang
is not shown; Linux (test request T-ZD24); macOS.

Needs other testers:

Linux (any GPU): build the host, boot to the title and play the prologue movie, expecting it to play to its end with no error
lines (T-ZD24)

Screenshots

Nothing on screen.

Risk and rollback

Every Broadcast caller (the movie framework's queues, NVIDIA's OS layer) slept until some unrelated Signal; now they wake on time. That changes timing on movie and NVN OS-layer paths. A wrong wake or hand-off would show as a stall or a hang on those paths. HOS_NO_HANDOFF=1 turns the hand-off off; reverting the commits restores the SDK's own Broadcast and acquires.

Reviewer notes

Merge order with #156 (three per-call costs, including Signal's early return): both change host/nn_os_sync.c next to Signal and merge without a textual conflict. Whichever lands second needs a short follow-up. That PR counts a waiter in (cv_waiting) before it takes the condition variable's lock, and keeps the waiters under the lock in cv_in:

the Broadcast then reserves the mutex for cv_in - cv_owed instead of cv_waiting - cv_owed;
this PR's test fixture gains cv_in;
a new test case: a waiter counted in but not yet under the lock must not be owed the mutex.

That fix goes in whichever PR merges second, in the same PR, not as a follow-up (HumanBean): the Broadcast counting from
cv_in, the fixture's cv_in, and the new case. The Signal change may now land through TrashPanda's #288 instead of #156;
the rule is the same.

Left for follow-ups, named here (HumanBean's review):

StopTimerEvent and RegisterSystemWorkerHandler (above).
A condition variable finalized and initialized again at the same address while woken waiters still owe the mutex: the
tombstone reuse clears cv_owed, cv_waiting and gen, but the mutex keeps the reserved count those reservations
added, so ordinary lockers are refused for good. Pre-existing; Broadcast can leak up to 8 reservations where Signal leaked
one. Fix: give back cv_owed_mutex[0..cv_owed) when a K_COND is dropped or made fresh.
TimedWaitConditionVariable, event_wait and the timed queue receive still compute now + timeout without saturating, so
a TimeSpan near INT64_MAX wraps to a deadline in the past. Fix: one shared deadline_after(ns) with sema_acquire_for's
saturation.

Issues

None.

Checklist

For everyone

One topic.
The title is a Conventional Commit (type(scope): description).
Rebased on current main.
python -I -B tests/run_all.py run and passing (any failure that also fails on main is named above).
Tested in the game, or what wasn't is named under Testing.
Testing filled in: what was tested, what wasn't, and what needs other testers (with test requests).
Measured: main and this PR at named commits, on the same PC, scene and settings, at least 2 interleaved runs each, the median reported. (No scene tested reaches the changed code: 0 calls in four scenes, so there is nothing to measure yet; it stays WIP until a scene that reaches it is found.)
No game files, dumps, build output or generated files.
third_party/ untouched (a new folder with a PROVENANCE.md instead).
The license is OK: GPL-3.0-or-later, nothing incompatible and nothing decompiled.

If applicable

AI assistance recorded with Assisted-by: / Co-Authored-By: trailers.
A game-free test added for the fix, failing on main and passing here.
Host code builds on both Windows (llvm-mingw) and Linux, with UTF-8 paths. (Windows syntax compile here; Linux asked in test request T-ZD24.)

🤖 Generated with Claude Code

Ported from Hookshot: https://github.com/NotRexed/Hookshot/pull/2765 (same commits, rebased onto Longshot main with the Longshot names). Review there: Joey-Magnus at 563a184f, "The code looks right", asking for an in-game run with a movie before merging (the run is under Measurements): https://github.com/NotRexed/Hookshot/pull/2765#pullrequestreview-5876249386 ## Why The movie framework's reply and buffer queues and NVIDIA's OS layer wait on condition variables that only BroadcastConditionVariable wakes, and the HLE never saw that Broadcast. Their waits are untimed, so a waiter got out only if some other Signal reached the same condition variable. This could explain movie hangs; it is not reproduced here, and no scene tested so far reaches these callers (Measurements): this is hardening of those paths, not a fix of an observed hang. ## What changed The HLE replaces the nn::os condition variable and semaphore functions with host objects, but a few siblings were left to the game's own SDK code. That code acts on guest words the HLE never writes, and signals kernel keys that no HLE waiter sleeps on. - **Broadcast** (`host/nn_os_sync.c`): - The SDK's version signals the kernel key at cv+4 (`svcSignalProcessWideKey`), which no HLE waiter sleeps on. The HLE's `WaitConditionVariable` sleeps on its host object until the object's generation moves. - The new HLE moves the generation under `cv->lk` and wakes all waiters. - In Horizon a broadcast moves every waiter onto the mutex. So each waiter that is not yet owed the mutex is owed it here, the same hand-off a Signal makes (`cv_reserve`): one mutex for all of them, at most 8 owed. `HOS_NO_HANDOFF=1` turns it off, as for Signal. - **`TimedAcquireSemaphore`, `TimedAcquireLightSemaphore`, `TryAcquireLightSemaphore`:** - The timed acquire never saw an HLE release. The C library's timed semaphore wait and NVIDIA's OS layer call it; the light-semaphore ones have the same gap but no direct callers. - All three now take the HLE's count through `sema_acquire_for`, which waits up to the timeout. A timeout of 0 or less tries once. - **`wait_until`:** sleeps in steps of at most 0x7FFFFFFF ms, because a finite wait of 0xFFFFFFFF ms is `INFINITE` on Windows. Its callers already wait again until their deadline. - The timed acquire's waits are noted for `HOS_CHAIN`, like `sema_acquire`'s. Its deadline saturates for a huge TimeSpan. How these were found: the image's exports were compared with the HLE's list, per sync object type. Their callers were found statically, in the image: the import slots holding each function's address, the stubs that load from them, and the calls to those stubs. All are in the SDK, none in the game's own code: for Broadcast, the movie framework's message reply and its live data source's buffer and end-of-stream queues, and NVIDIA's OS layer; for the timed acquire, nlib's semaphore wait, the C library's timed semaphore wait and NVIDIA's OS layer. Siblings still left to the SDK that have direct callers: `StopTimerEvent` (the C library's timers) and `RegisterSystemWorkerHandler`. The HLE timer has no separate "signalled" state, so a correct Stop needs a model change; it is left for a follow-up. ## Flags and settings None new. `HOS_NO_HANDOFF=1` (existing) now also turns off the Broadcast's hand-off. ## Measurements A `fix`: the proof of the code is the game-free test, which fails on main and passes here. In the game, the question is whether play reaches the changed code at all, so each run below counted every call: a local log line for each Broadcast and each timed or try acquire, added to the measured build only (not part of this PR). main: 839e85ff (2026-11-13) this PR: fix/cv-broadcast-hle @ d98acf80 (measured as this PR's three commits on 839e85ff, as a dev_packs pack, plus the local log line) Hardware: RX 9070 XT, AMD 26.10.2, Ryzen 7 5700X3D, 64 GB, Windows 11 Scene: each scene from its save, 16:9, frame rate unlocked, the fixed Custom profile; one run each, about 55 s from the start (the save loads at about 29 s, then 20+ s of play) Method: counting runs, not an A/B: the log lines counted per run Baseline: this PR @ d98acf80 + log line: Broadcast calls (a count needs no other arm: main has no Broadcast HLE, so main makes no such call through the HLE by construction). The counts hold on main 514591a3: the merge changes neither of this PR's files. | scene | this PR @ d98acf80 + log line: Broadcast calls | this PR @ d98acf80 + log line: timed or try acquires | |---|---|---| | Hyrule Field (the standard test's save, camera turning) | 0 | 0 | | Kokiri Forest (the default save) | 0 | 0 | | The Temple of Time route (the Castle Town save) | 0 | 0 | | The prologue movie from a new game (Hookshot 46780503, the same change, earlier) | 0 | not counted | No scene tested reaches the changed code. So nothing here can show the movie-hang link, and nothing here can show a cost: with no calls, the change does no work in these scenes. ## Testing **Tested:** this branch (first on Longshot main 39647e1a and 1be180bb; merged with main 514591a3 since, e76dd95e, with the checks run again there), Windows 11, Ryzen 7 5700X3D: - `tests/test_os_lifted_siblings.py` (new) runs the real code cut from the source (`cv_enter`, `cv_leave`, `cv_reserve`, the Broadcast HLE, `sema_acquire_for`) over counting stubs and a fake clock. - **Broadcast:** with no waiters, three, one already owed plus two, ten (8 owed), and mixed mutexes. Each case is run with `HOS_NO_HANDOFF` unset, `1` and `0`. - **Semaphore:** a count already there, a zero or negative timeout, a release inside the timeout, no release, a release after the deadline, a timeout so large the deadline would wrap, and the `HOS_CHAIN` notes. - **Broken variants must fail:** one wake instead of all, no generation move, one reservation, a single wait, taking a count of 0, no decrement. - A line check pins the three acquire HLEs to `sema_acquire_for`, and `wait_until`'s clamp below `INFINITE`. - It fails on main, which has no Broadcast HLE. - `tests/test_multiwait_wake.py` passes. - `python -I -B tests/run_all.py` on this branch merged with main 514591a3 (`LONGSHOT_MINGW` = llvm-mingw 20261104): 496 passed, 4 failed, 34 skipped. The failures fail the same way on unmodified main: nx2/test_nup_paths.py, tests/test_check_third_party.py, tests/test_menu_i18n.py (fixed by #273), tests/test_release.py. - Syntax compile of every `host/*.c` (llvm-mingw clang, against a build's generated runtime headers): the changed files compile. 5 files stop at library headers this PC lacks (dav1d, zstd, opus, libavcodec), the same on main. - On Hookshot: the in-game run above, and a gpt-6.1-sol review (three rounds, the last clean). **Not tested:** a scene that reaches Broadcast in the game (none of the four above does), so whether this fixes a movie hang is not shown; Linux (test request T-ZD24); macOS. **Needs other testers:** - Linux (any GPU): build the host, boot to the title and play the prologue movie, expecting it to play to its end with no error lines (T-ZD24) ## Screenshots Nothing on screen. ## Risk and rollback Every Broadcast caller (the movie framework's queues, NVIDIA's OS layer) slept until some unrelated Signal; now they wake on time. That changes timing on movie and NVN OS-layer paths. A wrong wake or hand-off would show as a stall or a hang on those paths. `HOS_NO_HANDOFF=1` turns the hand-off off; reverting the commits restores the SDK's own Broadcast and acquires. ## Reviewer notes **Merge order with #156 (three per-call costs, including Signal's early return):** both change `host/nn_os_sync.c` next to Signal and merge without a textual conflict. Whichever lands second needs a short follow-up. That PR counts a waiter in (`cv_waiting`) before it takes the condition variable's lock, and keeps the waiters under the lock in `cv_in`: - the Broadcast then reserves the mutex for `cv_in - cv_owed` instead of `cv_waiting - cv_owed`; - this PR's test fixture gains `cv_in`; - a new test case: a waiter counted in but not yet under the lock must not be owed the mutex. That fix goes in whichever PR merges second, in the same PR, not as a follow-up (HumanBean): the Broadcast counting from `cv_in`, the fixture's `cv_in`, and the new case. The Signal change may now land through TrashPanda's #288 instead of #156; the rule is the same. Left for follow-ups, named here (HumanBean's review): - `StopTimerEvent` and `RegisterSystemWorkerHandler` (above). - A condition variable finalized and initialized again at the same address while woken waiters still owe the mutex: the tombstone reuse clears `cv_owed`, `cv_waiting` and `gen`, but the mutex keeps the `reserved` count those reservations added, so ordinary lockers are refused for good. Pre-existing; Broadcast can leak up to 8 reservations where Signal leaked one. Fix: give back `cv_owed_mutex[0..cv_owed)` when a K_COND is dropped or made fresh. - `TimedWaitConditionVariable`, `event_wait` and the timed queue receive still compute `now + timeout` without saturating, so a TimeSpan near INT64_MAX wraps to a deadline in the past. Fix: one shared `deadline_after(ns)` with `sema_acquire_for`'s saturation. ## Issues None. ## Checklist ### For everyone - [x] One topic. - [x] The title is a Conventional Commit (`type(scope): description`). - [x] Rebased on current `main`. - [x] `python -I -B tests/run_all.py` run and passing (any failure that also fails on `main` is named above). - [x] Tested in the game, or what wasn't is named under Testing. - [x] Testing filled in: what was tested, what wasn't, and what needs other testers (with test requests). - [ ] Measured: main and this PR at named commits, on the same PC, scene and settings, at least 2 interleaved runs each, the median reported. (No scene tested reaches the changed code: 0 calls in four scenes, so there is nothing to measure yet; it stays WIP until a scene that reaches it is found.) - [x] No game files, dumps, build output or generated files. - [x] `third_party/` untouched (a new folder with a `PROVENANCE.md` instead). - [x] The license is OK: GPL-3.0-or-later, nothing incompatible and nothing decompiled. ### If applicable - [x] AI assistance recorded with `Assisted-by:` / `Co-Authored-By:` trailers. - [x] A game-free test added for the fix, failing on `main` and passing here. - [ ] Host code builds on both Windows (llvm-mingw) and Linux, with UTF-8 paths. (Windows syntax compile here; Linux asked in test request T-ZD24.) 🤖 Generated with [Claude Code](https://claude.com/claude-code)

klimovich008 added 3 commits

2026-11-13 07:42:47 +00:00

fix(host): BroadcastConditionVariable and the timed/try semaphore acquires on the HLE's objects 69dd9d20bd

The HLE replaces the nn::os condition variable and semaphore functions with host objects, but left some siblings to the
game's own SDK code, which acts on guest words the HLE never writes and signals kernel keys no HLE waiter sleeps on.
- BroadcastConditionVariable woke nobody (the movie framework and NVIDIA's OS layer call it, with untimed waiters). It
  now moves the generation and wakes all, and each waiter not yet owed the mutex is owed it (the EvilOOT hand-off;
  HOS_NO_HANDOFF=1: none).
- TimedAcquireSemaphore, TimedAcquireLightSemaphore and TryAcquireLightSemaphore take the HLE's count
  (sema_acquire_for). Its deadline saturates, and its waits are noted for HOS_CHAIN.
- wait_until sleeps in bounded steps: a finite 0xFFFFFFFF ms is INFINITE on Windows.

Co-Authored-By: Claude Opus 5.5 <[email protected]>

test: pin wait_until's clamp below INFINITE; comments name the callers by role, not by symbol 93bd468784

The fixture stubs wait_until, so a line check now pins its 0x7FFFFFFF ms cap (hckrman101's review). The comments in
host/nn_os_sync.c and the test's docstring name the Broadcast and timed-acquire callers by role (the movie framework's
reply and buffer queues, NVIDIA's OS layer, the C library) rather than by the image's symbols, as the description does.

Co-Authored-By: Claude Opus 5.5 <[email protected]>

chore: Longshot's names (the env header, the HLE comments' tag)

PR description / Description follows the template (pull_request_target) Successful in 4s

Details

CI / Conventional Commit title (pull_request) Has been cancelled

Details

CI / Python source checks (ubuntu-24.04, part 1/2) (pull_request) Has been cancelled

Details

CI / Python source checks (ubuntu-24.04, part 2/2) (pull_request) Has been cancelled

Details

CI / SPIR-V fixtures (global narrow load/store, direct surface atomics, F2I immediate) (pull_request) Has been cancelled

Details

CI / Workflow lint (actionlint) (pull_request) Has been cancelled

Details

Third-party parts / As their notes say (pull_request) Has been cancelled

Details

Third-party parts / The same as upstream (pull_request) Has been cancelled

Details

d98acf8023

Co-Authored-By: Claude Opus 5.5 <[email protected]>

klimovich008 referenced this pull request

2026-11-13 07:42:55 +00:00

WIP: perf(host): three per-call costs on hot paths removed (own-thread lookups, a Signal with no waiter, switches read once) #156

HumanBean requested changes

2026-11-13 16:19:05 +00:00

HumanBean left a comment

The code does what it says, and I read it against nn::os:

Broadcast: it moves gen under cv->lk and wakes everyone after the lock is let go, so no wakeup is lost (a waiter records gen under the same lock before it gives the mutex back). Each waiter not yet owed the mutex is owed it (cv_waiting - cv_owed, capped by cv_reserve's 8 and switched off by cv_mixed and HOS_NO_HANDOFF). That matches Horizon's SignalProcessWideKey(-1), which moves every waiter onto the mutex. Waiters past the 8th fall back to mutex_lock, and mutex_give's wake-all while reserved > 0 keeps them from stalling.
Timed acquires: TimeSpan is int64 nanoseconds passed in x1, as TimedWaitEvent already reads it. <= 0 tries once. The deadline saturates. The loop copes with spurious wakeups and the vcore away/back re-lock. The bool return is the count it took. wait_until's clamp to 0x7FFFFFFF ms fixes a real truncation, where a long finite wait could become INFINITE or 0 ms. On Linux, rel_ms() and the futex take 0x7FFFFFFF ms without overflow.
Hooking: nx2/standalone.py hooks an HLE() symbol at the sdk's own definition as well as at the imports, so SDK-internal callers reach it too. The four mangled names are spelled correctly.
Main's dependence on the old behaviour: I found none. The decoder limit from #110 (in this PR's base) paces through hos_system_event_*, not a condition variable or a timed semaphore. Your own run shows the prologue movie makes no Broadcast call, so its numbers should not move. Nothing else under host/ references these symbols.
The test cuts the real functions out of the source, and its broken variants are the right ones.

Must change

"Rebased on current main" is ticked but not true. The PR sits on 1be180bb, and main has about 29 commits since then (#78, #147, #197 and others). host/nn_os_sync.c has not changed on main since the fork, and the test file is new, so the rebase should be clean. Rebase and run the checks again, or untick the box.
The in-game run never reaches the change. In the prologue movie Broadcast was called 0 times, so the run Joey-Magnus asked for in https://github.com/NotRexed/Hookshot/pull/2765 shows no regression on a path that this PR leaves untouched. The Why says the movie framework's reply and buffer queues wait on condition variables that only Broadcast wakes, but a movie that played through called Broadcast 0 times. Please do three things:
- Say where the callers were found: static cross-references in the image, or a run.
- Find a scene that does call Broadcast or TimedAcquireSemaphore (keep the local log line).
- Run that scene on a Longshot build.
Until then it stays WIP, as the Measured box already says.

Should change

There is no CI to hand Linux to. The Linux box says "left to CI", but Forgejo runs no checks (AGENTS.md). Ask for a Linux build and run through a test request (scripts/test_requests.py), and change the reason beside the box.
Re-initialising the condition variable with waiters still inside can hang its mutex (pre-existing, but Broadcast makes it up to 8 times worse). Consider "Broadcast, then Finalize and Initialize the condition variable at the same address before the woken waiters reach cv_leave". obj_lookup reuses the tombstone and memsets cv_owed, cv_waiting and gen. The reserved count those reservations added to the mutex stays, so mutex_take refuses every ordinary locker of that mutex for good. The gen reset to 0 can also put back to sleep a waiter that saved gen == 0. With Signal this leaked one reservation, and with Broadcast it leaks up to 8. A small fix would give back cv_owed_mutex[0..cv_owed) (reserved--) when a K_COND is dropped or made fresh. A follow-up is fine, but name it in the PR.
Other timed calls still wrap on a huge TimeSpan. The new wait_until comment says callers wait again until the deadline. But TimedWaitConditionVariable (hos_now_ns() + ARG(2)), event_wait and the timed queue receive still compute now + timeout without saturating. A TimeSpan near INT64_MAX, used as a "forever" timeout, overflows to a negative deadline and times out at once. A shared deadline_after(ns) with the saturation from sema_acquire_for would fix all of them. This is optional for this PR.

The code does what it says, and I read it against nn::os: - **Broadcast**: it moves `gen` under `cv->lk` and wakes everyone after the lock is let go, so no wakeup is lost (a waiter records `gen` under the same lock before it gives the mutex back). Each waiter not yet owed the mutex is owed it (`cv_waiting - cv_owed`, capped by `cv_reserve`'s 8 and switched off by `cv_mixed` and `HOS_NO_HANDOFF`). That matches Horizon's `SignalProcessWideKey(-1)`, which moves every waiter onto the mutex. Waiters past the 8th fall back to `mutex_lock`, and `mutex_give`'s wake-all while `reserved > 0` keeps them from stalling. - **Timed acquires**: `TimeSpan` is int64 nanoseconds passed in x1, as `TimedWaitEvent` already reads it. `<= 0` tries once. The deadline saturates. The loop copes with spurious wakeups and the vcore away/back re-lock. The bool return is the count it took. `wait_until`'s clamp to 0x7FFFFFFF ms fixes a real truncation, where a long finite wait could become `INFINITE` or 0 ms. On Linux, `rel_ms()` and the futex take 0x7FFFFFFF ms without overflow. - **Hooking**: `nx2/standalone.py` hooks an `HLE()` symbol at the sdk's own definition as well as at the imports, so SDK-internal callers reach it too. The four mangled names are spelled correctly. - **Main's dependence on the old behaviour**: I found none. The decoder limit from https://git.magicbean.dev/MagicBeanDev/Longshot/pulls/110 (in this PR's base) paces through `hos_system_event_*`, not a condition variable or a timed semaphore. Your own run shows the prologue movie makes no Broadcast call, so its numbers should not move. Nothing else under `host/` references these symbols. - The test cuts the real functions out of the source, and its broken variants are the right ones. ## Must change - **"Rebased on current `main`" is ticked but not true.** The PR sits on 1be180bb, and `main` has about 29 commits since then (https://git.magicbean.dev/MagicBeanDev/Longshot/pulls/78, https://git.magicbean.dev/MagicBeanDev/Longshot/pulls/147, https://git.magicbean.dev/MagicBeanDev/Longshot/pulls/197 and others). `host/nn_os_sync.c` has not changed on main since the fork, and the test file is new, so the rebase should be clean. Rebase and run the checks again, or untick the box. - **The in-game run never reaches the change.** In the prologue movie Broadcast was called 0 times, so the run Joey-Magnus asked for in https://github.com/NotRexed/Hookshot/pull/2765 shows no regression on a path that this PR leaves untouched. The *Why* says the movie framework's reply and buffer queues wait on condition variables that only Broadcast wakes, but a movie that played through called Broadcast 0 times. Please do three things: - Say where the callers were found: static cross-references in the image, or a run. - Find a scene that does call Broadcast or `TimedAcquireSemaphore` (keep the local log line). - Run that scene on a Longshot build. Until then it stays WIP, as the *Measured* box already says. ## Should change - **There is no CI to hand Linux to.** The Linux box says "left to CI", but Forgejo runs no checks (AGENTS.md). Ask for a Linux build and run through a test request (`scripts/test_requests.py`), and change the reason beside the box. - **Re-initialising the condition variable with waiters still inside can hang its mutex** (pre-existing, but Broadcast makes it up to 8 times worse). Consider "Broadcast, then Finalize and Initialize the condition variable at the same address before the woken waiters reach `cv_leave`". `obj_lookup` reuses the tombstone and memsets `cv_owed`, `cv_waiting` and `gen`. The `reserved` count those reservations added to the mutex stays, so `mutex_take` refuses every ordinary locker of that mutex for good. The `gen` reset to 0 can also put back to sleep a waiter that saved `gen == 0`. With Signal this leaked one reservation, and with Broadcast it leaks up to 8. A small fix would give back `cv_owed_mutex[0..cv_owed)` (`reserved--`) when a K_COND is dropped or made fresh. A follow-up is fine, but name it in the PR. - **Other timed calls still wrap on a huge TimeSpan.** The new `wait_until` comment says callers wait again until the deadline. But `TimedWaitConditionVariable` (`hos_now_ns() + ARG(2)`), `event_wait` and the timed queue receive still compute `now + timeout` without saturating. A `TimeSpan` near INT64_MAX, used as a "forever" timeout, overflows to a negative deadline and times out at once. A shared `deadline_after(ns)` with the saturation from `sema_acquire_for` would fix all of them. This is optional for this PR.

host/nn_os_sync.c

					
				@ -283,3 +283,3 @@

				    int64_t left = deadline - hos_now_ns();

				    if (left <= 0) return 0;

				    hos_cv_sleep(&o->cv, &o->lk, (DWORD)((left + 999999) / 1000000));

				    /* (in steps of at most ~24 days: 0xFFFFFFFF ms is INFINITE; callers wait again until the deadline) */

Good catch: before this, a long finite wait could truncate to INFINITE or to 0 ms. On Linux, rel_ms() and the futex take 0x7FFFFFFF ms fine. The comment's "callers wait again until the deadline" holds for every wait_until caller as long as their deadline did not wrap (see the comment on sema_acquire_for).

Good catch: before this, a long finite wait could truncate to `INFINITE` or to 0 ms. On Linux, `rel_ms()` and the futex take 0x7FFFFFFF ms fine. The comment's "callers wait again until the deadline" holds for every `wait_until` caller as long as their deadline did not wrap (see the comment on `sema_acquire_for`).

host/nn_os_sync.c

					
				@ -589,0 +596,4 @@

				    if (watched_cv(ARG(0))) watch_log("broadcast", watch_mutex(), ARG(0), c, NULL);

				    HOS_CHAIN(HOS_CHAIN_SIGNAL, cv->key);

				    AcquireSRWLockExclusive(&cv->lk);

				    for (int n = cv->cv_waiting - cv->cv_owed; n > 0; n--) cv_reserve(cv);

Correct against Horizon's Broadcast, which moves every waiter onto the mutex. One hazard is now up to 8 times larger, though it is pre-existing. Suppose the condition variable is finalized and initialized again at the same address before these woken waiters reach cv_leave. obj_lookup's tombstone reuse then memsets cv_owed, cv_waiting and gen, but the mutex's reserved keeps these reservations, so mutex_take refuses every ordinary locker of that mutex for good. Giving back cv_owed_mutex[0..cv_owed) when a K_COND is dropped or made fresh would close it. A follow-up is fine.

Correct against Horizon's Broadcast, which moves every waiter onto the mutex. One hazard is now up to 8 times larger, though it is pre-existing. Suppose the condition variable is finalized and initialized again at the same address before these woken waiters reach `cv_leave`. `obj_lookup`'s tombstone reuse then memsets `cv_owed`, `cv_waiting` and `gen`, but the mutex's `reserved` keeps these reservations, so `mutex_take` refuses every ordinary locker of that mutex for good. Giving back `cv_owed_mutex[0..cv_owed)` when a K_COND is dropped or made fresh would close it. A follow-up is fine.

host/nn_os_sync.c

					
				@ -919,0 +934,4 @@

				/* an acquire that waits at most ns (<= 0: does not wait); whether it got one */

				static int sema_acquire_for(Obj *s, int64_t ns) {

				    const int64_t now = hos_now_ns();

				    const int64_t deadline = ns <= 0 ? now : ns > INT64_MAX - now ? INT64_MAX : now + ns;   /* (a huge TimeSpan: no wrap) */

This saturation is right. The other timed paths still compute now + timeout without it: TimedWaitConditionVariable, event_wait and the timed queue receive. A TimeSpan near INT64_MAX wraps to a negative deadline there and times out at once, so the clamp in wait_until never gets to help them. A shared deadline_after(ns) helper would cover them all.

This saturation is right. The other timed paths still compute `now + timeout` without it: `TimedWaitConditionVariable`, `event_wait` and the timed queue receive. A TimeSpan near INT64_MAX wraps to a negative deadline there and times out at once, so the clamp in `wait_until` never gets to help them. A shared `deadline_after(ns)` helper would cover them all.

HumanBean referenced this pull request

2026-11-13 17:53:57 +00:00

WIP: perf(host): three per-call costs on hot paths removed (own-thread lookups, a Signal with no waiter, switches read once) #156

HumanBean referenced this pull request

2026-11-13 18:05:40 +00:00

WIP: perf(host): three per-call costs on hot paths removed (own-thread lookups, a Signal with no waiter, switches read once) #156

klimovich008 added 2 commits

2026-11-13 20:32:17 +00:00

Merge main 2e1e2e8c into fix/cv-broadcast-hle a4eac68674

Co-Authored-By: Claude Opus 5.5 <[email protected]>

Merge main 514591a3 into fix/cv-broadcast-hle

CI / Conventional Commit title (pull_request) Blocked by required conditions

Details

CI / Python source checks (ubuntu-24.04, part 1/2) (pull_request) Blocked by required conditions

Details

CI / Python source checks (ubuntu-24.04, part 2/2) (pull_request) Blocked by required conditions

Details

CI / SPIR-V fixtures (global narrow load/store, direct surface atomics, F2I immediate) (pull_request) Blocked by required conditions

Details

CI / Workflow lint (actionlint) (pull_request) Blocked by required conditions

Details

Third-party parts / As their notes say (pull_request) Blocked by required conditions

Details

Third-party parts / The same as upstream (pull_request) Blocked by required conditions

Details

Website / Website on Cloudflare (pull_request) Blocked by required conditions

Details

PR description / Description follows the template (pull_request_target) Successful in 3s

Details

e76dd95ec6

Co-Authored-By: Claude Opus 5.5 <[email protected]>

klimovich008 commented

2026-11-13 20:32:42 +00:00

Thanks for the review, HumanBean. Each point:

Must change

Rebased: main 514591a3 is merged in (e76dd95e, no force-push; the PR's diff is still its two files). run_all on the merge: 496 passed, 4 failed, 34 skipped; the 4 (test_nup_paths, test_check_third_party, test_menu_i18n, test_release) fail the same way on unmodified 514591a3 (test_menu_i18n is #138's renamed row, fixed by #273). test_os_lifted_siblings and test_multiwait_wake pass. The box is true again.
The in-game run never reaches the change: you're right, and it still doesn't.
- Where the callers were found: statically, in the image (the import slots holding each function's address, the stubs that load them, the calls to those stubs). All are in the SDK, none in the game's own code: for Broadcast, the movie framework's message reply and its live data source's buffer and end-of-stream queues, and NVIDIA's OS layer; for the timed acquire, nlib's semaphore wait, the C library's timed semaphore wait and NVIDIA's OS layer.
- A scene that calls them: none found. With the local log line kept, a Longshot build (this PR's commits on 839e85ff) counted 0 Broadcast calls and 0 timed or try acquires in Hyrule Field, Kokiri Forest and the Temple of Time route, as the prologue movie had 0 Broadcasts on Hookshot.
- So the description now says it is hardening of those paths, not a fix of an observed hang, and it stays WIP with the Measured box unticked and that reason beside it, until a scene that reaches it turns up.

Should change

Linux: the box no longer says "left to CI"; test request T-ZD24 asks for a Linux build and the prologue movie.
Merge order with the Signal change: agreed, the fix goes in whichever PR merges second, in that PR. The description says so; the Signal change may now land through #288 rather than #156.
Re-init with owed waiters and the unsaturated now + timeout paths: both named as follow-ups in Reviewer notes, with the fixes you suggested (giving back cv_owed_mutex[0..cv_owed) on a K_COND's drop or fresh init; one shared deadline_after(ns)).

Thanks for the review, HumanBean. Each point: **Must change** 1. **Rebased:** main 514591a3 is merged in (e76dd95e, no force-push; the PR's diff is still its two files). `run_all` on the merge: 496 passed, 4 failed, 34 skipped; the 4 (`test_nup_paths`, `test_check_third_party`, `test_menu_i18n`, `test_release`) fail the same way on unmodified 514591a3 (`test_menu_i18n` is #138's renamed row, fixed by #273). `test_os_lifted_siblings` and `test_multiwait_wake` pass. The box is true again. 2. **The in-game run never reaches the change:** you're right, and it still doesn't. - Where the callers were found: statically, in the image (the import slots holding each function's address, the stubs that load them, the calls to those stubs). All are in the SDK, none in the game's own code: for Broadcast, the movie framework's message reply and its live data source's buffer and end-of-stream queues, and NVIDIA's OS layer; for the timed acquire, nlib's semaphore wait, the C library's timed semaphore wait and NVIDIA's OS layer. - A scene that calls them: none found. With the local log line kept, a Longshot build (this PR's commits on 839e85ff) counted 0 Broadcast calls and 0 timed or try acquires in Hyrule Field, Kokiri Forest and the Temple of Time route, as the prologue movie had 0 Broadcasts on Hookshot. - So the description now says it is hardening of those paths, not a fix of an observed hang, and it stays WIP with the Measured box unticked and that reason beside it, until a scene that reaches it turns up. **Should change** - **Linux:** the box no longer says "left to CI"; test request T-ZD24 asks for a Linux build and the prologue movie. - **Merge order with the Signal change:** agreed, the fix goes in whichever PR merges second, in that PR. The description says so; the Signal change may now land through #288 rather than #156. - **Re-init with owed waiters** and **the unsaturated `now + timeout` paths:** both named as follow-ups in Reviewer notes, with the fixes you suggested (giving back `cv_owed_mutex[0..cv_owed)` on a K_COND's drop or fresh init; one shared `deadline_after(ns)`).

klimovich008 commented

2026-11-13 20:32:59 +00:00

Description only: the Measurements' Baseline line names its column, and says why the counts hold on main 514591a3 (the merge changes neither of this PR's files).

Description only: the Measurements' Baseline line names its column, and says why the counts hold on main 514591a3 (the merge changes neither of this PR's files).

CI / Conventional Commit title (pull_request) Blocked by required conditions

Details

CI / Python source checks (ubuntu-24.04, part 1/2) (pull_request) Blocked by required conditions

Details

CI / Python source checks (ubuntu-24.04, part 2/2) (pull_request) Blocked by required conditions

Details

CI / SPIR-V fixtures (global narrow load/store, direct surface atomics, F2I immediate) (pull_request) Blocked by required conditions

Details

CI / Workflow lint (actionlint) (pull_request) Blocked by required conditions

Details

Third-party parts / As their notes say (pull_request) Blocked by required conditions

Details

Third-party parts / The same as upstream (pull_request) Blocked by required conditions

Details

Website / Website on Cloudflare (pull_request) Blocked by required conditions

Details

PR description / Description follows the template (pull_request_target) Successful in 3s

Details

This pull request is marked as a work in progress.

Some workflows are waiting to be reviewed.

View command line instructions

Checkout

From your project repository, check out a new branch and test the changes.

git fetch -u 'fix/cv-broadcast-hle:klimovich008-fix/cv-broadcast-hle'

git switch 'klimovich008-fix/cv-broadcast-hle'

Sign in to join this conversation.

No reviewers

No milestone

No project

No assignees

2 participants

Notifications

Due date

The due date is invalid or out of range. Please use the format "yyyy-mm-dd".

No due date set.

Dependencies

No dependencies set

Reference

MagicBeanDev/Longshot!157

Rows
Columns