MagicBeanDev/Hookshot
Archived
Watch
6
0
Fork
You've already forked Hookshot
 0
Code Issues 125 Pull requests 205 Activity

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

Open
github-sync wants to merge 8 commits from github/pr/2708 into main
pull from: github/pr/2708
merge into: MagicBeanDev:main
Conversation 2 Commits 8 Files changed 7 +315 -15
github-sync commented 2026-11-12 08:30:52 +00:00
Owner

@klimovich008 opened this pull request on GitHub · 2026-11-12 07:51 UTC · original

The game calls a few nn::os and host paths thousands of times a frame, and each call does work whose answer is already known. Each change below removes that work and keeps its result. They are small, and together they are hard to measure apart (see Measured).

The change

  • The calling thread's own ThreadType (host/hos_thread.c thread_arg). GetThreadPriority, GetThreadId, ChangeThreadPriority and the others scanned every thread under threads_lock. The game asks about its own thread most of the time (GetThreadPriority(GetCurrentThread()), about 2,400 a frame in Hyrule Field).
    • When the ThreadType is the caller's own, the caller's record answers without the scan.
    • Any other ThreadType is looked up as before.
    • Same result, so no switch.
  • SignalConditionVariable with no waiter (host/nn_os_sync.c). It returns at once when the condition variable's waiter count is 0, without the lock pair and wake call. HOS_CV_SIGNAL_ALWAYS=1: as before.
    • Ordering: a waiter counts itself in (cv_enter, a seq-cst add under the condition variable's lock) before it gives the guest mutex back (mutex_release_all). A signaller that holds the mutex therefore sees the waiter, and the waiter cannot be skipped.
    • A signaller that does not hold the mutex could already miss a waiter that had not yet started to wait, before this change as well (that is the condition-variable contract).
  • Switches read once. These were read from the environment on every call: menu_vsync (RX_VK_FIFO / RX_VK_IMMEDIATE, twice a present) and HOS_FS_OVERLAY (every OpenFile). They hold for a whole run. Each is now read once and published atomically (HOS_FS_OVERLAY as one atomic pointer).
    • RX_VK_DUMP_STAGES stays a per-read getenv: vk_shader.cpp is part of the shader-cache stamp, so changing it would make every player's prepared shaders stale.
    • RX_VK_TAAU_KEYS stays a per-dispatch read: tests/test_taau_env_lookup.py pins that.

Measured

Windows 11, Ryzen 7 5700X3D (16 logical processors), RX 9070 XT, AMD driver 26.10.2. Standard Hyrule Field test: right stick held right from the load, HOS_FPS_MAX=240, 2560x720, 20 s window, warm caches, 6 alternating pairs a cell, base main 46780503 (with #2602).

These changes were measured only together with #2709 (both over the base; 8 and 6 threads: the game held to 0xFF / 0x3F):

threads arm fps CPU ms a frame p99 worst runs with a frame over 100 / 250 ms
16 main 52.85 77.3 25.7 439.1 1 / 1
16 this PR + #2709 52.7 72.9 (−5.8 %) 24.7 87.9 0 / 0
8 main 44.05 87.1 29.4 88.9 0 / 0
8 this PR + #2709 44.05 80.1 (−8.0 %) 29.0 60.5 0 / 0
6 main 41.35 80.93 33.0 136.5 1 / 0
6 this PR + #2709 40.7 (−1.6 %) 80.91 32.1 117.5 1 / 0
  • The 6-thread cell was measured with #2709's 5 µs spin, which #2709 then limited to 7+ logical processors, so it does not apply to this PR alone; no 6-thread cell was run with that limit.
  • This PR's own share is not separated: the own-thread lookup and the switch reads have no switch to compare, and the Signal change was not measured alone on this base.
  • A counter run on 46780503 (standard test, n=1) counted ~2,800 SignalConditionVariable calls a frame in the play window, none of them with a waiter: the case this early return skips. (During the load, 170-304 per 5 s found a waiter.) This counts the calls; their time is not measured separately.
  • Earlier figures on 57ed3aa8 (before #2602's fix of a main regression, 41 → 53 fps on this test) are replaced by these.

Tested

  • tests/test_thread_arg_self.py (new): thread_arg from the source, over own and other ThreadTypes. Broken variants must fail: every call scanning, the caller's record returned for any ThreadType.
  • tests/test_cv_signal_early.py (new): Signal over waiter counts, with and without HOS_CV_SIGNAL_ALWAYS. Broken variants must fail: the early return taken even with HOS_CV_SIGNAL_ALWAYS=1, the early return reading the wrong count, a waiter that does not count itself in. A structural check: cv_enter comes before mutex_release_all in both waits.
  • tests/test_vsync_row.py: one process per switch setting now (the value is read once), with a negative control.
  • tests/run_all.py; syntax compile of every changed host/*.c; a gpt-6.1-sol review (three rounds, last clean).

Not tested

  • This PR alone in the game; other scenes; Linux.

🤖 Generated with Claude Code

> [@klimovich008](https://github.com/klimovich008) opened this pull request on GitHub · 2026-11-12 07:51 UTC · [original](https://github.com/NotRexed/Hookshot/pull/2708) The game calls a few nn::os and host paths thousands of times a frame, and each call does work whose answer is already known. Each change below removes that work and keeps its result. They are small, and together they are hard to measure apart (see Measured). ## The change - **The calling thread's own ThreadType (`host/hos_thread.c` `thread_arg`).** GetThreadPriority, GetThreadId, ChangeThreadPriority and the others scanned every thread under `threads_lock`. The game asks about its own thread most of the time (`GetThreadPriority(GetCurrentThread())`, about 2,400 a frame in Hyrule Field). - When the ThreadType is the caller's own, the caller's record answers without the scan. - Any other ThreadType is looked up as before. - Same result, so no switch. - **SignalConditionVariable with no waiter (`host/nn_os_sync.c`).** It returns at once when the condition variable's waiter count is 0, without the lock pair and wake call. `HOS_CV_SIGNAL_ALWAYS=1`: as before. - **Ordering:** a waiter counts itself in (`cv_enter`, a seq-cst add under the condition variable's lock) before it gives the guest mutex back (`mutex_release_all`). A signaller that holds the mutex therefore sees the waiter, and the waiter cannot be skipped. - A signaller that does not hold the mutex could already miss a waiter that had not yet started to wait, before this change as well (that is the condition-variable contract). - **Switches read once.** These were read from the environment on every call: `menu_vsync` (`RX_VK_FIFO` / `RX_VK_IMMEDIATE`, twice a present) and `HOS_FS_OVERLAY` (every OpenFile). They hold for a whole run. Each is now read once and published atomically (`HOS_FS_OVERLAY` as one atomic pointer). - `RX_VK_DUMP_STAGES` stays a per-read `getenv`: `vk_shader.cpp` is part of the shader-cache stamp, so changing it would make every player's prepared shaders stale. - `RX_VK_TAAU_KEYS` stays a per-dispatch read: `tests/test_taau_env_lookup.py` pins that. ## Measured Windows 11, Ryzen 7 5700X3D (16 logical processors), RX 9070 XT, AMD driver 26.10.2. Standard Hyrule Field test: right stick held right from the load, `HOS_FPS_MAX=240`, 2560x720, 20 s window, warm caches, 6 alternating pairs a cell, base main 46780503 (with #2602). These changes were measured only together with #2709 (both over the base; 8 and 6 threads: the game held to `0xFF` / `0x3F`): | threads | arm | fps | CPU ms a frame | p99 | worst | runs with a frame over 100 / 250 ms | |---|---|---|---|---|---|---| | 16 | main | 52.85 | 77.3 | 25.7 | 439.1 | 1 / 1 | | 16 | this PR + #2709 | 52.7 | 72.9 (−5.8 %) | 24.7 | 87.9 | 0 / 0 | | 8 | main | 44.05 | 87.1 | 29.4 | 88.9 | 0 / 0 | | 8 | this PR + #2709 | 44.05 | 80.1 (−8.0 %) | 29.0 | 60.5 | 0 / 0 | | 6 | main | 41.35 | 80.93 | 33.0 | 136.5 | 1 / 0 | | 6 | this PR + #2709 | 40.7 (−1.6 %) | 80.91 | 32.1 | 117.5 | 1 / 0 | - The 6-thread cell was measured with #2709's 5 µs spin, which #2709 then limited to 7+ logical processors, so it does not apply to this PR alone; no 6-thread cell was run with that limit. - This PR's own share is not separated: the own-thread lookup and the switch reads have no switch to compare, and the Signal change was not measured alone on this base. - A counter run on 46780503 (standard test, n=1) counted ~2,800 SignalConditionVariable calls a frame in the play window, none of them with a waiter: the case this early return skips. (During the load, 170-304 per 5 s found a waiter.) This counts the calls; their time is not measured separately. - Earlier figures on 57ed3aa8 (before #2602's fix of a main regression, 41 → 53 fps on this test) are replaced by these. ## Tested - `tests/test_thread_arg_self.py` (new): `thread_arg` from the source, over own and other ThreadTypes. Broken variants must fail: every call scanning, the caller's record returned for any ThreadType. - `tests/test_cv_signal_early.py` (new): Signal over waiter counts, with and without `HOS_CV_SIGNAL_ALWAYS`. Broken variants must fail: the early return taken even with `HOS_CV_SIGNAL_ALWAYS=1`, the early return reading the wrong count, a waiter that does not count itself in. A structural check: `cv_enter` comes before `mutex_release_all` in both waits. - `tests/test_vsync_row.py`: one process per switch setting now (the value is read once), with a negative control. - `tests/run_all.py`; syntax compile of every changed `host/*.c`; a gpt-6.1-sol review (three rounds, last clean). ## Not tested - This PR alone in the game; other scenes; Linux. 🤖 Generated with [Claude Code](https://claude.com/claude-code) <!-- github-sync:item:2708 -->
thread_arg (GetThreadPriority, GetThreadId, ChangeThreadPriority...) scanned every thread under threads_lock; the game
asks about its own thread most of the time (GetThreadPriority(GetCurrentThread()), ~2,400 a frame in Hyrule). The
calling thread's record answers that, compared with its current ThreadType.

Co-Authored-By: Claude Opus 5.5 <[email protected]>
The waiter count is read atomically before the lock pair and wake call; waiters count themselves in under the
condition variable's lock before they give the guest mutex back, so a signaller holding the mutex sees them.
HOS_CV_SIGNAL_ALWAYS=1: as before.

Co-Authored-By: Claude Opus 5.5 <[email protected]>
menu_vsync (two getenv a present), HOS_FS_OVERLAY (every OpenFile) and RX_VK_DUMP_STAGES (every code read) asked the
environment each time. They are set for a whole run; the V-Sync fixture now runs a process per switch setting.
(RX_VK_TAAU_KEYS stays per dispatch: tests/test_taau_env_lookup.py pins that read, about once a frame.)

Co-Authored-By: Claude Opus 5.5 <[email protected]>
(sol review: two first OpenFile calls could see the flag set before the pointer.)

Co-Authored-By: Claude Opus 5.5 <[email protected]>
(sol review round 2: the first signals of several threads read and wrote it at once.)

Co-Authored-By: Claude Opus 5.5 <[email protected]>
vk_shader.cpp is hashed into the shader-cache stamp (nx2/build.py), so any change there makes every player's translated
programs and prepared pipelines stale: a full re-prepare for a debug switch on the translation path, which does not run
in play once programs are prepared.

Co-Authored-By: Claude Opus 5.5 <[email protected]>
github-sync commented 2026-11-12 14:44:27 +00:00
Author
Owner

@Joey-Magnus approved on GitHub · 2026-11-12 14:34 UTC · original

I merged this into current main (51da0464) and ran tests/run_all.py: everything passes except test_lift_cache.py, which fails on main here too. The new test_thread_arg_self.py and test_cv_signal_early.py pass, and so does the reworked test_vsync_row.py.

I checked the Signal early return closely, since a missed wake would hang a thread. cv_enter counts the waiter in under cv->lk before mutex_release_all, and the gen snapshot is taken there too. So a signaller that skips at host/nn_os_sync.c:605 is ordered before the waiter's entry, and the old code would have missed that waiter in the same way. thread_arg's shortcut returns the same record hos_thread_of would. The once-read switches are run-wide.

Two notes. The Measured section still says provisional, on 57ed3aa8 and only together with #2709; please put in the re-measure on current main when you have it. #2765 adds BroadcastConditionVariable right below Signal. It merges cleanly with this, but it could take the same no-waiter return later.

> [@Joey-Magnus](https://github.com/Joey-Magnus) approved on GitHub · 2026-11-12 14:34 UTC · [original](https://github.com/NotRexed/Hookshot/pull/2708#pullrequestreview-5876236516) I merged this into current main (51da0464) and ran tests/run_all.py: everything passes except test_lift_cache.py, which fails on main here too. The new test_thread_arg_self.py and test_cv_signal_early.py pass, and so does the reworked test_vsync_row.py. I checked the Signal early return closely, since a missed wake would hang a thread. cv_enter counts the waiter in under cv->lk before mutex_release_all, and the gen snapshot is taken there too. So a signaller that skips at host/nn_os_sync.c:605 is ordered before the waiter's entry, and the old code would have missed that waiter in the same way. thread_arg's shortcut returns the same record hos_thread_of would. The once-read switches are run-wide. Two notes. The Measured section still says provisional, on 57ed3aa8 and only together with #2709; please put in the re-measure on current main when you have it. #2765 adds BroadcastConditionVariable right below Signal. It merges cleanly with this, but it could take the same no-waiter return later. <!-- github-sync:r:5876236516 -->
github-sync commented 2026-11-12 19:01:54 +00:00
Author
Owner

@mooki000 commented on GitHub · 2026-11-12 18:57 UTC · original

Hardware: RTX 4090 24 GB, NVIDIA 615.78.08, i9-13900K, Arch Linux 7.2.15-1-omarchy.
Built 8ee32e5c6139e6fd0d1dac68f3c16196cd507a60, on branch base c544828ff2d545829505cd894bb7968c7c2c5fcb.

The head is now f0943875: the same six commits (identical patches) plus two merges of main, the first of which brought #2709's neighbouring idle-thread and short-sleep changes. I did not run that combination.

Mixed Linux result: no lasting gameplay hang in these checks; CPU savings remain unmeasured. I compared HOS_CV_SIGNAL_ALWAYS=1 with unset on one binary. Windowless, muted, requested 2048x1152, game's own upscaler at 100%, frame generation off, 60 cap. These were ordinary shared runs, not quiet speed measurements.

Both modes loaded copied saves, used the game pause menu, transferred from the forest to the field entrance, and wrote a new progress save. I captured the old-mode Insert menu and the new-mode saving indicator. Both dungeon runs used the blue warp, played the story movie through its decoder stop, and continued the later dialogue.

Activity / Signal Wall seconds Load before -> after Longest present gap, including loads (ms)
Forest to field entrance / old 471.6 22.77 -> 126.63 28529.45
Forest to field entrance / new 471.2 74.46 -> 21.28 4534.01
Dungeon and story / old 430.8 40.68 -> 14.73 4544.36
Dungeon and story / new 430.8 34.29 -> 26.98 4701.05

All reached their timers. The gaps resumed; they are stall observations under unequal load/cache conditions, not a timing win. Both story runs logged the same background mutex wait of 10 seconds. The movie decoded 7040 pictures in each mode, but skipped 174 with old Signal and 1740 with new Signal to catch up: the movie finished, but I cannot call playback equally smooth or attribute that difference from this shared pair.

Removed work: a local logging-only diff counted calls and actual early returns. I can share it: 27 added lines and one removed line; the original return condition is unchanged. Counts were enabled only for the correctness runs.

Counted interval Signal Seconds Calls/frame Early returns/frame
Forest, including pause old 175.0 1910.2 0.0
Forest, including pause new 175.0 1909.4 1909.4
Field entrance corridor old 175.7 656.9 0.0
Field entrance corridor new 170.0 634.3 634.3

Those returns avoid the lock pair and wake call. The corridor is not the heavy open-field view.

CPU comparison: one old/new/old/new set loaded the same open-field save, with call counting off and the game's own processor counters recorded. The loads were:

Order / Signal Load before -> after Highest sampled load
1 / old 122.52 -> 64.39 122.52
2 / new 64.39 -> 38.52 64.40
3 / old 38.52 -> 35.52 38.52
4 / new 35.52 -> 49.87 49.96

Not comparable under this load. No CPU difference or FPS claim, and no repeat to chase quieter conditions.

Across all eight starts: no fatal error, crash, device loss or kernel driver fault. Two startup virtual-core escapes occurred, both with old Signal. One known NVIDIA mapping-refusal line overlapped the new story run; no Xid or unknown driver signature.

Checks: all three requested tests pass with GCC 16.2.1 and Clang 23.1.3. Against main 46780503, the own-thread and V-Sync checks fail at their behavioural assertions; the Signal check stops at its absent new extraction anchor, not an executed lost-wake test. Five Windows-target fixture units pass syntax checking, not Windows execution. No full-suite rerun.

Pictures inspected: labelled forest/pause/save panels, both field-entrance views, paired dungeon/movie/later-dialogue panels, and all four open-field positions. The new tour's sparse captures missed the Insert overlay itself. Stills do not prove absence of flicker.

Old and new Signal side by side: 1-2 the forest after loading the same copied save, 3-4 the game's pause menu, 5 the Insert menu with the old Signal, 6 an area transfer with the Saving indicator with the new one

Old and new Signal side by side: 1-2 the dungeon warp dialogue, 3-4 the story movie, 5-6 the dialogue after the movie

Not tested: the merged-main combination above, Windows execution, Intel/AMD Linux, real-window V-Sync, a non-empty filesystem overlay, audible audio continuity, a long soak, or reloading the newly written entrance save. This isolates the Signal switch, not the total CPU saving of all three changes.

> [@mooki000](https://github.com/mooki000) commented on GitHub · 2026-11-12 18:57 UTC · [original](https://github.com/NotRexed/Hookshot/pull/2708#issuecomment-6833020156) Hardware: RTX 4090 24 GB, NVIDIA 615.78.08, i9-13900K, Arch Linux 7.2.15-1-omarchy. Built `8ee32e5c6139e6fd0d1dac68f3c16196cd507a60`, on branch base `c544828ff2d545829505cd894bb7968c7c2c5fcb`. The head is now `f0943875`: the same six commits (identical patches) plus two merges of main, the first of which brought #2709's neighbouring idle-thread and short-sleep changes. I did not run that combination. **Mixed Linux result: no lasting gameplay hang in these checks; CPU savings remain unmeasured.** I compared `HOS_CV_SIGNAL_ALWAYS=1` with unset on one binary. Windowless, muted, requested 2048x1152, game's own upscaler at 100%, frame generation off, 60 cap. These were ordinary shared runs, not quiet speed measurements. Both modes loaded copied saves, used the game pause menu, transferred from the forest to the field entrance, and wrote a new progress save. I captured the old-mode Insert menu and the new-mode saving indicator. Both dungeon runs used the blue warp, played the story movie through its decoder stop, and continued the later dialogue. | Activity / Signal | Wall seconds | Load before -> after | Longest present gap, including loads (ms) | |---|---:|---|---:| | Forest to field entrance / old | 471.6 | 22.77 -> 126.63 | 28529.45 | | Forest to field entrance / new | 471.2 | 74.46 -> 21.28 | 4534.01 | | Dungeon and story / old | 430.8 | 40.68 -> 14.73 | 4544.36 | | Dungeon and story / new | 430.8 | 34.29 -> 26.98 | 4701.05 | All reached their timers. The gaps resumed; they are stall observations under unequal load/cache conditions, not a timing win. Both story runs logged the same background mutex wait of 10 seconds. The movie decoded 7040 pictures in each mode, but skipped 174 with old Signal and 1740 with new Signal to catch up: the movie finished, but I cannot call playback equally smooth or attribute that difference from this shared pair. **Removed work:** a local logging-only diff counted calls and actual early returns. I can share it: 27 added lines and one removed line; the original return condition is unchanged. Counts were enabled only for the correctness runs. | Counted interval | Signal | Seconds | Calls/frame | Early returns/frame | |---|---|---:|---:|---:| | Forest, including pause | old | 175.0 | 1910.2 | 0.0 | | Forest, including pause | new | 175.0 | 1909.4 | 1909.4 | | Field entrance corridor | old | 175.7 | 656.9 | 0.0 | | Field entrance corridor | new | 170.0 | 634.3 | 634.3 | Those returns avoid the lock pair and wake call. The corridor is not the heavy open-field view. **CPU comparison:** one old/new/old/new set loaded the same open-field save, with call counting off and the game's own processor counters recorded. The loads were: | Order / Signal | Load before -> after | Highest sampled load | |---|---|---:| | 1 / old | 122.52 -> 64.39 | 122.52 | | 2 / new | 64.39 -> 38.52 | 64.40 | | 3 / old | 38.52 -> 35.52 | 38.52 | | 4 / new | 35.52 -> 49.87 | 49.96 | **Not comparable under this load.** No CPU difference or FPS claim, and no repeat to chase quieter conditions. Across all eight starts: no fatal error, crash, device loss or kernel driver fault. Two startup virtual-core escapes occurred, both with old Signal. One known NVIDIA mapping-refusal line overlapped the new story run; no Xid or unknown driver signature. **Checks:** all three requested tests pass with GCC 16.2.1 and Clang 23.1.3. Against main `46780503`, the own-thread and V-Sync checks fail at their behavioural assertions; the Signal check stops at its absent new extraction anchor, not an executed lost-wake test. Five Windows-target fixture units pass syntax checking, not Windows execution. No full-suite rerun. **Pictures inspected:** labelled forest/pause/save panels, both field-entrance views, paired dungeon/movie/later-dialogue panels, and all four open-field positions. The new tour's sparse captures missed the Insert overlay itself. Stills do not prove absence of flicker. ![Old and new Signal side by side: 1-2 the forest after loading the same copied save, 3-4 the game's pause menu, 5 the Insert menu with the old Signal, 6 an area transfer with the Saving indicator with the new one](https://gist.githubusercontent.com/mooki000/b86bd3f35969803fb032f71f035844f8/raw/05a17968d1286d62b36046b7b68c5d5add8d1d85/133-pr2708-play-pause-save.jpg) ![Old and new Signal side by side: 1-2 the dungeon warp dialogue, 3-4 the story movie, 5-6 the dialogue after the movie](https://gist.githubusercontent.com/mooki000/b86bd3f35969803fb032f71f035844f8/raw/05a17968d1286d62b36046b7b68c5d5add8d1d85/134-pr2708-dungeon-story.jpg) **Not tested:** the merged-main combination above, Windows execution, Intel/AMD Linux, real-window V-Sync, a non-empty filesystem overlay, audible audio continuity, a long soak, or reloading the newly written entrance save. This isolates the Signal switch, not the total CPU saving of all three changes. <!-- github-sync:c:6833020156 -->
Commenting is not possible because the repository is archived.
Reviewers
No reviewers
Labels
Clear labels   
accessibility
Barrier affecting people with disabilities

  
autorelease: pending

   
autorelease: tagged

   
bug
Something isn't working

  
dependencies
Pull requests that update a dependency file

  
documentation
Improvements or additions to documentation

  
duplicate
This issue or pull request already exists

  
enhancement
New feature or request

  
github_actions
Pull requests that update GitHub Actions code

  
good first issue
Good for newcomers

  
help wanted
Extra attention is needed

  
invalid
This doesn't seem right

  
question
Further information is requested

  
testing wanted

   
wontfix
This will not be worked on

No labels
accessibility
autorelease: pending
autorelease: tagged
bug
dependencies
documentation
duplicate
enhancement
github_actions
good first issue
help wanted
invalid
question
testing wanted
wontfix
Milestone
Clear milestone
No items
No milestone
Assignees
Clear assignees
No assignees
1 participant
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/Hookshot!2708
No description provided.